From d6681cc8bf0b96f89170bfd7bf646c8016c02e9f Mon Sep 17 00:00:00 2001 From: Xin Li Date: Mon, 5 Apr 2021 22:56:52 -0700 Subject: [PATCH 1/8] OIDC codeflow support --- src/auth/auth-config.ts | 89 +++++++++++++++++++++++ src/auth/index.ts | 41 +++++++++++ src/utils/error.ts | 8 +++ test/integration/auth.spec.ts | 33 +++++++++ test/unit/auth/auth-config.spec.ts | 110 +++++++++++++++++++++++++++++ 5 files changed, 281 insertions(+) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 26db94126e..6600ad255a 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -24,6 +24,7 @@ import MultiFactorConfigState = auth.MultiFactorConfigState; import AuthFactorType = auth.AuthFactorType; import EmailSignInProviderConfig = auth.EmailSignInProviderConfig; import OIDCAuthProviderConfig = auth.OIDCAuthProviderConfig; +import OAuthResponseType = auth.OAuthResponseType; import SAMLAuthProviderConfig = auth.SAMLAuthProviderConfig; /** A maximum of 10 test phone number / code pairs can be configured. */ @@ -75,6 +76,8 @@ export interface OIDCConfigServerRequest { issuer?: string; displayName?: string; enabled?: boolean; + clientSecret?: string; + responseType?: OAuthResponseType; [key: string]: any; } @@ -87,6 +90,8 @@ export interface OIDCConfigServerResponse { issuer?: string; displayName?: string; enabled?: boolean; + clientSecret?: string; + responseType?: OAuthResponseType; } /** The server side email configuration request interface. */ @@ -650,6 +655,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig { public readonly providerId: string; public readonly issuer: string; public readonly clientId: string; + public readonly clientSecret: string; + public readonly responseType: OAuthResponseType; /** * Converts a client side request to a OIDCConfigServerRequest which is the format @@ -676,6 +683,12 @@ export class OIDCConfig implements OIDCAuthProviderConfig { request.displayName = options.displayName; request.issuer = options.issuer; request.clientId = options.clientId; + if (typeof options.clientSecret !== 'undefined') { + request.clientSecret = options.clientSecret; + } + if (typeof options.responseType !== 'undefined') { + request.responseType = options.responseType; + } return request; } @@ -715,6 +728,12 @@ export class OIDCConfig implements OIDCAuthProviderConfig { providerId: true, clientId: true, issuer: true, + clientSecret: true, + responseType: true, + }; + const validResponseTypes = { + idToken: true, + code: true, }; if (!validator.isNonNullObject(options)) { throw new FirebaseAuthError( @@ -773,6 +792,59 @@ export class OIDCConfig implements OIDCAuthProviderConfig { '"OIDCAuthProviderConfig.displayName" must be a valid string.', ); } + if (typeof options.clientSecret !== 'undefined' && + !validator.isNonEmptyString(options.clientSecret)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_CONFIG, + '"OIDCAuthProviderConfig.clientSecret" must be a valid string.', + ); + } + if (typeof options.responseType !== 'undefined') { + let idTokenType = false; + let codeType = false; + for (const responseTypeKey in options.responseType) { + if (!(responseTypeKey in validResponseTypes)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_CONFIG, + `"${responseTypeKey}" is not a valid OAuthResponseType parameter.`, + ); + } else { + if (responseTypeKey == 'idToken') { + if (!validator.isBoolean(options.responseType.idToken)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, + ); + } + idTokenType = options.responseType && options.responseType.idToken; + } else if (responseTypeKey == 'code') { + if (!validator.isBoolean(options.responseType.code)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, + ); + } + codeType = options.responseType && options.responseType.code; + } + } + } + + // Exact one of OAuth response type needs to be set to true. + if ((idTokenType && codeType) || + (!idTokenType && !codeType)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_OAUTH_RESPONSETYPE, + 'Only exact one of the OAuth response types has to be set to true.', + ); + } + // If code flow is enabled, client secret must be provided. + if (codeType && typeof options.clientSecret === 'undefined') { + throw new FirebaseAuthError( + AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET, + 'The OAuth configuration client secret is required to enable OIDC code flow.', + ); + } + } } /** @@ -806,6 +878,21 @@ export class OIDCConfig implements OIDCAuthProviderConfig { // When enabled is undefined, it takes its default value of false. this.enabled = !!response.enabled; this.displayName = response.displayName; + + if (typeof response.clientSecret !== 'undefined') { + this.clientSecret = response.clientSecret; + } + // If we do not set responseType, we have idToken flow + // set to true by default. + if (typeof response.responseType !== 'undefined') { + this.responseType = response.responseType; + } else { + const responseType = { + idToken: true, + code: false, + } + this.responseType = responseType; + } } /** @return {OIDCAuthProviderConfig} The plain object representation of the OIDCConfig. */ @@ -816,6 +903,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig { providerId: this.providerId, issuer: this.issuer, clientId: this.clientId, + clientSecret: this.clientSecret, + responseType: this.responseType, }; } } diff --git a/src/auth/index.ts b/src/auth/index.ts index b1bc47f725..9d307097a9 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -1289,6 +1289,26 @@ export namespace auth { callbackURL?: string; } + /** + * The interface representing OIDC provider's response object for OAuth + * authorization flow. + * We need either one of them or both true. There are three different cases: + * If idToken true, code false, then we are doing hybrid flow. + * If idToken false, code true, then we are doing code flow. + * If idToken true, code true, then we are doing idToken flow. + */ + export interface OAuthResponseType { + /** + * Whether ID token is returned from IdP's authorization endpoint. + */ + idToken: boolean; + + /** + * Whether authorization code is returned from IdP's authorization endpoint. + */ + code: boolean; + } + /** * The [OIDC](https://openid.net/specs/openid-connect-core-1_0-final.html) Auth * provider configuration interface. An OIDC provider can be created via @@ -1321,6 +1341,16 @@ export namespace auth { * [spec](https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation). */ issuer: string; + + /** + * The OIDC provider's client secret to enable OIDC code flow. + */ + clientSecret?: string; + + /** + * The OIDC provider's response object for OAuth authorization flow. + */ + responseType?: OAuthResponseType; } /** @@ -1403,6 +1433,17 @@ export namespace auth { * configuration's value is not modified. */ issuer?: string; + + /** + * The OIDC provider's client secret to enable OIDC code flow. + * If not provided, the existing configuration's value is not modified. + */ + clientSecret?: string; + + /** + * The OIDC provider's response object for OAuth authorization flow. + */ + responseType?: OAuthResponseType; } /** diff --git a/src/utils/error.ts b/src/utils/error.ts index 5809a294b6..41952ce5a5 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -521,6 +521,10 @@ export class AuthClientErrorCode { code: 'invalid-provider-uid', message: 'The providerUid must be a valid provider uid string.', }; + public static INVALID_OAUTH_RESPONSETYPE = { + code: 'invalid-oauth-responsetype', + message: 'The oauth response type object must set exact one response type to true', + }; public static INVALID_SESSION_COOKIE_DURATION = { code: 'invalid-session-cookie-duration', message: 'The session cookie duration must be a valid number in milliseconds ' + @@ -593,6 +597,10 @@ export class AuthClientErrorCode { code: 'missing-oauth-client-id', message: 'The OAuth/OIDC configuration client ID must not be empty.', }; + public static MISSING_OAUTH_CLIENT_SECRET = { + code: 'missing-oauth-client-secret', + message: 'The OAuth configuration client secret is required to enable OIDC code flow.', + }; public static MISSING_PROVIDER_ID = { code: 'missing-provider-id', message: 'A valid provider ID must be provided in the request.', diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index 53174f2ff5..4d98a597dc 100644 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -1338,12 +1338,21 @@ describe('admin.auth', () => { enabled: true, issuer: 'https://oidc.com/issuer1', clientId: 'CLIENT_ID1', + responseType: { + idToken: true, + code: false, + }, }; const modifiedConfigOptions = { displayName: 'OIDC_DISPLAY_NAME3', enabled: false, issuer: 'https://oidc.com/issuer3', clientId: 'CLIENT_ID3', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; before(function() { @@ -1637,6 +1646,10 @@ describe('admin.auth', () => { enabled: true, issuer: 'https://oidc.com/issuer1', clientId: 'CLIENT_ID1', + responseType: { + idToken: true, + code: false, + }, }; const authProviderConfig2 = { providerId: randomOidcProviderId(), @@ -1644,6 +1657,11 @@ describe('admin.auth', () => { enabled: true, issuer: 'https://oidc.com/issuer2', clientId: 'CLIENT_ID2', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; const removeTempConfigs = (): Promise => { @@ -1716,6 +1734,11 @@ describe('admin.auth', () => { enabled: false, issuer: 'https://oidc.com/issuer3', clientId: 'CLIENT_ID3', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; return admin.auth().updateProviderConfig(authProviderConfig1.providerId, modifiedConfigOptions) .then((config) => { @@ -1729,6 +1752,11 @@ describe('admin.auth', () => { const deltaChanges = { displayName: 'OIDC_DISPLAY_NAME4', issuer: 'https://oidc.com/issuer4', + clientSecret: '', + responseType: { + idToken: true, + code: false, + }, }; // Only above fields should be modified. const modifiedConfigOptions = { @@ -1736,6 +1764,11 @@ describe('admin.auth', () => { enabled: false, issuer: 'https://oidc.com/issuer4', clientId: 'CLIENT_ID3', + clientSecret: '', + responseType: { + idToken: true, + code: false, + }, }; return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges) .then((config) => { diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index ad81c5e62c..e1d44beae8 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -727,6 +727,11 @@ describe('OIDCConfig', () => { issuer: 'https://oidc.com/issuer', displayName: 'oidcProviderName', enabled: true, + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; const serverResponse: OIDCConfigServerResponse = { name: 'projects/project_id/oauthIdpConfigs/oidc.provider', @@ -734,6 +739,11 @@ describe('OIDCConfig', () => { issuer: 'https://oidc.com/issuer', displayName: 'oidcProviderName', enabled: true, + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; const clientRequest: OIDCAuthProviderConfig = { providerId: 'oidc.provider', @@ -741,6 +751,11 @@ describe('OIDCConfig', () => { issuer: 'https://oidc.com/issuer', displayName: 'oidcProviderName', enabled: true, + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }; const config = new OIDCConfig(serverResponse); @@ -769,6 +784,29 @@ describe('OIDCConfig', () => { expect(config.enabled).to.be.true; }); + it('should set readonly property clientSecret', () => { + expect(config.clientSecret).to.equal('CLIENT_SECRET'); + }); + + it('should set readonly property expected responseType', () => { + const expectedResponseType = { + idToken: false, + code: true, + }; + expect(config.responseType).to.deep.equal(expectedResponseType); + }); + + it('should set readonly property default responseType', () => { + delete serverResponse.clientSecret; + delete serverResponse.responseType; + const expectedResponseType = { + idToken: true, + code: false, + }; + const testConfig = new OIDCConfig(serverResponse); + expect(testConfig.responseType).to.deep.equal(expectedResponseType); + }); + it('should throw on missing issuer', () => { const invalidResponse = deepCopy(serverResponse); delete invalidResponse.issuer; @@ -831,6 +869,11 @@ describe('OIDCConfig', () => { providerId: 'oidc.provider', issuer: 'https://oidc.com/issuer', clientId: 'CLIENT_ID', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + }, }); }); }); @@ -892,6 +935,43 @@ describe('OIDCConfig', () => { expect(() => OIDCConfig.validate(partialRequest, true)).not.to.throw(); }); + it('should throw on OAuth responseType contains invalid parameters', () => { + const invalidRequest = deepCopy(clientRequest) as any; + invalidRequest.responseType.token = true; + expect(() => OIDCConfig.validate(invalidRequest, true)) + .to.throw('"token" is not a valid OAuthResponseType parameter.'); + }); + + it('should not throw when exact one OAuth responseType is true', () => { + const validRequest = deepCopy(clientRequest) as any; + validRequest.responseType.code = false; + validRequest.responseType.idToken = true; + expect(() => OIDCConfig.validate(validRequest, true)).not.to.throw(); + }); + + it('should throw on two OAuth responseTypes set to true', () => { + const invalidRequest = deepCopy(clientRequest) as any; + invalidRequest.responseType.idToken = true; + invalidRequest.responseType.code = true; + expect(() => OIDCConfig.validate(invalidRequest, true)) + .to.throw('Only exact one of the OAuth response types has to be set to true.'); + }); + + it('should throw on no OAuth responseType set to true', () => { + const invalidRequest = deepCopy(clientRequest) as any; + invalidRequest.responseType.idToken = false; + invalidRequest.responseType.code = false; + expect(() => OIDCConfig.validate(invalidRequest, true)) + .to.throw('Only exact one of the OAuth response types has to be set to true.'); + }); + + it('should throw on no client secret when OAuth responseType code flow set to true', () => { + const invalidRequest = deepCopy(clientRequest) as any; + delete invalidRequest.clientSecret; + expect(() => OIDCConfig.validate(invalidRequest, true)) + .to.throw('The OAuth configuration client secret is required to enable OIDC code flow.'); + }); + const nonObjects = [null, NaN, 0, 1, true, false, '', 'a', [], [1, 'a'], _.noop]; nonObjects.forEach((request) => { it('should throw on non-null OIDCAuthProviderConfig object:' + JSON.stringify(request), () => { @@ -957,5 +1037,35 @@ describe('OIDCConfig', () => { .to.throw('"OIDCAuthProviderConfig.displayName" must be a valid string.'); }); }); + + const invalidClientSecrets = [null, NaN, 0, 1, true, false, '', [], [1, 'a'], {}, { a: 1 }, _.noop]; + invalidClientSecrets.forEach((invalidClientSecret) => { + it('should throw on invalid clientSecret:' + JSON.stringify(invalidClientSecret), () => { + const invalidClientRequest = deepCopy(clientRequest) as any; + invalidClientRequest.clientSecret = invalidClientSecret; + expect(() => OIDCConfig.validate(invalidClientRequest)) + .to.throw('"OIDCAuthProviderConfig.clientSecret" must be a valid string.'); + }); + }); + + const invalidOAuthResponseIdTokenBooleans = [null, NaN, 0, 1, 'invalid', '', [], [1, 'a'], {}, { a: 1 }, _.noop]; + invalidOAuthResponseIdTokenBooleans.forEach((invalidOAuthResponseIdTokenBoolean) => { + it('should throw on invalid responseType.idToken:' + JSON.stringify(invalidOAuthResponseIdTokenBoolean), () => { + const invalidClientRequest = deepCopy(clientRequest) as any; + invalidClientRequest.responseType.idToken = invalidOAuthResponseIdTokenBoolean; + expect(() => OIDCConfig.validate(invalidClientRequest)) + .to.throw('"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.'); + }); + }); + + const invalidOAuthResponseCodeBooleans = [null, NaN, 0, 1, 'invalid', '', [], [1, 'a'], {}, { a: 1 }, _.noop]; + invalidOAuthResponseCodeBooleans.forEach((invalidOAuthResponseCodeBoolean) => { + it('should throw on invalid responseType.code:' + JSON.stringify(invalidOAuthResponseCodeBoolean), () => { + const invalidClientRequest = deepCopy(clientRequest) as any; + invalidClientRequest.responseType.code = invalidOAuthResponseCodeBoolean; + expect(() => OIDCConfig.validate(invalidClientRequest)) + .to.throw('"OIDCAuthProviderConfig.responseType.code" must be a boolean.'); + }); + }); }); }); From 3a925a549313e0a1e7a97f35d9f7ef9fe659dfd5 Mon Sep 17 00:00:00 2001 From: Xin Li Date: Tue, 6 Apr 2021 11:58:53 -0700 Subject: [PATCH 2/8] improve configs to simulate the real cases --- src/auth/auth-config.ts | 9 +++++--- src/auth/index.ts | 4 ++-- test/integration/auth.spec.ts | 33 ++++++------------------------ test/unit/auth/auth-config.spec.ts | 14 +++++++++---- 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 6600ad255a..a6a327af3c 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -816,7 +816,9 @@ export class OIDCConfig implements OIDCAuthProviderConfig { `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, ); } - idTokenType = options.responseType && options.responseType.idToken; + if (options.responseType && options.responseType.idToken) { + idTokenType = options.responseType.idToken; + } } else if (responseTypeKey == 'code') { if (!validator.isBoolean(options.responseType.code)) { throw new FirebaseAuthError( @@ -824,7 +826,9 @@ export class OIDCConfig implements OIDCAuthProviderConfig { `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, ); } - codeType = options.responseType && options.responseType.code; + if (options.responseType && options.responseType.code) { + codeType = options.responseType.code; + } } } } @@ -889,7 +893,6 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } else { const responseType = { idToken: true, - code: false, } this.responseType = responseType; } diff --git a/src/auth/index.ts b/src/auth/index.ts index 9d307097a9..c06dc98673 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -1301,12 +1301,12 @@ export namespace auth { /** * Whether ID token is returned from IdP's authorization endpoint. */ - idToken: boolean; + idToken?: boolean; /** * Whether authorization code is returned from IdP's authorization endpoint. */ - code: boolean; + code?: boolean; } /** diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index 4d98a597dc..3ed5e4b46a 100644 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -1648,7 +1648,6 @@ describe('admin.auth', () => { clientId: 'CLIENT_ID1', responseType: { idToken: true, - code: false, }, }; const authProviderConfig2 = { @@ -1659,7 +1658,6 @@ describe('admin.auth', () => { clientId: 'CLIENT_ID2', clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }; @@ -1728,8 +1726,8 @@ describe('admin.auth', () => { }); }); - it('updateProviderConfig() successfully overwrites an OIDC config', () => { - const modifiedConfigOptions = { + it('updateProviderConfig() successfully partially modifies an OIDC config', () => { + const deltaChanges = { displayName: 'OIDC_DISPLAY_NAME3', enabled: false, issuer: 'https://oidc.com/issuer3', @@ -1740,34 +1738,15 @@ describe('admin.auth', () => { code: true, }, }; - return admin.auth().updateProviderConfig(authProviderConfig1.providerId, modifiedConfigOptions) - .then((config) => { - const modifiedConfig = deepExtend( - { providerId: authProviderConfig1.providerId }, modifiedConfigOptions); - assertDeepEqualUnordered(modifiedConfig, config); - }); - }); - - it('updateProviderConfig() successfully partially modifies an OIDC config', () => { - const deltaChanges = { - displayName: 'OIDC_DISPLAY_NAME4', - issuer: 'https://oidc.com/issuer4', - clientSecret: '', - responseType: { - idToken: true, - code: false, - }, - }; // Only above fields should be modified. const modifiedConfigOptions = { - displayName: 'OIDC_DISPLAY_NAME4', + displayName: 'OIDC_DISPLAY_NAME3', enabled: false, - issuer: 'https://oidc.com/issuer4', + issuer: 'https://oidc.com/issuer3', clientId: 'CLIENT_ID3', - clientSecret: '', + clientSecret: 'CLIENT_SECRET', responseType: { - idToken: true, - code: false, + code: true, }, }; return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges) diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index e1d44beae8..4c416ccca2 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -741,7 +741,6 @@ describe('OIDCConfig', () => { enabled: true, clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }; @@ -790,7 +789,6 @@ describe('OIDCConfig', () => { it('should set readonly property expected responseType', () => { const expectedResponseType = { - idToken: false, code: true, }; expect(config.responseType).to.deep.equal(expectedResponseType); @@ -801,7 +799,6 @@ describe('OIDCConfig', () => { delete serverResponse.responseType; const expectedResponseType = { idToken: true, - code: false, }; const testConfig = new OIDCConfig(serverResponse); expect(testConfig.responseType).to.deep.equal(expectedResponseType); @@ -871,7 +868,6 @@ describe('OIDCConfig', () => { clientId: 'CLIENT_ID', clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }); @@ -887,12 +883,22 @@ describe('OIDCConfig', () => { const updateRequest: OIDCUpdateAuthProviderRequest = { clientId: 'CLIENT_ID', displayName: 'OIDC_PROVIDER_DISPLAY_NAME', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + } }; const updateServerRequest: OIDCConfigServerRequest = { clientId: 'CLIENT_ID', displayName: 'OIDC_PROVIDER_DISPLAY_NAME', issuer: undefined, enabled: undefined, + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + } }; expect(OIDCConfig.buildServerRequest(updateRequest, true)).to.deep.equal(updateServerRequest); }); From 4f2e7bc389b01686d3bc0de4a74afb43be263703 Mon Sep 17 00:00:00 2001 From: Xin Li Date: Tue, 6 Apr 2021 12:23:14 -0700 Subject: [PATCH 3/8] update for changes in signiture --- etc/firebase-admin.api.md | 8 ++++++++ src/auth/auth-config.ts | 9 +++++--- src/auth/index.ts | 11 +++++----- test/integration/auth.spec.ts | 33 ++++++------------------------ test/unit/auth/auth-config.spec.ts | 14 +++++++++---- 5 files changed, 35 insertions(+), 40 deletions(-) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index eaf280aec8..9ce92d54fa 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -208,15 +208,23 @@ export namespace auth { export interface MultiFactorUpdateSettings { enrolledFactors: UpdateMultiFactorInfoRequest[] | null; } + export interface OAuthResponseType { + code?: boolean; + idToken?: boolean; + } export interface OIDCAuthProviderConfig extends AuthProviderConfig { clientId: string; + clientSecret?: string; issuer: string; + responseType?: OAuthResponseType; } export interface OIDCUpdateAuthProviderRequest { clientId?: string; + clientSecret?: string; displayName?: string; enabled?: boolean; issuer?: string; + responseType?: OAuthResponseType; } export interface PhoneIdentifier { // (undocumented) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 6600ad255a..a6a327af3c 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -816,7 +816,9 @@ export class OIDCConfig implements OIDCAuthProviderConfig { `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, ); } - idTokenType = options.responseType && options.responseType.idToken; + if (options.responseType && options.responseType.idToken) { + idTokenType = options.responseType.idToken; + } } else if (responseTypeKey == 'code') { if (!validator.isBoolean(options.responseType.code)) { throw new FirebaseAuthError( @@ -824,7 +826,9 @@ export class OIDCConfig implements OIDCAuthProviderConfig { `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, ); } - codeType = options.responseType && options.responseType.code; + if (options.responseType && options.responseType.code) { + codeType = options.responseType.code; + } } } } @@ -889,7 +893,6 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } else { const responseType = { idToken: true, - code: false, } this.responseType = responseType; } diff --git a/src/auth/index.ts b/src/auth/index.ts index 9d307097a9..83fb622d67 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -1292,21 +1292,20 @@ export namespace auth { /** * The interface representing OIDC provider's response object for OAuth * authorization flow. - * We need either one of them or both true. There are three different cases: - * If idToken true, code false, then we are doing hybrid flow. - * If idToken false, code true, then we are doing code flow. - * If idToken true, code true, then we are doing idToken flow. + * We need either of them to be true, there are two cases: + * If set code to true, then we are doing code flow. + * If set idToken to true, then we are doing idToken flow. */ export interface OAuthResponseType { /** * Whether ID token is returned from IdP's authorization endpoint. */ - idToken: boolean; + idToken?: boolean; /** * Whether authorization code is returned from IdP's authorization endpoint. */ - code: boolean; + code?: boolean; } /** diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index 4d98a597dc..3ed5e4b46a 100644 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -1648,7 +1648,6 @@ describe('admin.auth', () => { clientId: 'CLIENT_ID1', responseType: { idToken: true, - code: false, }, }; const authProviderConfig2 = { @@ -1659,7 +1658,6 @@ describe('admin.auth', () => { clientId: 'CLIENT_ID2', clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }; @@ -1728,8 +1726,8 @@ describe('admin.auth', () => { }); }); - it('updateProviderConfig() successfully overwrites an OIDC config', () => { - const modifiedConfigOptions = { + it('updateProviderConfig() successfully partially modifies an OIDC config', () => { + const deltaChanges = { displayName: 'OIDC_DISPLAY_NAME3', enabled: false, issuer: 'https://oidc.com/issuer3', @@ -1740,34 +1738,15 @@ describe('admin.auth', () => { code: true, }, }; - return admin.auth().updateProviderConfig(authProviderConfig1.providerId, modifiedConfigOptions) - .then((config) => { - const modifiedConfig = deepExtend( - { providerId: authProviderConfig1.providerId }, modifiedConfigOptions); - assertDeepEqualUnordered(modifiedConfig, config); - }); - }); - - it('updateProviderConfig() successfully partially modifies an OIDC config', () => { - const deltaChanges = { - displayName: 'OIDC_DISPLAY_NAME4', - issuer: 'https://oidc.com/issuer4', - clientSecret: '', - responseType: { - idToken: true, - code: false, - }, - }; // Only above fields should be modified. const modifiedConfigOptions = { - displayName: 'OIDC_DISPLAY_NAME4', + displayName: 'OIDC_DISPLAY_NAME3', enabled: false, - issuer: 'https://oidc.com/issuer4', + issuer: 'https://oidc.com/issuer3', clientId: 'CLIENT_ID3', - clientSecret: '', + clientSecret: 'CLIENT_SECRET', responseType: { - idToken: true, - code: false, + code: true, }, }; return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges) diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index e1d44beae8..4c416ccca2 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -741,7 +741,6 @@ describe('OIDCConfig', () => { enabled: true, clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }; @@ -790,7 +789,6 @@ describe('OIDCConfig', () => { it('should set readonly property expected responseType', () => { const expectedResponseType = { - idToken: false, code: true, }; expect(config.responseType).to.deep.equal(expectedResponseType); @@ -801,7 +799,6 @@ describe('OIDCConfig', () => { delete serverResponse.responseType; const expectedResponseType = { idToken: true, - code: false, }; const testConfig = new OIDCConfig(serverResponse); expect(testConfig.responseType).to.deep.equal(expectedResponseType); @@ -871,7 +868,6 @@ describe('OIDCConfig', () => { clientId: 'CLIENT_ID', clientSecret: 'CLIENT_SECRET', responseType: { - idToken: false, code: true, }, }); @@ -887,12 +883,22 @@ describe('OIDCConfig', () => { const updateRequest: OIDCUpdateAuthProviderRequest = { clientId: 'CLIENT_ID', displayName: 'OIDC_PROVIDER_DISPLAY_NAME', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + } }; const updateServerRequest: OIDCConfigServerRequest = { clientId: 'CLIENT_ID', displayName: 'OIDC_PROVIDER_DISPLAY_NAME', issuer: undefined, enabled: undefined, + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: true, + } }; expect(OIDCConfig.buildServerRequest(updateRequest, true)).to.deep.equal(updateServerRequest); }); From 4b86338987f8e7295ebaa7013bd975832ea32f8e Mon Sep 17 00:00:00 2001 From: Xin Li Date: Wed, 14 Apr 2021 17:54:02 -0700 Subject: [PATCH 4/8] resolve comments --- src/auth/auth-config.ts | 38 ++++++++++++++++-------------- src/utils/error.ts | 2 +- test/integration/auth.spec.ts | 36 +++++++++++++++++++++++++--- test/unit/auth/auth-config.spec.ts | 37 ++++++++++++++++++++++------- 4 files changed, 83 insertions(+), 30 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index a6a327af3c..a806aaa64d 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -655,7 +655,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { public readonly providerId: string; public readonly issuer: string; public readonly clientId: string; - public readonly clientSecret: string; + public readonly clientSecret?: string; public readonly responseType: OAuthResponseType; /** @@ -799,9 +799,11 @@ export class OIDCConfig implements OIDCAuthProviderConfig { '"OIDCAuthProviderConfig.clientSecret" must be a valid string.', ); } - if (typeof options.responseType !== 'undefined') { - let idTokenType = false; - let codeType = false; + if (validator.isNonNullObject(options.responseType) && typeof options.responseType !== 'undefined') { + let idTokenType; + let codeType; + let setIdTokenType = false; + let setCodeType = false; for (const responseTypeKey in options.responseType) { if (!(responseTypeKey in validResponseTypes)) { throw new FirebaseAuthError( @@ -809,36 +811,39 @@ export class OIDCConfig implements OIDCAuthProviderConfig { `"${responseTypeKey}" is not a valid OAuthResponseType parameter.`, ); } else { - if (responseTypeKey == 'idToken') { + if (responseTypeKey === 'idToken') { if (!validator.isBoolean(options.responseType.idToken)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, + '"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.', ); } - if (options.responseType && options.responseType.idToken) { + if (typeof options.responseType.idToken !== 'undefined') { idTokenType = options.responseType.idToken; + setIdTokenType = true; } - } else if (responseTypeKey == 'code') { + } else if (responseTypeKey === 'code') { if (!validator.isBoolean(options.responseType.code)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - `"OIDCAuthProviderConfig.responseType.${responseTypeKey}" must be a boolean.`, + '"OIDCAuthProviderConfig.responseType.code" must be a boolean.', ); } - if (options.responseType && options.responseType.code) { + if (typeof options.responseType.code !== 'undefined') { codeType = options.responseType.code; + setCodeType = true; } } } } - // Exact one of OAuth response type needs to be set to true. - if ((idTokenType && codeType) || - (!idTokenType && !codeType)) { + // Only one of OAuth response types can be set to true. + if ((setIdTokenType && setCodeType) && + ((idTokenType && codeType) || + (!idTokenType && !codeType))) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_OAUTH_RESPONSETYPE, - 'Only exact one of the OAuth response types has to be set to true.', + 'Only exactly one OAuth responseType should be set to true.', ); } // If code flow is enabled, client secret must be provided. @@ -891,10 +896,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { if (typeof response.responseType !== 'undefined') { this.responseType = response.responseType; } else { - const responseType = { - idToken: true, - } - this.responseType = responseType; + this.responseType = { idToken: true }; } } diff --git a/src/utils/error.ts b/src/utils/error.ts index 41952ce5a5..d53448ed63 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -523,7 +523,7 @@ export class AuthClientErrorCode { }; public static INVALID_OAUTH_RESPONSETYPE = { code: 'invalid-oauth-responsetype', - message: 'The oauth response type object must set exact one response type to true', + message: 'Only exactly one OAuth responseType should be set to true.', }; public static INVALID_SESSION_COOKIE_DURATION = { code: 'invalid-session-cookie-duration', diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index 3ed5e4b46a..c24a29d60c 100644 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -1740,6 +1740,7 @@ describe('admin.auth', () => { }; // Only above fields should be modified. const modifiedConfigOptions = { + providerId: authProviderConfig1.providerId, displayName: 'OIDC_DISPLAY_NAME3', enabled: false, issuer: 'https://oidc.com/issuer3', @@ -1751,12 +1752,41 @@ describe('admin.auth', () => { }; return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges) .then((config) => { - const modifiedConfig = deepExtend( - { providerId: authProviderConfig1.providerId }, modifiedConfigOptions); - assertDeepEqualUnordered(modifiedConfig, config); + assertDeepEqualUnordered(modifiedConfigOptions, config); }); }); + it('updateProviderConfig() with invalid oauth response type should be rejected', () => { + const deltaChanges = { + displayName: 'OIDC_DISPLAY_NAME4', + enabled: false, + issuer: 'https://oidc.com/issuer4', + clientId: 'CLIENT_ID4', + clientSecret: 'CLIENT_SECRET', + responseType: { + idToken: false, + code: false, + }, + }; + return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges). + should.eventually.be.rejected.and.have.property('code', 'auth/invalid-oauth-responsetype'); + }); + + it('updateProviderConfig() code flow with no client secret should be rejected', () => { + const deltaChanges = { + displayName: 'OIDC_DISPLAY_NAME5', + enabled: false, + issuer: 'https://oidc.com/issuer5', + clientId: 'CLIENT_ID5', + responseType: { + idToken: false, + code: true, + }, + }; + return admin.auth().updateProviderConfig(authProviderConfig1.providerId, deltaChanges). + should.eventually.be.rejected.and.have.property('code', 'auth/missing-oauth-client-secret'); + }); + it('deleteProviderConfig() successfully deletes an existing OIDC config', () => { return admin.auth().deleteProviderConfig(authProviderConfig1.providerId).then(() => { return admin.auth().getProviderConfig(authProviderConfig1.providerId) diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index 4c416ccca2..234936f309 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -795,12 +795,13 @@ describe('OIDCConfig', () => { }); it('should set readonly property default responseType', () => { - delete serverResponse.clientSecret; - delete serverResponse.responseType; + const testResponse = deepCopy(serverResponse); + delete testResponse.clientSecret; + delete testResponse.responseType; const expectedResponseType = { idToken: true, }; - const testConfig = new OIDCConfig(serverResponse); + const testConfig = new OIDCConfig(testResponse); expect(testConfig.responseType).to.deep.equal(expectedResponseType); }); @@ -943,24 +944,38 @@ describe('OIDCConfig', () => { it('should throw on OAuth responseType contains invalid parameters', () => { const invalidRequest = deepCopy(clientRequest) as any; - invalidRequest.responseType.token = true; + invalidRequest.responseType.unknownField = true; expect(() => OIDCConfig.validate(invalidRequest, true)) - .to.throw('"token" is not a valid OAuthResponseType parameter.'); + .to.throw('"unknownField" is not a valid OAuthResponseType parameter.'); }); - it('should not throw when exact one OAuth responseType is true', () => { + it('should not throw when exactly one OAuth responseType is true', () => { const validRequest = deepCopy(clientRequest) as any; validRequest.responseType.code = false; validRequest.responseType.idToken = true; expect(() => OIDCConfig.validate(validRequest, true)).not.to.throw(); }); + it('should not throw when only idToken responseType is set to true', () => { + const validRequest = deepCopy(clientRequest) as any; + const validResponseType = { idToken: true }; + validRequest.responseType = validResponseType; + expect(() => OIDCConfig.validate(validRequest, true)).not.to.throw(); + }); + + it('should not throw when only code responseType is set to true', () => { + const validRequest = deepCopy(clientRequest) as any; + const validResponseType = { code: true }; + validRequest.responseType = validResponseType; + expect(() => OIDCConfig.validate(validRequest, true)).not.to.throw(); + }); + it('should throw on two OAuth responseTypes set to true', () => { const invalidRequest = deepCopy(clientRequest) as any; invalidRequest.responseType.idToken = true; invalidRequest.responseType.code = true; expect(() => OIDCConfig.validate(invalidRequest, true)) - .to.throw('Only exact one of the OAuth response types has to be set to true.'); + .to.throw('Only exactly one OAuth responseType should be set to true.'); }); it('should throw on no OAuth responseType set to true', () => { @@ -968,7 +983,13 @@ describe('OIDCConfig', () => { invalidRequest.responseType.idToken = false; invalidRequest.responseType.code = false; expect(() => OIDCConfig.validate(invalidRequest, true)) - .to.throw('Only exact one of the OAuth response types has to be set to true.'); + .to.throw('Only exactly one OAuth responseType should be set to true.'); + }); + + it('should not throw when responseType is empty', () => { + const testRequest = deepCopy(clientRequest) as any; + testRequest.responseType = {}; + expect(() => OIDCConfig.validate(testRequest, true)).not.to.throw(); }); it('should throw on no client secret when OAuth responseType code flow set to true', () => { From e565db3bd328ed2348e0b414f8c20bcbe8923aec Mon Sep 17 00:00:00 2001 From: Xin Li Date: Thu, 15 Apr 2021 16:01:38 -0700 Subject: [PATCH 5/8] improve validator logic --- src/auth/auth-config.ts | 77 ++++++++++++++---------------- test/unit/auth/auth-config.spec.ts | 3 +- 2 files changed, 36 insertions(+), 44 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index a806aaa64d..61bf53d00e 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -800,59 +800,52 @@ export class OIDCConfig implements OIDCAuthProviderConfig { ); } if (validator.isNonNullObject(options.responseType) && typeof options.responseType !== 'undefined') { - let idTokenType; - let codeType; - let setIdTokenType = false; - let setCodeType = false; - for (const responseTypeKey in options.responseType) { - if (!(responseTypeKey in validResponseTypes)) { + Object.keys(options.responseType).forEach((key) => { + if (!(key in validResponseTypes)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_CONFIG, - `"${responseTypeKey}" is not a valid OAuthResponseType parameter.`, + `"${key}" is not a valid OAuthResponseType parameter.`, + ); + } + }); + + const idToken = options.responseType.idToken; + if (typeof idToken !== 'undefined') { + if (!validator.isBoolean(idToken)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + '"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.', ); - } else { - if (responseTypeKey === 'idToken') { - if (!validator.isBoolean(options.responseType.idToken)) { - throw new FirebaseAuthError( - AuthClientErrorCode.INVALID_ARGUMENT, - '"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.', - ); - } - if (typeof options.responseType.idToken !== 'undefined') { - idTokenType = options.responseType.idToken; - setIdTokenType = true; - } - } else if (responseTypeKey === 'code') { - if (!validator.isBoolean(options.responseType.code)) { - throw new FirebaseAuthError( - AuthClientErrorCode.INVALID_ARGUMENT, - '"OIDCAuthProviderConfig.responseType.code" must be a boolean.', - ); - } - if (typeof options.responseType.code !== 'undefined') { - codeType = options.responseType.code; - setCodeType = true; - } - } } } - + + const code = options.responseType.code; + if (typeof code !== 'undefined') { + if (!validator.isBoolean(code)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + '"OIDCAuthProviderConfig.responseType.code" must be a boolean.', + ); + } + + // If code flow is enabled, client secret must be provided. + if (typeof options.clientSecret === 'undefined') { + throw new FirebaseAuthError( + AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET, + 'The OAuth configuration client secret is required to enable OIDC code flow.', + ); + } + } + + const allKeys = Object.keys(options.responseType).length; + const enabledCount = Object.values(options.responseType).filter(Boolean).length; // Only one of OAuth response types can be set to true. - if ((setIdTokenType && setCodeType) && - ((idTokenType && codeType) || - (!idTokenType && !codeType))) { + if (allKeys > 1 && enabledCount != 1) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_OAUTH_RESPONSETYPE, 'Only exactly one OAuth responseType should be set to true.', ); } - // If code flow is enabled, client secret must be provided. - if (codeType && typeof options.clientSecret === 'undefined') { - throw new FirebaseAuthError( - AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET, - 'The OAuth configuration client secret is required to enable OIDC code flow.', - ); - } } } diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index 234936f309..77b2ea2fd3 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -958,8 +958,7 @@ describe('OIDCConfig', () => { it('should not throw when only idToken responseType is set to true', () => { const validRequest = deepCopy(clientRequest) as any; - const validResponseType = { idToken: true }; - validRequest.responseType = validResponseType; + validRequest.responseType = { idToken: true }; expect(() => OIDCConfig.validate(validRequest, true)).not.to.throw(); }); From 0a3650109adca05333069f3fe8a87fb2a6e5fc82 Mon Sep 17 00:00:00 2001 From: Xin Li Date: Mon, 19 Apr 2021 15:04:31 -0700 Subject: [PATCH 6/8] remove unnecessary logic --- src/auth/auth-config.ts | 4 ---- test/unit/auth/auth-config.spec.ts | 11 ----------- 2 files changed, 15 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 61bf53d00e..71bf2b0f88 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -884,12 +884,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig { if (typeof response.clientSecret !== 'undefined') { this.clientSecret = response.clientSecret; } - // If we do not set responseType, we have idToken flow - // set to true by default. if (typeof response.responseType !== 'undefined') { this.responseType = response.responseType; - } else { - this.responseType = { idToken: true }; } } diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index 77b2ea2fd3..f6006df748 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -794,17 +794,6 @@ describe('OIDCConfig', () => { expect(config.responseType).to.deep.equal(expectedResponseType); }); - it('should set readonly property default responseType', () => { - const testResponse = deepCopy(serverResponse); - delete testResponse.clientSecret; - delete testResponse.responseType; - const expectedResponseType = { - idToken: true, - }; - const testConfig = new OIDCConfig(testResponse); - expect(testConfig.responseType).to.deep.equal(expectedResponseType); - }); - it('should throw on missing issuer', () => { const invalidResponse = deepCopy(serverResponse); delete invalidResponse.issuer; From 01f0c34839cf25e08c8ac84a98bec989a8a22c48 Mon Sep 17 00:00:00 2001 From: Xin Li Date: Tue, 20 Apr 2021 16:46:38 -0700 Subject: [PATCH 7/8] add tests and fix errors --- src/auth/auth-config.ts | 18 ++++++++--------- test/unit/auth/auth-api-request.spec.ts | 26 ++++++++++++++++++++++--- test/unit/auth/auth-config.spec.ts | 12 ++++++++---- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 71bf2b0f88..cd0e72cb48 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -810,13 +810,11 @@ export class OIDCConfig implements OIDCAuthProviderConfig { }); const idToken = options.responseType.idToken; - if (typeof idToken !== 'undefined') { - if (!validator.isBoolean(idToken)) { - throw new FirebaseAuthError( - AuthClientErrorCode.INVALID_ARGUMENT, - '"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.', - ); - } + if (typeof idToken !== 'undefined' && !validator.isBoolean(idToken)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + '"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.', + ); } const code = options.responseType.code; @@ -829,7 +827,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } // If code flow is enabled, client secret must be provided. - if (typeof options.clientSecret === 'undefined') { + if (code === true && typeof options.clientSecret === 'undefined') { throw new FirebaseAuthError( AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET, 'The OAuth configuration client secret is required to enable OIDC code flow.', @@ -897,8 +895,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig { providerId: this.providerId, issuer: this.issuer, clientId: this.clientId, - clientSecret: this.clientSecret, - responseType: this.responseType, + clientSecret: deepCopy(this.clientSecret), + responseType: deepCopy(this.responseType), }; } } diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 2df01889dd..3465a0da38 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -3503,12 +3503,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', + clientSecret: 'CLIENT_SECRET', + responseType: { + code: true, + }, }; const expectedRequest = { displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', + clientSecret: 'CLIENT_SECRET', + responseType: { + code: true, + }, }; const expectedResult = utils.responseFrom(deepExtend({ name: `projects/project1/oauthIdpConfigs/${providerId}`, @@ -3594,12 +3602,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', + clientSecret: 'CLIENT_SECRET', + responseType: { + code: true, + }, }; const expectedRequest = { displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', + clientSecret: 'CLIENT_SECRET', + responseType: { + code: true, + }, }; const expectedResult = utils.responseFrom(deepExtend({ name: `projects/project_id/oauthIdpConfigs/${providerId}`, @@ -3611,10 +3627,14 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { enabled: false, clientId: 'NEW_CLIENT_ID', issuer: 'https://oidc.com/issuer2', + clientSecret: 'CLIENT_SECRET', + responseType: { + code: true, + }, })); it('should be fulfilled given full parameters', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult); stubs.push(stub); @@ -3708,7 +3728,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); it('should be rejected when the backend returns a response missing name', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; const expectedError = new FirebaseAuthError( AuthClientErrorCode.INTERNAL_ERROR, 'INTERNAL ASSERT FAILED: Unable to update OIDC configuration', @@ -3728,7 +3748,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); it('should be rejected when the backend returns an error', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; const expectedServerError = utils.errorFrom({ error: { message: 'INVALID_CONFIG', diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index f6006df748..d9e6ce4abc 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -788,10 +788,14 @@ describe('OIDCConfig', () => { }); it('should set readonly property expected responseType', () => { - const expectedResponseType = { - code: true, - }; - expect(config.responseType).to.deep.equal(expectedResponseType); + expect(config.responseType).to.deep.equal({ code: true }); + }); + + it('should not throw on no responseType and clientSecret', () => { + const testResponse = deepCopy(serverResponse); + delete testResponse.clientSecret; + delete testResponse.responseType; + expect(() => new OIDCConfig(testResponse)).not.to.throw(); }); it('should throw on missing issuer', () => { From 4fb8597eeeccdcbc2d8857ad95d40c5cfb1d8859 Mon Sep 17 00:00:00 2001 From: Xin Li Date: Fri, 23 Apr 2021 10:24:49 -0700 Subject: [PATCH 8/8] add auth-api-request rests --- src/auth/auth-config.ts | 2 +- test/unit/auth/auth-api-request.spec.ts | 97 +++++++++++++++++++------ 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index cd0e72cb48..059d866574 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -827,7 +827,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } // If code flow is enabled, client secret must be provided. - if (code === true && typeof options.clientSecret === 'undefined') { + if (code && typeof options.clientSecret === 'undefined') { throw new FirebaseAuthError( AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET, 'The OAuth configuration client secret is required to enable OIDC code flow.', diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 3465a0da38..0538cab3eb 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -3497,30 +3497,44 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const providerId = 'oidc.provider'; const path = handler.path('v2', `/oauthIdpConfigs?oauthIdpConfigId=${providerId}`, 'project_id'); const expectedHttpMethod = 'POST'; + const clientSecret = 'CLIENT_SECRET'; + const responseType = { code: true }; const configOptions = { providerId, displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', - clientSecret: 'CLIENT_SECRET', - responseType: { - code: true, - }, }; const expectedRequest = { displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', - clientSecret: 'CLIENT_SECRET', - responseType: { - code: true, - }, }; const expectedResult = utils.responseFrom(deepExtend({ name: `projects/project1/oauthIdpConfigs/${providerId}`, }, expectedRequest)); + const expectedCodeFlowOptions = { + providerId, + displayName: 'OIDC_DISPLAY_NAME', + enabled: true, + clientId: 'CLIENT_ID', + issuer: 'https://oidc.com/issuer', + clientSecret, + responseType, + }; + const expectedCodeFlowRequest = { + displayName: 'OIDC_DISPLAY_NAME', + enabled: true, + clientId: 'CLIENT_ID', + issuer: 'https://oidc.com/issuer', + clientSecret, + responseType, + }; + const expectedCodeFlowResult = utils.responseFrom(deepExtend({ + name: `projects/project1/oauthIdpConfigs/${providerId}`, + }, expectedCodeFlowRequest)); it('should be fulfilled given valid parameters', () => { const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult); @@ -3535,6 +3549,19 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); }); + it('should be fulfilled given valid parameters for OIDC code flow', () => { + const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedCodeFlowResult); + stubs.push(stub); + + const requestHandler = handler.init(mockApp); + return requestHandler.createOAuthIdpConfig(expectedCodeFlowOptions) + .then((response) => { + expect(response).to.deep.equal(expectedCodeFlowResult.data); + expect(stub).to.have.been.calledOnce.and.calledWith( + callParams(path, expectedHttpMethod, expectedCodeFlowRequest)); + }); + }); + it('should be rejected given invalid parameters', () => { const expectedError = new FirebaseAuthError( AuthClientErrorCode.INVALID_CONFIG, @@ -3597,25 +3624,19 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const providerId = 'oidc.provider'; const path = handler.path('v2', `/oauthIdpConfigs/${providerId}`, 'project_id'); const expectedHttpMethod = 'PATCH'; + const clientSecret = 'CLIENT_SECRET'; + const responseType = { code: true }; const configOptions = { displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', - clientSecret: 'CLIENT_SECRET', - responseType: { - code: true, - }, }; const expectedRequest = { displayName: 'OIDC_DISPLAY_NAME', enabled: true, clientId: 'CLIENT_ID', issuer: 'https://oidc.com/issuer', - clientSecret: 'CLIENT_SECRET', - responseType: { - code: true, - }, }; const expectedResult = utils.responseFrom(deepExtend({ name: `projects/project_id/oauthIdpConfigs/${providerId}`, @@ -3627,14 +3648,30 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { enabled: false, clientId: 'NEW_CLIENT_ID', issuer: 'https://oidc.com/issuer2', - clientSecret: 'CLIENT_SECRET', - responseType: { - code: true, - }, })); + const expectedCodeFlowOptions = { + providerId, + displayName: 'OIDC_DISPLAY_NAME', + enabled: true, + clientId: 'CLIENT_ID', + issuer: 'https://oidc.com/issuer', + clientSecret, + responseType, + }; + const expectedCodeFlowRequest = { + displayName: 'OIDC_DISPLAY_NAME', + enabled: true, + clientId: 'CLIENT_ID', + issuer: 'https://oidc.com/issuer', + clientSecret, + responseType, + }; + const expectedCodeFlowResult = utils.responseFrom(deepExtend({ + name: `projects/project1/oauthIdpConfigs/${providerId}`, + }, expectedCodeFlowRequest)); it('should be fulfilled given full parameters', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult); stubs.push(stub); @@ -3647,6 +3684,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); }); + it('should be fulfilled given full parameters for OIDC code flow', () => { + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; + const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedCodeFlowResult); + stubs.push(stub); + + const requestHandler = handler.init(mockApp); + return requestHandler.updateOAuthIdpConfig(providerId, expectedCodeFlowOptions) + .then((response) => { + expect(response).to.deep.equal(expectedCodeFlowResult.data); + expect(stub).to.have.been.calledOnce.and.calledWith( + callParams(expectedPath, expectedHttpMethod, expectedCodeFlowRequest)); + }); + }); + it('should be fulfilled given partial parameters', () => { const expectedPath = path + '?updateMask=enabled,clientId'; const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedPartialResult); @@ -3728,7 +3779,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); it('should be rejected when the backend returns a response missing name', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; const expectedError = new FirebaseAuthError( AuthClientErrorCode.INTERNAL_ERROR, 'INTERNAL ASSERT FAILED: Unable to update OIDC configuration', @@ -3748,7 +3799,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); it('should be rejected when the backend returns an error', () => { - const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code'; + const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId'; const expectedServerError = utils.errorFrom({ error: { message: 'INVALID_CONFIG',