From 7034e519413b07e3d54d181293808758ae7729b5 Mon Sep 17 00:00:00 2001 From: dschom Date: Fri, 28 Jul 2023 16:44:33 -0700 Subject: [PATCH] bug(settings): Fix issue with 'scope' string format Because: - Password reset flow would fail due to an invalid scope being set to the auth server's /authorization endpoint This Commit: - Ports code and tests from content-server's _normalizeScopesAndPermissions --- .../src/lib/reliers/relier-factory.test.ts | 16 +- .../oauth-redirect-integration.ts | 6 +- .../src/models/reliers/base-relier.test.ts | 4 +- .../src/models/reliers/base-relier.ts | 4 +- .../src/models/reliers/oauth-relier.test.ts | 169 +++++++++++++++++- .../src/models/reliers/oauth-relier.ts | 64 ++++++- .../ResetPasswordConfirmed/index.test.tsx | 2 +- .../src/pages/ResetPassword/index.test.tsx | 2 +- packages/fxa-shared/test/oauth/scopes.js | 1 + 9 files changed, 246 insertions(+), 22 deletions(-) diff --git a/packages/fxa-settings/src/lib/reliers/relier-factory.test.ts b/packages/fxa-settings/src/lib/reliers/relier-factory.test.ts index 764042c7cb1..8c97e92db17 100644 --- a/packages/fxa-settings/src/lib/reliers/relier-factory.test.ts +++ b/packages/fxa-settings/src/lib/reliers/relier-factory.test.ts @@ -149,7 +149,7 @@ describe('lib/reliers/relier-factory', () => { expect(relier.isOAuth()).toBeFalsy(); expect(await relier.isSync()).toBeFalsy(); expect(relier.wantsKeys()).toBeFalsy(); - expect(relier.isTrusted()).toBeTruthy(); + expect(await relier.isTrusted()).toBeTruthy(); }); // TODO: Remove with approval. @@ -185,7 +185,7 @@ describe('lib/reliers/relier-factory', () => { expect(relier.isOAuth()).toBeFalsy(); expect(await relier.isSync()).toBeTruthy(); expect(relier.wantsKeys()).toBeTruthy(); - expect(relier.isTrusted()).toBeTruthy(); + expect(await relier.isTrusted()).toBeTruthy(); }); it('populates model from the search parameters', async () => { @@ -214,6 +214,12 @@ describe('lib/reliers/relier-factory', () => { { initRelier: 1, initOAuthRelier: 1, initClientInfo: 1 }, (r: Relier) => r instanceof OAuthRelier ); + + sandbox.stub(relier, 'clientInfo').returns( + Promise.resolve({ + trusted: true, + }) + ); }); it('has correct state', async () => { @@ -221,7 +227,7 @@ describe('lib/reliers/relier-factory', () => { expect(relier.isOAuth()).toBeTruthy(); expect(await relier.isSync()).toBeFalsy(); expect(relier.wantsKeys()).toBeFalsy(); - expect(relier.isTrusted()).toBeFalsy(); + expect(await relier.isTrusted()).toBeFalsy(); }); // TODO: Port remaining tests from content-server }); @@ -245,7 +251,7 @@ describe('lib/reliers/relier-factory', () => { expect(relier.isOAuth()).toBeTruthy(); expect(await relier.isSync()).toBeFalsy(); expect(relier.wantsKeys()).toBeFalsy(); - expect(relier.isTrusted()).toBeFalsy(); + expect(await relier.isTrusted()).toBeFalsy(); }); }); @@ -268,7 +274,7 @@ describe('lib/reliers/relier-factory', () => { expect(relier.isOAuth()).toBeTruthy(); expect(await relier.isSync()).toBeFalsy(); expect(relier.wantsKeys()).toBeFalsy(); - expect(relier.isTrusted()).toBeFalsy(); + expect(await relier.isTrusted()).toBeFalsy(); }); }); }); diff --git a/packages/fxa-settings/src/models/integrations/oauth-redirect-integration.ts b/packages/fxa-settings/src/models/integrations/oauth-redirect-integration.ts index c04750666aa..026603316ed 100644 --- a/packages/fxa-settings/src/models/integrations/oauth-redirect-integration.ts +++ b/packages/fxa-settings/src/models/integrations/oauth-redirect-integration.ts @@ -94,7 +94,7 @@ export class OAuthRedirectIntegration { const clientKeyData = await this.callbacks.getOAuthScopedKeyData( sessionToken, this.relier.clientId, - this.relier.scope + await this.relier.getNormalizedScope() ); if (clientKeyData && Object.keys(clientKeyData).length > 0) { @@ -127,7 +127,7 @@ export class OAuthRedirectIntegration { acr_values: this.relier.acrValues, code_challenge: this.relier.codeChallenge, code_challenge_method: this.relier.codeChallengeMethod, - scope: this.relier.scope, + scope: await this.relier.getNormalizedScope(), }; if (keysJwe) { opts.keys_jwe = keysJwe; @@ -159,7 +159,7 @@ export class OAuthRedirectIntegration { code: string; state: string; }) { - // Ensure a redirect was provided. With out this info, we can't relay the oauth code + // Ensure a redirect was provided. Without this info, we can't relay the oauth code // and state! if (!this.relier.redirectTo) { throw new OAuthErrorInvalidRedirectUri(); diff --git a/packages/fxa-settings/src/models/reliers/base-relier.test.ts b/packages/fxa-settings/src/models/reliers/base-relier.test.ts index fb072097f37..408465bb0ea 100644 --- a/packages/fxa-settings/src/models/reliers/base-relier.test.ts +++ b/packages/fxa-settings/src/models/reliers/base-relier.test.ts @@ -32,8 +32,8 @@ describe('BaseRelier Model', function () { }); describe('isTrusted', function () { - it('returns `true`', function () { - expect(model.isTrusted()).toBeTruthy(); + it('returns `true`', async () => { + expect(await model.isTrusted()).toBeTruthy(); }); }); diff --git a/packages/fxa-settings/src/models/reliers/base-relier.ts b/packages/fxa-settings/src/models/reliers/base-relier.ts index 30f53e1c88c..c22555fb168 100644 --- a/packages/fxa-settings/src/models/reliers/base-relier.ts +++ b/packages/fxa-settings/src/models/reliers/base-relier.ts @@ -61,7 +61,7 @@ export interface Relier extends RelierData { shouldOfferToSync(view: string): boolean; wantsKeys(): boolean; wantsTwoStepAuthentication(): boolean; - isTrusted(): boolean; + isTrusted(): Promise; validate(): void; getService(): string | undefined; getRedirectUri(): string | undefined; @@ -199,7 +199,7 @@ export class BaseRelier extends ModelDataProvider implements Relier { return this.service; } - isTrusted() { + async isTrusted() { return true; } diff --git a/packages/fxa-settings/src/models/reliers/oauth-relier.test.ts b/packages/fxa-settings/src/models/reliers/oauth-relier.test.ts index defb094a98e..5f863c88fac 100644 --- a/packages/fxa-settings/src/models/reliers/oauth-relier.test.ts +++ b/packages/fxa-settings/src/models/reliers/oauth-relier.test.ts @@ -2,8 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +import exp from 'constants'; import { ModelDataStore, GenericData } from '../../lib/model-data'; -import { OAuthRelier } from './oauth-relier'; +import { OAuthRelier, replaceItemInArray } from './oauth-relier'; describe('models/reliers/oauth-relier', function () { let data: ModelDataStore; @@ -25,5 +26,171 @@ describe('models/reliers/oauth-relier', function () { expect(model).toBeDefined(); }); + describe('scope', () => { + const SCOPE = 'profile:email profile:uid'; + const SCOPE_PROFILE = 'profile'; + const SCOPE_PROFILE_UNRECOGNIZED = 'profile:unrecognized'; + const SCOPE_WITH_PLUS = 'profile:email+profile:uid'; + const SCOPE_WITH_EXTRAS = + 'profile:email profile:uid profile:non_whitelisted'; + const SCOPE_WITH_OPENID = 'profile:email profile:uid openid'; + + function getRelierWithScope(scope: string) { + const relier = new OAuthRelier( + new GenericData({ + scope, + }), + new GenericData({}), + { + scopedKeysEnabled: true, + scopedKeysValidation: {}, + isPromptNoneEnabled: true, + isPromptNoneEnabledClientIds: [], + } + ); + + relier.isTrusted = async () => { + return true; + }; + + return relier; + } + + describe('is invalid', () => { + function getRelier(scope: string) { + return getRelierWithScope(scope); + } + + it('empty scope', async () => { + const relier = getRelier(''); + await expect(relier.getPermissions()).rejects.toThrow(); + }); + + it('whitespace scope', async () => { + const relier = getRelier(' '); + await expect(relier.getPermissions()).rejects.toThrow(); + }); + }); + + describe('is valid', () => { + function getRelier(scope: string) { + return getRelierWithScope(scope); + } + + it(`normalizes ${SCOPE}`, async () => { + const relier = getRelier(SCOPE); + expect(await relier.getNormalizedScope()).toEqual( + 'profile:email profile:uid' + ); + }); + + it(`transforms ${SCOPE} to permissions`, async () => { + const relier = getRelier(SCOPE); + expect(await relier.getPermissions()).toEqual([ + 'profile:email', + 'profile:uid', + ]); + }); + + it(`transforms ${SCOPE_WITH_PLUS}`, async () => { + const relier = getRelier(SCOPE_WITH_PLUS); + expect(await relier.getPermissions()).toEqual([ + 'profile:email', + 'profile:uid', + ]); + }); + }); + + describe('untrusted reliers', () => { + function getRelier(scope: string) { + const relier = getRelierWithScope(scope); + relier.isTrusted = async () => { + return false; + }; + return relier; + } + + it(`normalizes ${SCOPE_WITH_EXTRAS}`, async () => { + const relier = getRelier(SCOPE_WITH_EXTRAS); + expect(await relier.getNormalizedScope()).toBe(SCOPE); + }); + + it(`normalizes ${SCOPE_WITH_OPENID}`, async () => { + const relier = getRelier(SCOPE_WITH_OPENID); + expect(await relier.getNormalizedScope()).toBe(SCOPE_WITH_OPENID); + }); + + it(`prohibits ${SCOPE_PROFILE}`, async () => { + const relier = getRelier(SCOPE_PROFILE); + await expect(relier.getNormalizedScope()).rejects.toThrow(); + }); + + it(`prohibits ${SCOPE_PROFILE_UNRECOGNIZED}`, async () => { + const relier = getRelier(SCOPE_PROFILE_UNRECOGNIZED); + await expect(relier.getNormalizedScope()).rejects.toThrow(); + }); + }); + + describe('trusted reliers that do not ask for consent', () => { + function getRelier(scope: string) { + const relier = getRelierWithScope(scope); + relier.wantsConsent = () => { + return false; + }; + return relier; + } + + it(`normalizes ${SCOPE_WITH_EXTRAS}`, async () => { + const relier = getRelier(SCOPE_WITH_EXTRAS); + expect(await relier.getNormalizedScope()).toEqual(SCOPE_WITH_EXTRAS); + }); + + it(`normalizes ${SCOPE_PROFILE}`, async () => { + const relier = getRelier(SCOPE_PROFILE); + expect(await relier.getNormalizedScope()).toEqual(SCOPE_PROFILE); + }); + + it(`normalizes ${SCOPE_PROFILE_UNRECOGNIZED}`, async () => { + const relier = getRelier(SCOPE_PROFILE_UNRECOGNIZED); + expect(await relier.getNormalizedScope()).toEqual( + SCOPE_PROFILE_UNRECOGNIZED + ); + }); + }); + }); + + describe('replaceItemInArray', () => { + it('handles empty array', () => { + expect(replaceItemInArray([], 'foo', ['bar'])).toEqual([]); + }); + + it('handles miss', () => { + expect(replaceItemInArray(['a', 'b', 'c'], '', ['foo'])).toEqual([ + 'a', + 'b', + 'c', + ]); + }); + + it('replaces and preserves order', () => { + expect(replaceItemInArray(['a', 'b', 'c'], 'b', ['foo', 'bar'])).toEqual([ + 'a', + 'foo', + 'bar', + 'c', + ]); + }); + + it('handles duplicates', () => { + expect( + replaceItemInArray(['a', 'b', 'b', 'c', 'c'], 'b', ['foo', 'foo']) + ).toEqual(['a', 'foo', 'c']); + }); + + it('handles empty replacement', () => { + expect(replaceItemInArray(['a', 'b', 'c'], 'a', [])).toEqual(['b', 'c']); + }); + }); + // TODO: OAuth Relier Model Test Coverage }); diff --git a/packages/fxa-settings/src/models/reliers/oauth-relier.ts b/packages/fxa-settings/src/models/reliers/oauth-relier.ts index 7ed1af34b89..ab9c5ca48de 100644 --- a/packages/fxa-settings/src/models/reliers/oauth-relier.ts +++ b/packages/fxa-settings/src/models/reliers/oauth-relier.ts @@ -9,7 +9,7 @@ import { KeyTransforms as T, ModelValidation as V, } from '../../lib/model-data'; -import { OAuthError } from '../../lib/oauth'; +import { ERRORS, OAuthError } from '../../lib/oauth'; import { BaseRelier, Relier, @@ -153,9 +153,6 @@ export class OAuthRelier extends BaseRelier implements OAuthRelierData, Relier { @bind([V.isGreaterThanZero], T.snakeCase) maxAge: number | undefined; - @bind([V.isString]) - permissions: string | undefined; - @bind([V.isString]) prompt: string | undefined; @@ -196,6 +193,36 @@ export class OAuthRelier extends BaseRelier implements OAuthRelierData, Relier { return this.service || this.clientId; } + async getPermissions() { + // Ported from content server, search for _normalizeScopesAndPermissions + let permissions = Array.from(scopeStrToArray(this.scope || '')); + if (await this.isTrusted()) { + // We have to normalize `profile` into is expanded sub-scopes + // in order to show the consent screen. + if (this.wantsConsent()) { + permissions = replaceItemInArray( + permissions, + Constants.OAUTH_TRUSTED_PROFILE_SCOPE, + Constants.OAUTH_TRUSTED_PROFILE_SCOPE_EXPANSION + ); + } + } else { + permissions = sanitizeUntrustedPermissions(permissions); + } + + if (!permissions.length) { + console.trace('!!!!!!!!'); + throw new OAuthError(ERRORS.INVALID_PARAMETER.errno, { params: 'scope' }); + } + + return permissions; + } + + async getNormalizedScope() { + const permissions = await this.getPermissions(); + return permissions.join(' '); + } + restoreOAuthState() { const oauth = this.storageData.get('oauth') as any; @@ -222,6 +249,11 @@ export class OAuthRelier extends BaseRelier implements OAuthRelierData, Relier { } async getServiceName() { + const permissions = await this.getPermissions(); + if (permissions.includes(Constants.OAUTH_OLDSYNC_SCOPE)) { + return Constants.RELIER_SYNC_SERVICE_NAME; + } + // If the clientId and the service are the same, prefer the clientInfo if (this.service && this.clientId === this.service) { const clientInfo = await this.clientInfo; @@ -230,7 +262,7 @@ export class OAuthRelier extends BaseRelier implements OAuthRelierData, Relier { } } - return super.getServiceName(); + return await super.getServiceName(); } async getClientInfo(): Promise { @@ -254,8 +286,8 @@ export class OAuthRelier extends BaseRelier implements OAuthRelierData, Relier { return clientInfo.serviceName === Constants.RELIER_SYNC_SERVICE_NAME; } - isTrusted() { - return this.trusted === true; + async isTrusted() { + return (await this.clientInfo)?.trusted === true; } wantsConsent() { @@ -376,3 +408,21 @@ function scopeStrToArray(scopes: string) { .filter((x) => x.length > 0); return new Set(arrScopes); } + +export function replaceItemInArray( + array: Array, + itemToReplace: string, + replaceWith: Array +) { + return Array.from( + new Set( + array.map((item) => (item === itemToReplace ? replaceWith : item)).flat() + ) + ); +} + +function sanitizeUntrustedPermissions(permissions: Array) { + return permissions.filter((x) => + Constants.OAUTH_UNTRUSTED_ALLOWED_PERMISSIONS.includes(x) + ); +} diff --git a/packages/fxa-settings/src/pages/ResetPassword/ResetPasswordConfirmed/index.test.tsx b/packages/fxa-settings/src/pages/ResetPassword/ResetPasswordConfirmed/index.test.tsx index 8aab3bcb113..7b7cfb5db0d 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/ResetPasswordConfirmed/index.test.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/ResetPasswordConfirmed/index.test.tsx @@ -22,7 +22,7 @@ jest.mock('../../../models/hooks', () => { isSync() { return false; }, - getServiceName() { + async getServiceName() { return 'account settings'; }, }), diff --git a/packages/fxa-settings/src/pages/ResetPassword/index.test.tsx b/packages/fxa-settings/src/pages/ResetPassword/index.test.tsx index d970d13c756..6ef3fa56259 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/index.test.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/index.test.tsx @@ -134,7 +134,7 @@ describe('PageResetPassword', () => { render( , - 'client_id=123&service=123Done&resume=123abc&redirect_uri=foo', + 'client_id=123&service=123Done&resume=123abc&redirect_uri=foo&scope=profile:email', account ); diff --git a/packages/fxa-shared/test/oauth/scopes.js b/packages/fxa-shared/test/oauth/scopes.js index 83dfbaaea2d..959b1762c1c 100644 --- a/packages/fxa-shared/test/oauth/scopes.js +++ b/packages/fxa-shared/test/oauth/scopes.js @@ -118,6 +118,7 @@ describe('oauth/scopes:', () => { 'profile:email!:write', ':', '::', + 'profile+https://identity.mozilla.com/account/subscriptions', ':profile', 'profile::email', 'profile profile\0:email',