Merge pull request #1961 from AzureAD/lowercase-scopes-fix-2.0

Lowercase scopes fix 2.0

.github/workflows/codeql.yml

@@ -1,7 +1,7 @@
 name: "Code Scanning - Action"
 
 on:
-  push:
+  # push:
   schedule:
     - cron: '0 0 * * 0'
 

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

@@ -112,7 +112,7 @@ export class SilentFlowClient extends BaseClient {
             credentialType: CredentialType.ACCESS_TOKEN,
             clientId: this.config.authOptions.clientId,
             realm: inputRealm,
-            target: scopes.printScopes()
+            target: scopes.printScopesLowerCase()
         };
         const credentialCache: CredentialCache = this.cacheManager.getCredentialsFilteredBy(accessTokenFilter);
         const accessTokens = Object.keys(credentialCache.accessTokens).map(key => credentialCache.accessTokens[key]);

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

@@ -9,15 +9,16 @@ import { ClientAuthError } from "../error/ClientAuthError";
 
 /**
  * The ScopeSet class creates a set of scopes. Scopes are case-insensitive, unique values, so the Set object in JS makes
- * the most sense to implement for this class. All scopes are trimmed and converted to lower case strings to ensure uniqueness of strings.
+ * the most sense to implement for this class. All scopes are trimmed and converted to lower case strings in intersection and union functions 
+ * to ensure uniqueness of strings.
  */
-export class ScopeSet {;
+export class ScopeSet {
     // Scopes as a Set of strings
     private scopes: Set<string>;
 
     constructor(inputScopes: Array<string>) {
         // Filter empty string and null/undefined array items
-        const scopeArr = inputScopes ? StringUtils.trimAndConvertArrayEntriesToLowerCase([...inputScopes]) : [];
+        const scopeArr = inputScopes ? StringUtils.trimArrayEntries([...inputScopes]) : [];
         const filteredInput = scopeArr ? StringUtils.removeEmptyStringsFromArray(scopeArr) : [];
 
         // Validate and filter scopes (validate function throws if validation fails)
@@ -77,7 +78,7 @@ export class ScopeSet {;
      */
     appendScope(newScope: string): void {
         if (!StringUtils.isEmpty(newScope)) {
-            this.scopes.add(newScope.trim().toLowerCase());
+            this.scopes.add(newScope.trim());
         }
     }
 
@@ -101,7 +102,7 @@ export class ScopeSet {;
         if (StringUtils.isEmpty(scope)) {
             throw ClientAuthError.createRemoveEmptyScopeFromSetError(scope);
         }
-        this.scopes.delete(scope.trim().toLowerCase());
+        this.scopes.delete(scope.trim());
     }
 
     /**
@@ -162,4 +163,11 @@ export class ScopeSet {;
         }
         return "";
     }
+
+    /**
+     * Prints scopes into a space-delimited lower-case string (used for caching)
+     */
+    printScopesLowerCase(): string {
+        return this.printScopes().toLowerCase();
+    }
 }

lib/msal-common/src/response/ResponseHandler.ts

@@ -173,7 +173,7 @@ export class ResponseHandler {
                 serverTokenResponse.access_token,
                 this.clientId,
                 idTokenObj.claims.tid,
-                responseScopes.printScopes(),
+                responseScopes.printScopesLowerCase(),
                 tokenExpirationSeconds,
                 extendedTokenExpirationSeconds
             );

lib/msal-common/src/utils/StringUtils.ts

@@ -69,12 +69,12 @@ export class StringUtils {
     }
 
     /**
-     * Trims entries and converts them to lower case.
+     * Trims entries in an array.
      *
      * @param arr
      */
-    static trimAndConvertArrayEntriesToLowerCase(arr: Array<string>): Array<string> {
-        return arr.map(entry => entry.trim().toLowerCase());
+    static trimArrayEntries(arr: Array<string>): Array<string> {
+        return arr.map(entry => entry.trim());
     }
 
     /**

lib/msal-common/test/request/ScopeSet.spec.ts

@@ -16,26 +16,25 @@ describe("ScopeSet.ts", () => {
             expect(() => new ScopeSet([])).to.throw(ClientConfigurationError);
         });
 
-        it("Converts array string values to lower case", () => {
-            const testScope1 = "TestScope1";
-            const lowerCaseScope1 = "testscope1";
-            const testScope2 = "TeStScOpE2";
-            const lowerCaseScope2 = "testscope2";
-            const testScope3 = "TESTSCOPE3";
-            const lowerCaseScope3 = "testscope3";
+        it("Trims array string values", () => {
+            const testScope1 = "    TestScope1";
+            const trimmedTestScope1 = "TestScope1";
+            const testScope2 = "TeStScOpE2   ";
+            const trimmedTestScope2 = "TeStScOpE2";
+            const testScope3 = "   TESTSCOPE3   ";
+            const trimmedTestScope3 = "TESTSCOPE3";
             const scopeSet = new ScopeSet([testScope1, testScope2, testScope3]);
-            expect(scopeSet.asArray()).to.deep.eq([lowerCaseScope1, lowerCaseScope2, lowerCaseScope3]);
+            expect(scopeSet.asArray()).to.deep.eq([trimmedTestScope1, trimmedTestScope2, trimmedTestScope3]);
         });
 
-        it("Removes duplicates from input scope array", () => {
+        it("Removes case-sensitive duplicates from input scope array", () => {
             const testScope1 = "TestScope";
             const testScope2 = "TeStScOpE";
             const testScope3 = "TESTSCOPE";
             const testScope4 = "testscope";
             const testScope5 = "testscope";
-            const lowerCaseScope = "testscope";
             const scopeSet = new ScopeSet([testScope1, testScope2, testScope3, testScope4, testScope5]);
-            expect(scopeSet.asArray()).to.deep.eq([lowerCaseScope]);
+            expect(scopeSet.asArray()).to.deep.eq([testScope1, testScope2, testScope3, testScope4]);
         });
     });
 
@@ -52,26 +51,25 @@ describe("ScopeSet.ts", () => {
             expect(() => ScopeSet.fromString(undefined)).to.throw(ClientConfigurationError);
         });
 
-        it("Trims and converts array string values to lower case", () => {
+        it("Trims array string values", () => {
             const testScope1 = "   TestScope1";
-            const lowerCaseScope1 = "testscope1";
+            const trimmedTestScope1 = "TestScope1";
             const testScope2 = "TeStScOpE2   ";
-            const lowerCaseScope2 = "testscope2";
+            const trimmedTestScope2 = "TeStScOpE2";
             const testScope3 = "   TESTSCOPE3  ";
-            const lowerCaseScope3 = "testscope3";
+            const trimmedTestScope3 = "TESTSCOPE3";
             const scopeSet = ScopeSet.fromString(`${testScope1} ${testScope2} ${testScope3}`);
-            expect(scopeSet.asArray()).to.deep.eq([lowerCaseScope1, lowerCaseScope2, lowerCaseScope3]);
+            expect(scopeSet.asArray()).to.deep.eq([trimmedTestScope1, trimmedTestScope2, trimmedTestScope3]);
         });
 
-        it("Removes duplicates from input scope array", () => {
+        it("Removes case-sensitive duplicates from input scope array", () => {
             const testScope1 = "TestScope";
-            const testScope2 = "TeStScOpE  ";
-            const testScope3 = "  TESTSCOPE ";
+            const testScope2 = "TeStScOpE";
+            const testScope3 = "TESTSCOPE";
             const testScope4 = "testscope";
             const testScope5 = "testscope";
-            const lowerCaseScope = "testscope";
             const scopeSet = ScopeSet.fromString(`${testScope1} ${testScope2} ${testScope3} ${testScope4} ${testScope5}`);
-            expect(scopeSet.asArray()).to.deep.eq([lowerCaseScope]);
+            expect(scopeSet.asArray()).to.deep.eq([testScope1, testScope2, testScope3, testScope4]);
         });
     });
 
@@ -113,10 +111,10 @@ describe("ScopeSet.ts", () => {
             expect(scopes.containsScopeSet(undefined)).to.be.false;
         });
 
-        it("appendScope() adds scope to set after trimming and converting to lower case", () => {
+        it("appendScope() adds scope to set after trimming", () => {
             expect(scopes.asArray()).to.contain(testScope);
             scopes.appendScope("   testScope2   ");
-            expect(scopes.asArray()).to.contain("testscope2");
+            expect(scopes.asArray()).to.contain("testScope2");
         });
 
         it("appendScope() does not add duplicates to ScopeSet", () => {
@@ -164,9 +162,9 @@ describe("ScopeSet.ts", () => {
             expect(scopes.asArray()).to.contain(testScope3);
         });
 
-        it("appendScopes() trims and converts scopes to lower case before adding", () => {
-            const testScope2 = "TestScope2";
-            const expectedTestScope2 = "testscope2";
+        it("appendScopes() trims scopes before adding", () => {
+            const testScope2 = "   TestScope2";
+            const expectedTestScope2 = "TestScope2";
             const testScope3 = "     testscope3   ";
             const expectedTestScope3 = "testscope3";
             scopes.appendScopes([testScope2, testScope3]);
@@ -261,11 +259,17 @@ describe("ScopeSet.ts", () => {
 
         let requiredScopeSet: ScopeSet;
         let nonRequiredScopeSet: ScopeSet;
+        let uppercaseScopeSet: ScopeSet;
+        let lowercaseScopeSet: ScopeSet;
         let testScope: string;
+        let testScope2: string;
         beforeEach(() => {
             testScope = "testscope";
+            testScope2 = "testScope";
             requiredScopeSet = new ScopeSet([testScope]);
             nonRequiredScopeSet = new ScopeSet([testScope]);
+            uppercaseScopeSet = new ScopeSet([testScope2]);
+            lowercaseScopeSet = new ScopeSet([testScope]);
         });
 
         afterEach(() => {
@@ -291,5 +295,10 @@ describe("ScopeSet.ts", () => {
             requiredScopeSet.removeScope(Constants.OFFLINE_ACCESS_SCOPE);
             expect(requiredScopeSet.printScopes()).to.be.eq("");
         });
+
+        it("printScopesLowerCase() prints space-delimited string of scopes in lowercase", () => {
+            const scopeArr = lowercaseScopeSet.asArray();
+            expect(uppercaseScopeSet.printScopesLowerCase()).to.be.eq(scopeArr.join(" "));
+        });
     });
 });

lib/msal-common/test/utils/StringUtils.spec.ts

@@ -3,6 +3,7 @@ import { StringUtils } from "../../src/utils/StringUtils";
 import { TEST_TOKENS } from "./StringConstants";
 import { ClientAuthError, ClientAuthErrorMessage } from "../../src/error/ClientAuthError";
 import { AuthError } from "../../src/error/AuthError";
+import { IdToken } from "../../src";
 
 describe("StringUtils.ts Class Unit Tests", () => {
 
@@ -122,8 +123,9 @@ describe("StringUtils.ts Class Unit Tests", () => {
         expect(StringUtils.queryStringToObject(serializedObj)).to.be.deep.eq(deserializedObj);        
     });
 
-    it("trimAndConvertArrayEntriesToLowerCase() converts entries to lower case and trims them", () => {
-
+    it("trimArrayEntries() correctly trims entries in an array", () => {
+        const arr = ["S1", " S2  ", " S3 "];
+        expect(StringUtils.trimArrayEntries(arr)).to.be.deep.eq(["S1", "S2", "S3"]);   
     });
 
     it("removeEmptyStringsFromArray() removes empty strings from an array", () => {