OIDC: Cleaned up provider settings, added extra validation

- Added endpoint validation to ensure HTTPS as per spec
- Added some missing types
- Removed redirectUri from OidcProviderSettings since it's not a
  provider-based setting, but a setting for the oauth client, so
  extracted that back to service.
This commit is contained in:
Dan Brown 2024-04-16 15:19:51 +01:00
parent dc6013fd7e
commit d640411adb
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
2 changed files with 17 additions and 8 deletions

View File

@ -18,7 +18,6 @@ class OidcProviderSettings
public string $issuer;
public string $clientId;
public string $clientSecret;
public ?string $redirectUri;
public ?string $authorizationEndpoint;
public ?string $tokenEndpoint;
public ?string $endSessionEndpoint;
@ -38,7 +37,7 @@ class OidcProviderSettings
/**
* Apply an array of settings to populate setting properties within this class.
*/
protected function applySettingsFromArray(array $settingsArray)
protected function applySettingsFromArray(array $settingsArray): void
{
foreach ($settingsArray as $key => $value) {
if (property_exists($this, $key)) {
@ -52,7 +51,7 @@ class OidcProviderSettings
*
* @throws InvalidArgumentException
*/
protected function validateInitial()
protected function validateInitial(): void
{
$required = ['clientId', 'clientSecret', 'redirectUri', 'issuer'];
foreach ($required as $prop) {
@ -74,12 +73,20 @@ class OidcProviderSettings
public function validate(): void
{
$this->validateInitial();
$required = ['keys', 'tokenEndpoint', 'authorizationEndpoint'];
foreach ($required as $prop) {
if (empty($this->$prop)) {
throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value");
}
}
$endpointProperties = ['tokenEndpoint', 'authorizationEndpoint', 'userinfoEndpoint'];
foreach ($endpointProperties as $prop) {
if (is_string($this->$prop) && !str_starts_with($this->$prop, 'https://')) {
throw new InvalidArgumentException("Endpoint value for \"{$prop}\" must start with https://");
}
}
}
/**
@ -87,7 +94,7 @@ class OidcProviderSettings
*
* @throws OidcIssuerDiscoveryException
*/
public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes)
public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes): void
{
try {
$cacheKey = 'oidc-discovery::' . $this->issuer;
@ -180,9 +187,9 @@ class OidcProviderSettings
/**
* Get the settings needed by an OAuth provider, as a key=>value array.
*/
public function arrayForProvider(): array
public function arrayForOAuthProvider(): array
{
$settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint'];
$settingKeys = ['clientId', 'clientSecret', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint'];
$settings = [];
foreach ($settingKeys as $setting) {
$settings[$setting] = $this->$setting;

View File

@ -91,7 +91,6 @@ class OidcService
'issuer' => $config['issuer'],
'clientId' => $config['client_id'],
'clientSecret' => $config['client_secret'],
'redirectUri' => url('/oidc/callback'),
'authorizationEndpoint' => $config['authorization_endpoint'],
'tokenEndpoint' => $config['token_endpoint'],
'endSessionEndpoint' => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null,
@ -130,7 +129,10 @@ class OidcService
*/
protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider
{
$provider = new OidcOAuthProvider($settings->arrayForProvider(), [
$provider = new OidcOAuthProvider([
...$settings->arrayForOAuthProvider(),
'redirectUri' => url('/oidc/callback'),
], [
'httpClient' => $this->http->buildClient(5),
'optionProvider' => new HttpBasicAuthOptionProvider(),
]);