More Openid connect work #25
Conversation
This makes sense since xmlseclib is already a dependency of simplesamlphp
lib/Auth/Source/OpenIDConnect.php
Outdated
| if (!$endSessionEndpoint) { | ||
| Logger::debug("authoauth2: $providerLabel No urlEndSession configured, not doing anything for logout"); | ||
| Logger::debug("authoauth2: $providerLabel OP does not provided and 'end_session_endpoint', not doing anything for logout"); |
There was a problem hiding this comment.
This should be reworded, I think.
Logger::debug("authoauth2: $providerLabel OP does not provide an 'end_session_endpoint', not doing anything for logout");maybe?
| $this->handleRequestFromRequest($_REQUEST); | ||
| } | ||
|
|
||
| public function handleRequestFromRequest(array $request) |
There was a problem hiding this comment.
Why is this necessary? If we need the additional level of indirection, maybe handleRequestFromRequest() should be private?
If the reason for the extra method is to have a default, I would do that in the handleRequest() method. We cannot use a variable as a default value for a function argument, but we can default it to null and capture that in the code:
public function handleRequest(array $request = null)
{
$request = $request ?: $_REQUEST;
Logger::debug('authoauth2: logout request=' . var_export($request, true));
$config = \SimpleSAML\Configuration::getInstance();
$sourceId = $request['authSource'];
$as = new \SimpleSAML\Auth\Simple($sourceId);
$as->logout([
'oidc:localLogout' => true,
'ReturnTo' => $config->getBasePath().'logout.php',
]);
}It might be worth also to investigate the possibility of using Symfony\Component\HttpFoundation\Request objects instead of the array scalar type for the argument, although this depends heavily on the module and Patrick should have a better overview. The symfony/http-foundation package has been available as a dependency starting on SSP 1.17.0.
There was a problem hiding this comment.
The reason is to make it easier to write unit tests. Also, this is the way it was done for existing, similar code in the module.
There was a problem hiding this comment.
Uhm, I see... But wouldn't my suggestion make it possible to test as well? Then you have two options: build a request and pass it as an argument to handleRequest(), or take that same request and assign it to $_REQUEST.
There was a problem hiding this comment.
Indeed it would. I was just following existing code patterns in the module.
|
|
||
| protected function getScopeSeparator() | ||
| { | ||
| return ' '; |
There was a problem hiding this comment.
Could this be done with a class constant?
There was a problem hiding this comment.
This implements an interface from the upstream oauth2-client library. We could of course define a class constant and return that here, but I don't see the point of that. To replace the entire function with a constant would require api changes to the library we are using
There was a problem hiding this comment.
If it's an external library, nevermind. Just not worth it.
|
|
||
| protected function getDefaultScopes() | ||
| { | ||
| return 'openid profile'; |
There was a problem hiding this comment.
Same with this. If this is not ever going to change, I think it makes sense to have it as a constant in the class, so that we reduce the amount of methods and make the values available to anybody using the class, if that ever makes sense.
There was a problem hiding this comment.
Same response as above
| return $config; | ||
| } | ||
|
|
||
| public static function base64url_decode($input) { |
There was a problem hiding this comment.
Not sure this makes sense as a public method. It doesn't feel like something this class should offer as an interface. If it's only intended for internal use, I'd suggest making it private or at least protected.
| $n = self::base64url_decode($key['n']); | ||
| $keys[$kid] = \RobRichards\XMLSecLibs\XMLSecurityKey::convertRSA($n, $e); | ||
| } else { | ||
| error_log("Failed to load key data for key id: " . $kid); |
There was a problem hiding this comment.
Logging should be done through SimpleSAML\Logger interface.
There was a problem hiding this comment.
My bad, copied this from the cli script a bit too quickly.
| */ | ||
| public function getBaseAuthorizationUrl() | ||
| { | ||
| return $this->getOpenIDConfiguration()["authorization_endpoint"]; |
There was a problem hiding this comment.
It feels a little bit weird that we need to go through getOpenIDConfiguration() every time we need to do something with the configuration. Would it be an option to initialize the $openIdConfiguration property in the constructor, and then refer always to $this->openIdConfiguration instead? Are there any other entrypoints to the class apart from the constructor?
There was a problem hiding this comment.
getOpenIDConfiguration() does a http fetch. I personally does not like to do IO in constructors. It makes for ugly code, and also I think it might be possible to use this class without requiring that configuration in some cases and then it would be a waste of resources to do the network IO in the constructor.
There was a problem hiding this comment.
That's a good point. If it's possible to use this class in some way without ever loading a OIDC configuration from the OP, then it makes sense to keep it like it is 👍
| */ | ||
| public function getBaseAccessTokenUrl(array $params) | ||
| { | ||
| return $this->getOpenIDConfiguration()["token_endpoint"]; |
There was a problem hiding this comment.
It's also a bit weird that $params here and $token in the next one aren't even used. What happens if token_endpoint is missing from the configuration, btw? Is that even possible?
There was a problem hiding this comment.
These parameters are also defined in the oauth2-client library's api. No idea what they are supposed to be used for, but we can't just remove them here. As for missing the endpoint in the configuration, that would require a OP that doesn't comply with the spec. We should probably check that we have a minimum required config in getOpenODConfiguration() and give some sort of sensible feedback to the user if not.
There was a problem hiding this comment.
I think that's a sensible idea for the getOpenIDConfiguration() method, indeed! At least, we should not take things for granted, just in case.
sigmunau
force-pushed
the
openid-connect
branch
2 times, most recently
from
0fc8a37
to
dbdc03d
Compare
This replaces the script that generates authsource config with just the need to place the issuer url in the authsource config. It turns out that some of the openid connect providers out there cycles the signing keys so often that storing them directly in the config isn't a good idea. This commit also moves most of the OpenID Connect specific code into a new Provider class, that could in theory also be used by other users of the oauth2-client library
sigmunau
force-pushed
the
openid-connect
branch
from
dbdc03d
to
3ec0e4a
Compare
|
Request updated with sanity checks on getOpenIDConfiguration and style guide fixes |
|
Thank you @sigmunau |
This pull request improves upon my previous pull request in the following ways:
@jaimeperez can you review?