Skip to content

Commit 7034e51

Browse files
dschomjulianpoy
authored andcommitted
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
1 parent 254b5a8 commit 7034e51

File tree

9 files changed

+246
-22
lines changed

9 files changed

+246
-22
lines changed

packages/fxa-settings/src/lib/reliers/relier-factory.test.ts

+11-5
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ describe('lib/reliers/relier-factory', () => {
149149
expect(relier.isOAuth()).toBeFalsy();
150150
expect(await relier.isSync()).toBeFalsy();
151151
expect(relier.wantsKeys()).toBeFalsy();
152-
expect(relier.isTrusted()).toBeTruthy();
152+
expect(await relier.isTrusted()).toBeTruthy();
153153
});
154154

155155
// TODO: Remove with approval.
@@ -185,7 +185,7 @@ describe('lib/reliers/relier-factory', () => {
185185
expect(relier.isOAuth()).toBeFalsy();
186186
expect(await relier.isSync()).toBeTruthy();
187187
expect(relier.wantsKeys()).toBeTruthy();
188-
expect(relier.isTrusted()).toBeTruthy();
188+
expect(await relier.isTrusted()).toBeTruthy();
189189
});
190190

191191
it('populates model from the search parameters', async () => {
@@ -214,14 +214,20 @@ describe('lib/reliers/relier-factory', () => {
214214
{ initRelier: 1, initOAuthRelier: 1, initClientInfo: 1 },
215215
(r: Relier) => r instanceof OAuthRelier
216216
);
217+
218+
sandbox.stub(relier, 'clientInfo').returns(
219+
Promise.resolve({
220+
trusted: true,
221+
})
222+
);
217223
});
218224

219225
it('has correct state', async () => {
220226
expect(relier.name).toEqual('oauth');
221227
expect(relier.isOAuth()).toBeTruthy();
222228
expect(await relier.isSync()).toBeFalsy();
223229
expect(relier.wantsKeys()).toBeFalsy();
224-
expect(relier.isTrusted()).toBeFalsy();
230+
expect(await relier.isTrusted()).toBeFalsy();
225231
});
226232
// TODO: Port remaining tests from content-server
227233
});
@@ -245,7 +251,7 @@ describe('lib/reliers/relier-factory', () => {
245251
expect(relier.isOAuth()).toBeTruthy();
246252
expect(await relier.isSync()).toBeFalsy();
247253
expect(relier.wantsKeys()).toBeFalsy();
248-
expect(relier.isTrusted()).toBeFalsy();
254+
expect(await relier.isTrusted()).toBeFalsy();
249255
});
250256
});
251257

@@ -268,7 +274,7 @@ describe('lib/reliers/relier-factory', () => {
268274
expect(relier.isOAuth()).toBeTruthy();
269275
expect(await relier.isSync()).toBeFalsy();
270276
expect(relier.wantsKeys()).toBeFalsy();
271-
expect(relier.isTrusted()).toBeFalsy();
277+
expect(await relier.isTrusted()).toBeFalsy();
272278
});
273279
});
274280
});

packages/fxa-settings/src/models/integrations/oauth-redirect-integration.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export class OAuthRedirectIntegration {
9494
const clientKeyData = await this.callbacks.getOAuthScopedKeyData(
9595
sessionToken,
9696
this.relier.clientId,
97-
this.relier.scope
97+
await this.relier.getNormalizedScope()
9898
);
9999

100100
if (clientKeyData && Object.keys(clientKeyData).length > 0) {
@@ -127,7 +127,7 @@ export class OAuthRedirectIntegration {
127127
acr_values: this.relier.acrValues,
128128
code_challenge: this.relier.codeChallenge,
129129
code_challenge_method: this.relier.codeChallengeMethod,
130-
scope: this.relier.scope,
130+
scope: await this.relier.getNormalizedScope(),
131131
};
132132
if (keysJwe) {
133133
opts.keys_jwe = keysJwe;
@@ -159,7 +159,7 @@ export class OAuthRedirectIntegration {
159159
code: string;
160160
state: string;
161161
}) {
162-
// Ensure a redirect was provided. With out this info, we can't relay the oauth code
162+
// Ensure a redirect was provided. Without this info, we can't relay the oauth code
163163
// and state!
164164
if (!this.relier.redirectTo) {
165165
throw new OAuthErrorInvalidRedirectUri();

packages/fxa-settings/src/models/reliers/base-relier.test.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ describe('BaseRelier Model', function () {
3232
});
3333

3434
describe('isTrusted', function () {
35-
it('returns `true`', function () {
36-
expect(model.isTrusted()).toBeTruthy();
35+
it('returns `true`', async () => {
36+
expect(await model.isTrusted()).toBeTruthy();
3737
});
3838
});
3939

packages/fxa-settings/src/models/reliers/base-relier.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export interface Relier extends RelierData {
6161
shouldOfferToSync(view: string): boolean;
6262
wantsKeys(): boolean;
6363
wantsTwoStepAuthentication(): boolean;
64-
isTrusted(): boolean;
64+
isTrusted(): Promise<boolean>;
6565
validate(): void;
6666
getService(): string | undefined;
6767
getRedirectUri(): string | undefined;
@@ -199,7 +199,7 @@ export class BaseRelier extends ModelDataProvider implements Relier {
199199
return this.service;
200200
}
201201

202-
isTrusted() {
202+
async isTrusted() {
203203
return true;
204204
}
205205

packages/fxa-settings/src/models/reliers/oauth-relier.test.ts

+168-1
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5+
import exp from 'constants';
56
import { ModelDataStore, GenericData } from '../../lib/model-data';
6-
import { OAuthRelier } from './oauth-relier';
7+
import { OAuthRelier, replaceItemInArray } from './oauth-relier';
78

89
describe('models/reliers/oauth-relier', function () {
910
let data: ModelDataStore;
@@ -25,5 +26,171 @@ describe('models/reliers/oauth-relier', function () {
2526
expect(model).toBeDefined();
2627
});
2728

29+
describe('scope', () => {
30+
const SCOPE = 'profile:email profile:uid';
31+
const SCOPE_PROFILE = 'profile';
32+
const SCOPE_PROFILE_UNRECOGNIZED = 'profile:unrecognized';
33+
const SCOPE_WITH_PLUS = 'profile:email+profile:uid';
34+
const SCOPE_WITH_EXTRAS =
35+
'profile:email profile:uid profile:non_whitelisted';
36+
const SCOPE_WITH_OPENID = 'profile:email profile:uid openid';
37+
38+
function getRelierWithScope(scope: string) {
39+
const relier = new OAuthRelier(
40+
new GenericData({
41+
scope,
42+
}),
43+
new GenericData({}),
44+
{
45+
scopedKeysEnabled: true,
46+
scopedKeysValidation: {},
47+
isPromptNoneEnabled: true,
48+
isPromptNoneEnabledClientIds: [],
49+
}
50+
);
51+
52+
relier.isTrusted = async () => {
53+
return true;
54+
};
55+
56+
return relier;
57+
}
58+
59+
describe('is invalid', () => {
60+
function getRelier(scope: string) {
61+
return getRelierWithScope(scope);
62+
}
63+
64+
it('empty scope', async () => {
65+
const relier = getRelier('');
66+
await expect(relier.getPermissions()).rejects.toThrow();
67+
});
68+
69+
it('whitespace scope', async () => {
70+
const relier = getRelier(' ');
71+
await expect(relier.getPermissions()).rejects.toThrow();
72+
});
73+
});
74+
75+
describe('is valid', () => {
76+
function getRelier(scope: string) {
77+
return getRelierWithScope(scope);
78+
}
79+
80+
it(`normalizes ${SCOPE}`, async () => {
81+
const relier = getRelier(SCOPE);
82+
expect(await relier.getNormalizedScope()).toEqual(
83+
'profile:email profile:uid'
84+
);
85+
});
86+
87+
it(`transforms ${SCOPE} to permissions`, async () => {
88+
const relier = getRelier(SCOPE);
89+
expect(await relier.getPermissions()).toEqual([
90+
'profile:email',
91+
'profile:uid',
92+
]);
93+
});
94+
95+
it(`transforms ${SCOPE_WITH_PLUS}`, async () => {
96+
const relier = getRelier(SCOPE_WITH_PLUS);
97+
expect(await relier.getPermissions()).toEqual([
98+
'profile:email',
99+
'profile:uid',
100+
]);
101+
});
102+
});
103+
104+
describe('untrusted reliers', () => {
105+
function getRelier(scope: string) {
106+
const relier = getRelierWithScope(scope);
107+
relier.isTrusted = async () => {
108+
return false;
109+
};
110+
return relier;
111+
}
112+
113+
it(`normalizes ${SCOPE_WITH_EXTRAS}`, async () => {
114+
const relier = getRelier(SCOPE_WITH_EXTRAS);
115+
expect(await relier.getNormalizedScope()).toBe(SCOPE);
116+
});
117+
118+
it(`normalizes ${SCOPE_WITH_OPENID}`, async () => {
119+
const relier = getRelier(SCOPE_WITH_OPENID);
120+
expect(await relier.getNormalizedScope()).toBe(SCOPE_WITH_OPENID);
121+
});
122+
123+
it(`prohibits ${SCOPE_PROFILE}`, async () => {
124+
const relier = getRelier(SCOPE_PROFILE);
125+
await expect(relier.getNormalizedScope()).rejects.toThrow();
126+
});
127+
128+
it(`prohibits ${SCOPE_PROFILE_UNRECOGNIZED}`, async () => {
129+
const relier = getRelier(SCOPE_PROFILE_UNRECOGNIZED);
130+
await expect(relier.getNormalizedScope()).rejects.toThrow();
131+
});
132+
});
133+
134+
describe('trusted reliers that do not ask for consent', () => {
135+
function getRelier(scope: string) {
136+
const relier = getRelierWithScope(scope);
137+
relier.wantsConsent = () => {
138+
return false;
139+
};
140+
return relier;
141+
}
142+
143+
it(`normalizes ${SCOPE_WITH_EXTRAS}`, async () => {
144+
const relier = getRelier(SCOPE_WITH_EXTRAS);
145+
expect(await relier.getNormalizedScope()).toEqual(SCOPE_WITH_EXTRAS);
146+
});
147+
148+
it(`normalizes ${SCOPE_PROFILE}`, async () => {
149+
const relier = getRelier(SCOPE_PROFILE);
150+
expect(await relier.getNormalizedScope()).toEqual(SCOPE_PROFILE);
151+
});
152+
153+
it(`normalizes ${SCOPE_PROFILE_UNRECOGNIZED}`, async () => {
154+
const relier = getRelier(SCOPE_PROFILE_UNRECOGNIZED);
155+
expect(await relier.getNormalizedScope()).toEqual(
156+
SCOPE_PROFILE_UNRECOGNIZED
157+
);
158+
});
159+
});
160+
});
161+
162+
describe('replaceItemInArray', () => {
163+
it('handles empty array', () => {
164+
expect(replaceItemInArray([], 'foo', ['bar'])).toEqual([]);
165+
});
166+
167+
it('handles miss', () => {
168+
expect(replaceItemInArray(['a', 'b', 'c'], '', ['foo'])).toEqual([
169+
'a',
170+
'b',
171+
'c',
172+
]);
173+
});
174+
175+
it('replaces and preserves order', () => {
176+
expect(replaceItemInArray(['a', 'b', 'c'], 'b', ['foo', 'bar'])).toEqual([
177+
'a',
178+
'foo',
179+
'bar',
180+
'c',
181+
]);
182+
});
183+
184+
it('handles duplicates', () => {
185+
expect(
186+
replaceItemInArray(['a', 'b', 'b', 'c', 'c'], 'b', ['foo', 'foo'])
187+
).toEqual(['a', 'foo', 'c']);
188+
});
189+
190+
it('handles empty replacement', () => {
191+
expect(replaceItemInArray(['a', 'b', 'c'], 'a', [])).toEqual(['b', 'c']);
192+
});
193+
});
194+
28195
// TODO: OAuth Relier Model Test Coverage
29196
});

0 commit comments

Comments
 (0)