Merge 92ea914 into 75521b5

change/@azure-msal-browser-2020-09-10-16-36-03-default-scopes-silent-calls.json

@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Default scope addition done in msal-common (#2267)",
+  "packageName": "@azure/msal-browser",
+  "email": "thomas.norling@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-09-10T23:35:46.940Z"
+}

change/@azure-msal-common-2020-09-10-16-36-03-default-scopes-silent-calls.json

@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Add default scopes in all requests and ignore in cache lookups (#2267)",
+  "packageName": "@azure/msal-common",
+  "email": "thomas.norling@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-09-10T23:36:03.734Z"
+}

lib/msal-browser/src/app/ClientApplication.ts

@@ -640,7 +640,7 @@ export abstract class ClientApplication {
     protected setDefaultScopes(request: AuthorizationUrlRequest|RedirectRequest|PopupRequest|SsoSilentRequest): AuthorizationUrlRequest {
         return {
             ...request,
-            scopes: [...((request && request.scopes) || []), ...DEFAULT_REQUEST.scopes]
+            scopes: [...((request && request.scopes) || [])]
         };
     }
 
@@ -685,16 +685,14 @@ export abstract class ClientApplication {
 
         validatedRequest.responseMode = ResponseMode.FRAGMENT;
 
-        validatedRequest = {
-            ...validatedRequest,
-            ...this.initializeBaseRequest(validatedRequest)
+        validatedRequest = {	
+            ...validatedRequest,	
+            ...this.initializeBaseRequest(validatedRequest)	
         };
 
         this.browserStorage.updateCacheEntries(validatedRequest.state, validatedRequest.nonce, validatedRequest.authority);
 
-        return {
-            ...validatedRequest
-        };
+        return validatedRequest;
     }
 
     /**

lib/msal-browser/test/app/PublicClientApplication.spec.ts

@@ -566,7 +566,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 const browserCrypto = new CryptoOps();
                 await pca.loginRedirect(tokenRequest);
                 const cachedRequest: AuthorizationCodeRequest = JSON.parse(browserCrypto.base64Decode(browserStorage.getItem(browserStorage.generateCacheKey(TemporaryCacheKeys.REQUEST_PARAMS), CacheSchemaType.TEMPORARY) as string));
-                expect(cachedRequest.scopes).to.be.deep.eq(TEST_CONFIG.DEFAULT_SCOPES);
+                expect(cachedRequest.scopes).to.be.deep.eq([]);
                 expect(cachedRequest.codeVerifier).to.be.deep.eq(TEST_CONFIG.TEST_VERIFIER);
                 expect(cachedRequest.authority).to.be.deep.eq(`${Constants.DEFAULT_AUTHORITY}`);
                 expect(cachedRequest.correlationId).to.be.deep.eq(RANDOM_TEST_GUID);
@@ -637,7 +637,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 await pca.loginRedirect(emptyRequest);
                 const validatedRequest: AuthorizationUrlRequest = {
                     ...emptyRequest,
-                    scopes: TEST_CONFIG.DEFAULT_SCOPES,
+                    scopes: [],
                     loginHint: idTokenClaims.upn,
                     state: TEST_STATE_VALUES.TEST_STATE,
                     correlationId: RANDOM_TEST_GUID,
@@ -685,7 +685,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 await pca.loginRedirect(loginRequest);
                 const validatedRequest: AuthorizationUrlRequest = {
                     ...loginRequest,
-                    scopes: TEST_CONFIG.DEFAULT_SCOPES,
+                    scopes: [],
                     state: TEST_STATE_VALUES.TEST_STATE,
                     correlationId: RANDOM_TEST_GUID,
                     authority: `${Constants.DEFAULT_AUTHORITY}`,
@@ -771,7 +771,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 const browserCrypto = new CryptoOps();
                 await pca.acquireTokenRedirect(tokenRequest);
                 const cachedRequest: AuthorizationCodeRequest = JSON.parse(browserCrypto.base64Decode(browserStorage.getItem(browserStorage.generateCacheKey(TemporaryCacheKeys.REQUEST_PARAMS), CacheSchemaType.TEMPORARY) as string));
-                expect(cachedRequest.scopes).to.be.deep.eq([testScope, ...TEST_CONFIG.DEFAULT_SCOPES]);
+                expect(cachedRequest.scopes).to.be.deep.eq([testScope]);
                 expect(cachedRequest.codeVerifier).to.be.deep.eq(TEST_CONFIG.TEST_VERIFIER);
                 expect(cachedRequest.authority).to.be.deep.eq(`${Constants.DEFAULT_AUTHORITY}`);
                 expect(cachedRequest.correlationId).to.be.deep.eq(RANDOM_TEST_GUID);
@@ -843,7 +843,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 await pca.acquireTokenRedirect(emptyRequest);
                 const validatedRequest: AuthorizationUrlRequest = {
                     ...emptyRequest,
-                    scopes: [...emptyRequest.scopes, ...TEST_CONFIG.DEFAULT_SCOPES],
+                    scopes: [...emptyRequest.scopes],
                     loginHint: idTokenClaims.upn,
                     state: TEST_STATE_VALUES.TEST_STATE,
                     correlationId: RANDOM_TEST_GUID,
@@ -892,7 +892,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 await pca.acquireTokenRedirect(loginRequest);
                 const validatedRequest: AuthorizationUrlRequest = {
                     ...loginRequest,
-                    scopes: [...loginRequest.scopes, ...TEST_CONFIG.DEFAULT_SCOPES],
+                    scopes: [...loginRequest.scopes],
                     state: TEST_STATE_VALUES.TEST_STATE,
                     correlationId: RANDOM_TEST_GUID,
                     authority: `${Constants.DEFAULT_AUTHORITY}`,
@@ -1362,6 +1362,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
             };
             const expectedTokenRequest: SilentFlowRequest = {
                 ...tokenRequest,
+                scopes: ["scope1"],
                 authority: `${Constants.DEFAULT_AUTHORITY}`,
                 correlationId: RANDOM_TEST_GUID
             };
@@ -1455,7 +1456,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
             };
             const expectedRequest: AuthorizationUrlRequest = {
                 ...silentFlowRequest,
-                scopes: ["User.Read", ...TEST_CONFIG.DEFAULT_SCOPES],
+                scopes: ["User.Read"],
                 authority: `${Constants.DEFAULT_AUTHORITY}`,
                 correlationId: RANDOM_TEST_GUID,
                 prompt: "none",

lib/msal-common/src/cache/CacheManager.ts

@@ -575,9 +575,9 @@ export abstract class CacheManager implements ICacheManager {
         const entityScopeSet: ScopeSet = ScopeSet.fromString(entity.target);
         const requestTargetScopeSet: ScopeSet = ScopeSet.fromString(target);
 
-        // ignore offline_access when comparing scopes
-        entityScopeSet.removeScope(Constants.OFFLINE_ACCESS_SCOPE);
-        requestTargetScopeSet.removeScope(Constants.OFFLINE_ACCESS_SCOPE);
+        if (!requestTargetScopeSet.containsOnlyDefaultScopes()) {
+            requestTargetScopeSet.removeDefaultScopes(); // ignore default scopes
+        }
         return entityScopeSet.containsScopeSet(requestTargetScopeSet);
     }
 

lib/msal-common/src/client/AuthorizationCodeClient.ts

@@ -8,7 +8,7 @@ import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest";
 import { AuthorizationCodeRequest } from "../request/AuthorizationCodeRequest";
 import { Authority } from "../authority/Authority";
 import { RequestParameterBuilder } from "../request/RequestParameterBuilder";
-import { GrantType, AADServerParamKeys } from "../utils/Constants";
+import { GrantType, AADServerParamKeys, Constants } from "../utils/Constants";
 import { ClientConfiguration } from "../config/ClientConfiguration";
 import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse";
 import { NetworkResponse } from "../network/NetworkManager";
@@ -155,8 +155,8 @@ export class AuthorizationCodeClient extends BaseClient {
         // validate the redirectUri (to be a non null value)
         parameterBuilder.addRedirectUri(request.redirectUri);
 
-        const scopeSet = new ScopeSet(request.scopes || []);
-        parameterBuilder.addScopes(scopeSet);
+        // Add scope array, parameter builder will add default scopes and dedupe
+        parameterBuilder.addScopes(request.scopes);
 
         // add code: user set, not validated
         parameterBuilder.addAuthorizationCode(request.code);
@@ -198,11 +198,8 @@ export class AuthorizationCodeClient extends BaseClient {
 
         parameterBuilder.addClientId(this.config.authOptions.clientId);
 
-        const scopeSet = new ScopeSet(request.scopes || []);
-        if (request.extraScopesToConsent) {
-            scopeSet.appendScopes(request.extraScopesToConsent);
-        }
-        parameterBuilder.addScopes(scopeSet);
+        const requestScopes = [...request.scopes || [], ...request.extraScopesToConsent || []];
+        parameterBuilder.addScopes(requestScopes);
 
         // validate the redirectUri (to be a non null value)
         parameterBuilder.addRedirectUri(request.redirectUri);

lib/msal-common/src/client/ClientCredentialClient.ts

@@ -117,7 +117,7 @@ export class ClientCredentialClient extends BaseClient {
 
         parameterBuilder.addClientId(this.config.authOptions.clientId);
 
-        parameterBuilder.addScopes(this.scopeSet);
+        parameterBuilder.addScopes(request.scopes, false);
 
         parameterBuilder.addGrantType(GrantType.CLIENT_CREDENTIALS_GRANT);
 

lib/msal-common/src/client/DeviceCodeClient.ts

@@ -119,8 +119,7 @@ export class DeviceCodeClient extends BaseClient {
 
         const parameterBuilder: RequestParameterBuilder = new RequestParameterBuilder();
 
-        const scopeSet = new ScopeSet(request.scopes || []);
-        parameterBuilder.addScopes(scopeSet);
+        parameterBuilder.addScopes(request.scopes);
         parameterBuilder.addClientId(this.config.authOptions.clientId);
 
         if (!StringUtils.isEmpty(request.claims) || this.config.authOptions.clientCapabilities && this.config.authOptions.clientCapabilities.length > 0) {
@@ -202,8 +201,7 @@ export class DeviceCodeClient extends BaseClient {
 
         const requestParameters: RequestParameterBuilder = new RequestParameterBuilder();
 
-        const scopeSet = new ScopeSet(request.scopes || []);
-        requestParameters.addScopes(scopeSet);
+        requestParameters.addScopes(request.scopes);
         requestParameters.addClientId(this.config.authOptions.clientId);
         requestParameters.addGrantType(GrantType.DEVICE_CODE_GRANT);
         requestParameters.addDeviceCode(deviceCodeResponse.deviceCode);

lib/msal-common/src/client/OnBehalfOfClient.ts

@@ -156,7 +156,7 @@ export class OnBehalfOfClient extends BaseClient {
 
         parameterBuilder.addClientId(this.config.authOptions.clientId);
 
-        parameterBuilder.addScopes(this.scopeSet);
+        parameterBuilder.addScopes(request.scopes);
 
         parameterBuilder.addGrantType(GrantType.JWT_BEARER);
 

lib/msal-common/src/client/RefreshTokenClient.ts

@@ -96,8 +96,7 @@ export class RefreshTokenClient extends BaseClient {
 
         parameterBuilder.addClientId(this.config.authOptions.clientId);
 
-        const scopeSet = new ScopeSet(request.scopes || []);
-        parameterBuilder.addScopes(scopeSet);
+        parameterBuilder.addScopes(request.scopes);
 
         parameterBuilder.addGrantType(GrantType.REFRESH_TOKEN_GRANT);
 

lib/msal-common/src/request/RequestParameterBuilder.ts

@@ -40,10 +40,13 @@ export class RequestParameterBuilder {
     }
 
     /**
-     * add scopes
+     * add scopes. set addOidcScopes to false to prevent default scopes in non-user scenarios
      * @param scopeSet
+     * @param addOidcScopes
      */
-    addScopes(scopeSet: ScopeSet): void {
+    addScopes(scopes: string[], addOidcScopes: boolean = true): void {
+        const requestScopes = addOidcScopes ? [...scopes || [], Constants.OPENID_SCOPE, Constants.PROFILE_SCOPE] : scopes || [];
+        const scopeSet = new ScopeSet(requestScopes);
         this.parameters.set(AADServerParamKeys.SCOPE, encodeURIComponent(scopeSet.printScopes()));
     }
 

lib/msal-common/src/request/ScopeSet.ts

@@ -6,6 +6,7 @@
 import { ClientConfigurationError } from "../error/ClientConfigurationError";
 import { StringUtils } from "../utils/StringUtils";
 import { ClientAuthError } from "../error/ClientAuthError";
+import { Constants } from "../utils/Constants";
 
 /**
  * The ScopeSet class creates a set of scopes. Scopes are case-insensitive, unique values, so the Set object in JS makes
@@ -72,6 +73,24 @@ export class ScopeSet {
         return (this.scopes.size >= scopeSet.scopes.size && scopeSet.asArray().every(scope => this.containsScope(scope)));
     }
 
+    /**
+     * Check if set of scopes contains only the defaults
+     */
+    containsOnlyDefaultScopes(): boolean {
+        let defaultScopeCount = 0;
+        if (this.containsScope(Constants.OPENID_SCOPE)) {
+            defaultScopeCount += 1;
+        } 
+        if (this.containsScope(Constants.PROFILE_SCOPE)) {
+            defaultScopeCount += 1;
+        }
+        if (this.containsScope(Constants.OFFLINE_ACCESS_SCOPE)) {
+            defaultScopeCount += 1;
+        }
+
+        return this.scopes.size === defaultScopeCount;
+    }
+
     /**
      * Appends single scope if passed
      * @param newScope
@@ -105,6 +124,16 @@ export class ScopeSet {
         this.scopes.delete(scope.trim());
     }
 
+    /**
+     * Removes default scopes from set of scopes
+     * Primarily used to prevent cache misses if the default scopes are not returned from the server
+     */
+    removeDefaultScopes(): void {
+        this.scopes.delete(Constants.OFFLINE_ACCESS_SCOPE);
+        this.scopes.delete(Constants.OPENID_SCOPE);
+        this.scopes.delete(Constants.PROFILE_SCOPE);
+    }
+
     /**
      * Combines an array of scopes with the current set of scopes.
      * @param otherScopes
@@ -130,7 +159,10 @@ export class ScopeSet {
 
         const unionScopes = this.unionScopeSets(otherScopes);
 
-        // Do not allow offline_access to be the only intersecting scope
+        // Do not allow default scopes to be the only intersecting scopes
+        if (!otherScopes.containsOnlyDefaultScopes()) {
+            otherScopes.removeDefaultScopes();
+        }
         const sizeOtherScopes = otherScopes.getScopeCount();
         const sizeThisScopes = this.getScopeCount();
         const sizeUnionScopes = unionScopes.size;

lib/msal-common/test/cache/CacheManager.spec.ts

@@ -171,7 +171,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("save account", () => {
-        let ac = new AccountEntity();
+        const ac = new AccountEntity();
         ac.homeAccountId = "someUid.someUtid";
         ac.environment = "login.microsoftonline.com";
         ac.realm = "microsoft";
@@ -188,7 +188,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("save credential", () => {
-        let at = new AccessTokenEntity();
+        const at = new AccessTokenEntity();
         Object.assign(at, {
             homeAccountId: "someUid.someUtid",
             environment: "login.microsoftonline.com",
@@ -211,7 +211,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("getAccount", () => {
-        let ac = new AccountEntity();
+        const ac = new AccountEntity();
         ac.homeAccountId = "someUid.someUtid";
         ac.environment = "login.microsoftonline.com";
         ac.realm = "microsoft";
@@ -229,7 +229,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("getCredential", () => {
-        let accessTokenEntity = new AccessTokenEntity();
+        const accessTokenEntity = new AccessTokenEntity();
         accessTokenEntity.homeAccountId = "someUid.someUtid";
         accessTokenEntity.environment = "login.microsoftonline.com";
         accessTokenEntity.realm = "microsoft";
@@ -375,8 +375,8 @@ describe("CacheManager.ts test cases", () => {
             expect(Object.keys(credentials.accessTokens).length).to.eql(0);
             expect(Object.keys(credentials.refreshTokens).length).to.eql(0);
 
-            const filterOidcscopes = { target: "scope1 scope2 scope3 offline_access" };
-            let filteredCredentials = cacheManager.getCredentialsFilteredBy(filterOidcscopes);
+            const filterOidcscopes = { target: "scope1 scope2 scope3 offline_access openid profile" };
+            const filteredCredentials = cacheManager.getCredentialsFilteredBy(filterOidcscopes);
             expect(Object.keys(filteredCredentials.idTokens).length).to.eql(0);
             expect(Object.keys(filteredCredentials.accessTokens).length).to.eql(1);
             expect(Object.keys(filteredCredentials.refreshTokens).length).to.eql(0);
@@ -391,7 +391,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("removeAllAccounts", () => {
-        let ac = new AccountEntity();
+        const ac = new AccountEntity();
         ac.homeAccountId = "someUid.someUtid";
         ac.environment = "login.microsoftonline.com";
         ac.realm = "microsoft";
@@ -417,7 +417,7 @@ describe("CacheManager.ts test cases", () => {
     });
 
     it("removeCredential", () => {
-        let at = new AccessTokenEntity();
+        const at = new AccessTokenEntity();
         Object.assign(at, {
             homeAccountId: "someUid.someUtid",
             environment: "login.microsoftonline.com",
@@ -451,7 +451,7 @@ describe("CacheManager.ts test cases", () => {
             },
             refreshTokens: null,
             idTokens: null
-        }
+        };
 
         sinon.stub(CacheManager.prototype, <any>"getCredentialsFilteredBy").returns(mockedCredentialCache);