Skip to content

Commit ad54f77

Browse files
mikeruddmikerudd
authored
Merge pull request #5 from talis/asp_2543_account_for_client_id_on_login
2 parents aac10eb + e3a86a1 commit ad54f77

File tree

5 files changed

+159
-37
lines changed

5 files changed

+159
-37
lines changed

‎docker-compose.yml

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,35 @@
11
version: '3.7'
22
services:
33
composer:
4-
image: composer:latest
4+
image: composer:2.2
55
environment:
66
- COMPOSER_CACHE_DIR=/app/.cache/composer
77
volumes:
88
- .:/app
9-
restart: never
9+
restart: never
1010
php55:
1111
image: php:5.5-cli
12-
volumes:
12+
volumes:
1313
- .:/app
1414
working_dir: /app
1515
restart: never
1616
php70:
1717
image: php:7.0-cli
18-
volumes:
18+
volumes:
1919
- .:/app
2020
working_dir: /app
21-
restart: never
21+
restart: never
2222
phpunit55:
2323
image: php:5.5-cli
2424
restart: never
2525
volumes:
2626
- .:/app
2727
working_dir: /app
28-
entrypoint: vendor/bin/phpunit
28+
entrypoint: vendor/bin/phpunit
2929
phpunit70:
3030
image: php:7.0-cli
3131
restart: never
3232
volumes:
3333
- .:/app
3434
working_dir: /app
35-
entrypoint: vendor/bin/phpunit
35+
entrypoint: vendor/bin/phpunit

‎src/lti/LTI_OIDC_Login.php

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<?php
2+
23
namespace IMSGlobal\LTI;
34

45
class LTI_OIDC_Login {
@@ -14,7 +15,8 @@ class LTI_OIDC_Login {
1415
* @param Cache $cache Instance of the Cache interface used to loading and storing launches. If non is provided launch data will be store in $_SESSION.
1516
* @param Cookie $cookie Instance of the Cookie interface used to set and read cookies. Will default to using $_COOKIE and setcookie.
1617
*/
17-
function __construct(Database $database, Cache $cache = null, Cookie $cookie = null) {
18+
function __construct(Database $database, Cache $cache = null, Cookie $cookie = null)
19+
{
1820
$this->db = $database;
1921
if ($cache === null) {
2022
$cache = new FileCache();
@@ -33,10 +35,11 @@ function __construct(Database $database, Cache $cache = null, Cookie $cookie = n
3335
* @param Database $database Instance of the database interface used for looking up registrations and deployments.
3436
* @param Cache $cache Instance of the Cache interface used to loading and storing launches. If non is provided launch data will be store in $_SESSION.
3537
* @param Cookie $cookie Instance of the Cookie interface used to set and read cookies. Will default to using $_COOKIE and setcookie.
36-
*
38+
*
3739
* @return LTI_OIDC_Login
3840
*/
39-
public static function newInstance(Database $database, Cache $cache = null, Cookie $cookie = null) {
41+
public static function newInstance(Database $database, Cache $cache = null, Cookie $cookie = null)
42+
{
4043
return new LTI_OIDC_Login($database, $cache, $cookie);
4144
}
4245

@@ -48,7 +51,8 @@ public static function newInstance(Database $database, Cache $cache = null, Cook
4851
*
4952
* @return Redirect Returns a redirect object containing the fully formed OIDC login URL.
5053
*/
51-
public function do_oidc_login_redirect($launch_url, array $request = null) {
54+
public function do_oidc_login_redirect($launch_url, array $request = null)
55+
{
5256

5357
if ($request === null) {
5458
$request = $_REQUEST;
@@ -100,7 +104,8 @@ public function do_oidc_login_redirect($launch_url, array $request = null) {
100104

101105
}
102106

103-
protected function validate_oidc_login($request) {
107+
protected function validate_oidc_login($request)
108+
{
104109

105110
// Validate Issuer.
106111
if (empty($request['iss'])) {
@@ -112,7 +117,7 @@ protected function validate_oidc_login($request) {
112117
throw new OIDC_Exception("Could not find login hint", 1);
113118
}
114119

115-
$clientId = isset($request['aud']) ? $request['aud'] : null;
120+
$clientId = $this->getClientIdFromRequest($request);
116121

117122
// Fetch Registration Details.
118123
$registration = $this->db->find_registration_by_issuer($request['iss'], $clientId);
@@ -125,4 +130,24 @@ protected function validate_oidc_login($request) {
125130
// Return Registration.
126131
return $registration;
127132
}
133+
134+
/**
135+
* Get the clientId from the request. If `client_id` and `aud` are present, then `client_id` has preference.
136+
*
137+
* @param array $request An array of request parameters.
138+
*
139+
* @return string|null
140+
*/
141+
private function getClientIdFromRequest(array $request)
142+
{
143+
$clientId = null;
144+
145+
if (isset($request['client_id'])) {
146+
$clientId = $request['client_id'];
147+
} elseif (isset($request['aud'])) {
148+
$clientId = $request['aud'];
149+
}
150+
151+
return $clientId;
152+
}
128153
}

‎tests/unit/LTI_OIDC_Login_Test.php

Lines changed: 93 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,17 @@
88
use IMSGlobal\LTI\Tests\unit\helpers\DummyDatabase;
99
use IMSGlobal\LTI\Tests\unit\helpers\InMemoryCache;
1010

11-
class LTI_OIDC_Login_Test extends TestBase {
11+
class LTI_OIDC_Login_Test extends TestBase
12+
{
1213
private $launchUrl = 'https://example.com/lti1p3/launch';
1314

1415
public function testNewInstance()
1516
{
1617
$this->assertInstanceOf(LTI_OIDC_Login::class, LTI_OIDC_Login::newInstance(
1718
new DummyDatabase()
1819
));
19-
}
20-
20+
}
21+
2122
public function testDoOidcLoginRedirectEmptyLaunchUrl()
2223
{
2324
$this->setExpectedException('IMSGlobal\LTI\OIDC_Exception', 'No launch URL configured');
@@ -31,7 +32,7 @@ public function testValidateOidcLoginNoIssuer()
3132
LTI_OIDC_Login::newInstance(new DummyDatabase())->do_oidc_login_redirect(
3233
$this->launchUrl,
3334
[]
34-
);
35+
);
3536
}
3637

3738
public function testValidateOidcLoginNoPasswordHint()
@@ -40,9 +41,9 @@ public function testValidateOidcLoginNoPasswordHint()
4041
LTI_OIDC_Login::newInstance(new DummyDatabase())->do_oidc_login_redirect(
4142
$this->launchUrl,
4243
['iss' => 'aaa']
43-
);
44-
}
45-
44+
);
45+
}
46+
4647
public function testValidateOidcLoginRegistrationNotFound()
4748
{
4849
$this->setExpectedException('IMSGlobal\LTI\OIDC_Exception', 'Could not find registration details');
@@ -51,7 +52,7 @@ public function testValidateOidcLoginRegistrationNotFound()
5152
$registrationDatabase = $this->getMockBuilder(DummyDatabase::class)
5253
->setMethods(['find_registration_by_issuer'])
5354
->getMock();
54-
55+
5556
$registrationDatabase->expects($this->atLeastOnce())
5657
->method('find_registration_by_issuer')
5758
->with('xyz', null)
@@ -63,25 +64,25 @@ public function testValidateOidcLoginRegistrationNotFound()
6364
'iss' => 'xyz',
6465
'login_hint' => 'password123'
6566
]
66-
);
67-
}
68-
67+
);
68+
}
69+
6970
public function testDoOidcLoginRedirect()
7071
{
7172
/** @var InMemoryCache|\PHPUnit_Framework_MockObject_MockObject */
7273
$cache = $this->getMockBuilder(InMemoryCache::class)
7374
->setMethods(['cache_nonce'])
7475
->getMock();
75-
76+
7677
$cache->expects($this->once())->method('cache_nonce')->with(
7778
$this->stringStartsWith('nonce-')
7879
);
79-
80+
8081
/** @var Cookie|\PHPUnit_Framework_MockObject_MockObject $cookie */
8182
$cookie = $this->getMockBuilder(Cookie::class)
8283
->setMethods(['set_cookie'])
8384
->getMock();
84-
85+
8586
$cookie->expects($this->once())->method('set_cookie')->with(
8687
$this->stringStartsWith('lti1p3_state-'),
8788
$this->stringStartsWith('state-')
@@ -98,14 +99,14 @@ public function testDoOidcLoginRedirect()
9899
'login_hint' => 'password123',
99100
'lti_message_hint' => 'my message hint'
100101
]
101-
);
102-
102+
);
103+
103104
$this->assertInstanceOf(Redirect::class, $redirect);
104105

105106
$redirectUrl = parse_url($redirect->get_redirect_url());
106107

107108
$this->assertEquals('https', $redirectUrl['scheme']);
108-
$this->assertEquals('example.com', $redirectUrl['host']);
109+
$this->assertEquals('example.com', $redirectUrl['host']);
109110
$this->assertEquals('/lti1p3/aaa/12345/auth_login_url', $redirectUrl['path']);
110111
parse_str($redirectUrl['query'], $query);
111112
$this->assertEquals('openid', $query['scope']);
@@ -119,4 +120,79 @@ public function testDoOidcLoginRedirect()
119120
$this->assertEquals('password123', $query['login_hint']);
120121
$this->assertEquals('my message hint', $query['lti_message_hint']);
121122
}
123+
124+
/**
125+
* @dataProvider clientIdProvider
126+
* @param array $request The request array to use for the test
127+
*/
128+
public function testClientIdIsSetOnOidcLogin(array $request)
129+
{
130+
/** @var InMemoryCache|\PHPUnit_Framework_MockObject_MockObject */
131+
$cache = $this->getMockBuilder(InMemoryCache::class)
132+
->setMethods(['cache_nonce'])
133+
->getMock();
134+
135+
$cache->expects($this->once())->method('cache_nonce')->with(
136+
$this->stringStartsWith('nonce-')
137+
);
138+
139+
/** @var Cookie|\PHPUnit_Framework_MockObject_MockObject $cookie */
140+
$cookie = $this->getMockBuilder(Cookie::class)
141+
->setMethods(['set_cookie'])
142+
->getMock();
143+
144+
$cookie->expects($this->once())->method('set_cookie')->with(
145+
$this->stringStartsWith('lti1p3_state-'),
146+
$this->stringStartsWith('state-')
147+
)->willReturn($cookie);
148+
149+
$redirect = LTI_OIDC_Login::newInstance(
150+
new DummyDatabase(),
151+
$cache,
152+
$cookie
153+
)->do_oidc_login_redirect(
154+
$this->launchUrl,
155+
$request
156+
);
157+
158+
$expectedId = isset($request['client_id']) ? $request['client_id'] : $request['aud'];
159+
160+
$this->assertInstanceOf(Redirect::class, $redirect);
161+
162+
$redirectUrl = parse_url($redirect->get_redirect_url());
163+
164+
parse_str($redirectUrl['query'], $query);
165+
$this->assertEquals($expectedId, $query['client_id']);
166+
}
167+
168+
public function clientIdProvider()
169+
{
170+
return [
171+
'clientId as client_id' => [
172+
[
173+
'iss' => 'aaa',
174+
'login_hint' => 'password123',
175+
'lti_message_hint' => 'my message hint',
176+
'client_id' => '12345'
177+
]
178+
],
179+
'clientId as aud' => [
180+
[
181+
'iss' => 'bbb',
182+
'login_hint' => 'password123',
183+
'lti_message_hint' => 'my message hint',
184+
'aud' => 'aud_12345'
185+
]
186+
],
187+
'clientId and aud present' => [
188+
[
189+
'iss' => 'ccc',
190+
'login_hint' => 'password123',
191+
'lti_message_hint' => 'my message hint',
192+
'aud' => 'aud_12345',
193+
'client_id' => '12345'
194+
]
195+
]
196+
];
197+
}
122198
}

‎tests/unit/fixtures/registration_db.json

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,23 @@
55
"deployment_id": "aaa-12345",
66
"auth_login_url": "https://example.com/lti1p3/aaa/12345/auth_login_url",
77
"auth_token_url": "https://example.com/lti1p3/aaa/12345/auth_token_url",
8-
"key_set_url": "https://example.com/lti1p3/aaa/12345/key_set_url"
8+
"key_set_url": "https://example.com/lti1p3/aaa/12345/key_set_url"
9+
},
10+
{
11+
"issuer": "bbb",
12+
"aud": "aud_12345",
13+
"deployment_id": "bbb-12345",
14+
"auth_login_url": "https://example.com/lti1p3/bbb/aud_12345/auth_login_url",
15+
"auth_token_url": "https://example.com/lti1p3/bbb/aud_12345/auth_token_url",
16+
"key_set_url": "https://example.com/lti1p3/bbb/aud_12345/key_set_url"
17+
},
18+
{
19+
"issuer": "ccc",
20+
"aud": "aud_12345",
21+
"client_id": "12345",
22+
"deployment_id": "bbb-12345",
23+
"auth_login_url": "https://example.com/lti1p3/bbb/aud_12345/auth_login_url",
24+
"auth_token_url": "https://example.com/lti1p3/bbb/aud_12345/auth_token_url",
25+
"key_set_url": "https://example.com/lti1p3/bbb/aud_12345/key_set_url"
926
}
10-
]
27+
]

‎tests/unit/helpers/DummyDatabase.php

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
use IMSGlobal\LTI\LTI_Registration;
77
use IMSGlobal\LTI\LTI_Deployment;
88

9-
class DummyDatabase implements Database {
9+
class DummyDatabase implements Database
10+
{
1011
public function find_registration_by_issuer($iss, $clientId = null)
1112
{
1213
$privateKeyFileContents = file_get_contents(dirname(dirname(__FILE__)) . '/fixtures/private.key');
@@ -16,24 +17,27 @@ public function find_registration_by_issuer($iss, $clientId = null)
1617
$registrations = json_decode($registrationDBFile, true);
1718
foreach ($registrations as $registrationDetails) {
1819
if ($registrationDetails['issuer'] === $iss) {
19-
if (empty($clientId) || $registrationDetails['client_id'] === $clientId) {
20+
if (empty($clientId)
21+
|| $registrationDetails['client_id'] === $clientId
22+
|| $registrationDetails['aud'] === $clientId
23+
) {
2024
$details = $registrationDetails;
2125
break;
2226
}
2327
}
2428
}
2529
if (!empty($details)) {
2630
if (empty($clientId)) {
27-
$clientId = $details['client_id'];
31+
$clientId = isset($details['client_id'])? $details['client_id'] : $details['aud'];
2832
}
2933

3034
$registration = LTI_Registration::newInstance()
3135
->set_auth_login_url($details['auth_login_url'])
3236
->set_auth_token_url($details['auth_token_url'])
3337
->set_key_set_url($details['key_set_url'])
3438
->set_kid("key_{$iss}_{$clientId}")
35-
->set_tool_private_key($privateKeyFileContents);
36-
39+
->set_tool_private_key($privateKeyFileContents);
40+
3741

3842
$registration->set_client_id($clientId);
3943
return $registration;

0 commit comments

Comments
 (0)