From 1217969e22ffbbaadb1dbfe6d081389845d9b7ce Mon Sep 17 00:00:00 2001 From: iMarKoLiGa Date: Fri, 20 Sep 2024 16:19:54 +0200 Subject: [PATCH] feat: Ability to configure OIDC claims source (#888) Closes #884 --- docker-compose-dev.yml | 1 + docker-compose.yml | 1 + server/.env.sample | 1 + .../access-tokens/exchange-using-oidc.js | 8 +-- .../users/get-or-create-one-using-oidc.js | 59 +++++++++++-------- server/api/helpers/utils/send-webhooks.js | 4 +- server/config/custom.js | 1 + 7 files changed, 46 insertions(+), 29 deletions(-) diff --git a/docker-compose-dev.yml b/docker-compose-dev.yml index 88a6433f..b62db9d4 100644 --- a/docker-compose-dev.yml +++ b/docker-compose-dev.yml @@ -37,6 +37,7 @@ services: # - OIDC_RESPONSE_MODE=fragment # - OIDC_USE_DEFAULT_RESPONSE_MODE=true # - OIDC_ADMIN_ROLES=admin + # - OIDC_CLAIMS_SOURCE=userinfo # - OIDC_EMAIL_ATTRIBUTE=email # - OIDC_NAME_ATTRIBUTE=name # - OIDC_USERNAME_ATTRIBUTE=preferred_username diff --git a/docker-compose.yml b/docker-compose.yml index ecbf6eaa..d825cbac 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -44,6 +44,7 @@ services: # - OIDC_RESPONSE_MODE=fragment # - OIDC_USE_DEFAULT_RESPONSE_MODE=true # - OIDC_ADMIN_ROLES=admin + # - OIDC_CLAIMS_SOURCE=userinfo # - OIDC_EMAIL_ATTRIBUTE=email # - OIDC_NAME_ATTRIBUTE=name # - OIDC_USERNAME_ATTRIBUTE=preferred_username diff --git a/server/.env.sample b/server/.env.sample index c7418927..46442b40 100644 --- a/server/.env.sample +++ b/server/.env.sample @@ -35,6 +35,7 @@ SECRET_KEY=notsecretkey # OIDC_RESPONSE_MODE=fragment # OIDC_USE_DEFAULT_RESPONSE_MODE=true # OIDC_ADMIN_ROLES=admin +# OIDC_CLAIMS_SOURCE=userinfo # OIDC_EMAIL_ATTRIBUTE=email # OIDC_NAME_ATTRIBUTE=name # OIDC_USERNAME_ATTRIBUTE=preferred_username diff --git a/server/api/controllers/access-tokens/exchange-using-oidc.js b/server/api/controllers/access-tokens/exchange-using-oidc.js index 689f6138..1fcf06e0 100644 --- a/server/api/controllers/access-tokens/exchange-using-oidc.js +++ b/server/api/controllers/access-tokens/exchange-using-oidc.js @@ -6,8 +6,8 @@ const Errors = { INVALID_CODE_OR_NONCE: { invalidCodeOrNonce: 'Invalid code or nonce', }, - INVALID_USERINFO_SIGNATURE: { - invalidUserinfoSignature: 'Invalid signature on userinfo due to client misconfiguration', + INVALID_USERINFO_CONFIGURATION: { + invalidUserinfoConfiguration: 'Invalid userinfo configuration', }, EMAIL_ALREADY_IN_USE: { emailAlreadyInUse: 'Email already in use', @@ -40,7 +40,7 @@ module.exports = { invalidCodeOrNonce: { responseType: 'unauthorized', }, - invalidUserinfoSignature: { + invalidUserinfoConfiguration: { responseType: 'unauthorized', }, emailAlreadyInUse: { @@ -63,7 +63,7 @@ module.exports = { sails.log.warn(`Invalid code or nonce! (IP: ${remoteAddress})`); return Errors.INVALID_CODE_OR_NONCE; }) - .intercept('invalidUserinfoSignature', () => Errors.INVALID_USERINFO_SIGNATURE) + .intercept('invalidUserinfoConfiguration', () => Errors.INVALID_USERINFO_CONFIGURATION) .intercept('emailAlreadyInUse', () => Errors.EMAIL_ALREADY_IN_USE) .intercept('usernameAlreadyInUse', () => Errors.USERNAME_ALREADY_IN_USE) .intercept('missingValues', () => Errors.MISSING_VALUES); diff --git a/server/api/helpers/users/get-or-create-one-using-oidc.js b/server/api/helpers/users/get-or-create-one-using-oidc.js index 465a55b6..2c3cb65f 100644 --- a/server/api/helpers/users/get-or-create-one-using-oidc.js +++ b/server/api/helpers/users/get-or-create-one-using-oidc.js @@ -12,7 +12,7 @@ module.exports = { exits: { invalidCodeOrNonce: {}, - invalidUserinfoSignature: {}, + invalidUserinfoConfiguration: {}, missingValues: {}, emailAlreadyInUse: {}, usernameAlreadyInUse: {}, @@ -21,9 +21,9 @@ module.exports = { async fn(inputs) { const client = sails.hooks.oidc.getClient(); - let userInfo; + let tokenSet; try { - const tokenSet = await client.callback( + tokenSet = await client.callback( sails.config.custom.oidcRedirectUri, { iss: sails.config.custom.oidcIssuer, @@ -33,23 +33,36 @@ module.exports = { nonce: inputs.nonce, }, ); - userInfo = await client.userinfo(tokenSet); - } catch (e) { - if ( - e instanceof SyntaxError && - e.message.includes('Unexpected token e in JSON at position 0') - ) { - sails.log.warn('Error while exchanging OIDC code: userinfo response is signed'); - throw 'invalidUserinfoSignature'; - } - - sails.log.warn(`Error while exchanging OIDC code: ${e}`); + } catch (error) { + sails.log.warn(`Error while exchanging OIDC code: ${error}`); throw 'invalidCodeOrNonce'; } + let claims; + if (sails.config.custom.oidcClaimsSource === 'id_token') { + claims = tokenSet.claims(); + } else { + try { + claims = await client.userinfo(tokenSet); + } catch (error) { + let errorText; + if ( + error instanceof SyntaxError && + error.message.includes('Unexpected token e in JSON at position 0') + ) { + errorText = 'response is signed'; + } else { + errorText = error.toString(); + } + + sails.log.warn(`Error while fetching OIDC userinfo: ${errorText}`); + throw 'invalidUserinfoConfiguration'; + } + } + if ( - !userInfo[sails.config.custom.oidcEmailAttribute] || - !userInfo[sails.config.custom.oidcNameAttribute] + !claims[sails.config.custom.oidcEmailAttribute] || + !claims[sails.config.custom.oidcNameAttribute] ) { throw 'missingValues'; } @@ -58,23 +71,23 @@ module.exports = { if (sails.config.custom.oidcAdminRoles.includes('*')) { isAdmin = true; } else { - const roles = userInfo[sails.config.custom.oidcRolesAttribute]; + const roles = claims[sails.config.custom.oidcRolesAttribute]; if (Array.isArray(roles)) { // Use a Set here to avoid quadratic time complexity - const userRoles = new Set(userInfo[sails.config.custom.oidcRolesAttribute]); + const userRoles = new Set(claims[sails.config.custom.oidcRolesAttribute]); isAdmin = sails.config.custom.oidcAdminRoles.findIndex((role) => userRoles.has(role)) > -1; } } const values = { isAdmin, - email: userInfo[sails.config.custom.oidcEmailAttribute], + email: claims[sails.config.custom.oidcEmailAttribute], isSso: true, - name: userInfo[sails.config.custom.oidcNameAttribute], + name: claims[sails.config.custom.oidcNameAttribute], subscribeToOwnCards: false, }; if (!sails.config.custom.oidcIgnoreUsername) { - values.username = userInfo[sails.config.custom.oidcUsernameAttribute]; + values.username = claims[sails.config.custom.oidcUsernameAttribute]; } let user; @@ -84,7 +97,7 @@ module.exports = { // concurrently with logging in via OIDC. let identityProviderUser = await IdentityProviderUser.findOne({ issuer: sails.config.custom.oidcIssuer, - sub: userInfo.sub, + sub: claims.sub, }); if (identityProviderUser) { @@ -108,7 +121,7 @@ module.exports = { identityProviderUser = await IdentityProviderUser.create({ userId: user.id, issuer: sails.config.custom.oidcIssuer, - sub: userInfo.sub, + sub: claims.sub, }); } diff --git a/server/api/helpers/utils/send-webhooks.js b/server/api/helpers/utils/send-webhooks.js index addfb8af..0458a875 100644 --- a/server/api/helpers/utils/send-webhooks.js +++ b/server/api/helpers/utils/send-webhooks.js @@ -130,8 +130,8 @@ async function sendWebhook(webhook, event, data, user) { `Webhook ${webhook.url} failed with status ${response.status} and message: ${message}`, ); } - } catch (e) { - sails.log.error(`Webhook ${webhook.url} failed with error message: ${e.message}`); + } catch (error) { + sails.log.error(`Webhook ${webhook.url} failed with error: ${error}`); } } diff --git a/server/config/custom.js b/server/config/custom.js index cfafaf01..770695b7 100644 --- a/server/config/custom.js +++ b/server/config/custom.js @@ -52,6 +52,7 @@ module.exports.custom = { oidcResponseMode: process.env.OIDC_RESPONSE_MODE || 'fragment', oidcUseDefaultResponseMode: process.env.OIDC_USE_DEFAULT_RESPONSE_MODE === 'true', oidcAdminRoles: process.env.OIDC_ADMIN_ROLES ? process.env.OIDC_ADMIN_ROLES.split(',') : [], + oidcClaimsSource: process.env.OIDC_CLAIMS_SOURCE || 'userinfo', oidcEmailAttribute: process.env.OIDC_EMAIL_ATTRIBUTE || 'email', oidcNameAttribute: process.env.OIDC_NAME_ATTRIBUTE || 'name', oidcUsernameAttribute: process.env.OIDC_USERNAME_ATTRIBUTE || 'preferred_username',