Use the same issuer in token and discovery responses #24
Conversation
|
I see that there are no tests for Laravel stuff including DiscoveryController. It means that some tests fail because can't use |
|
I added forcing of |
| return $this->config | ||
| ->builder() | ||
| ->permittedFor($accessToken->getClient()->getIdentifier()) | ||
| ->issuedBy($issuer) | ||
| ->issuedBy('https://' . $_SERVER['HTTP_HOST']) |
There was a problem hiding this comment.
I don't think we should force the https here, we could use the default value from laravel and fallback on https only if missing.
Locally, we may want to work on unsecured connection.
There was a problem hiding this comment.
To be fair it was 'https://' . $_SERVER['HTTP_HOST'] before #13
| $response = [ | ||
| 'issuer' => url('/'), | ||
| 'issuer' => 'https://' . $_SERVER['HTTP_HOST'], |
There was a problem hiding this comment.
@jeremy379 This broke my unit test which was calling it without a HTTP_HOST. Maybe a good idea to fallback on both to route('/'), so also for the Id & Access tokens?
There was a problem hiding this comment.
@christiaangoossens @jeremy379 Same here. Since this is a Laravel package, I think we need to stop using $_SERVER variables, and use the Request object to obtain the needed urls.
Made an issue #32
There was a problem hiding this comment.
Hello
I'm sorry for these issue
Request isn't directly available here, so I choose to provide option on how this should be filled.
Here is a fix MR: #33
OIDC clients compare discovered issuer with token's issuer. So, these must be the same.
Mentioned in #23.