Skip to content

Commit

Permalink
Fix MSAL Angular MsalInterceptor bug matching to query string (#7137)
Browse files Browse the repository at this point in the history
This PR addresses a bug where the `MsalInterceptor` could match the
protectedResource to the query string part of the URL instead of the
host name and port part of the URL. It also refactors the code
surrounding relative URLs.

This addresses issue #7111
  • Loading branch information
jo-arroyo authored Jun 3, 2024
1 parent 69e58c3 commit 8e4b664
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 47 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fix query string instead of HostNameAndPort bug in MsalInterceptor #7137",
"packageName": "@azure/msal-angular",
"email": "joarroyo@microsoft.com",
"dependentChangeType": "patch"
}
5 changes: 0 additions & 5 deletions lib/msal-angular/src/msal.interceptor.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,3 @@ export type ProtectedResourceScopes = {
httpMethod: string;
scopes: Array<string> | null;
};

export type MatchingResources = {
absoluteResources: Array<string>;
relativeResources: Array<string>;
};
46 changes: 44 additions & 2 deletions lib/msal-angular/src/msal.interceptor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ function MSALInterceptorFactory(): MsalInterceptorConfiguration {
string,
Array<string | ProtectedResourceScopes> | null
>([
["https://MY_API_SITE_2", ["api://MY_API_SITE_2/as_user"]],
["https://MY_API_SITE_1", ["api://MY_API_SITE_1/as_user"]],
["https://graph.microsoft.com/v1.0/me", ["user.read"]],
["relative/me", ["relative.scope"]],
["https://myapplication.com/user/*", ["customscope.read"]],
Expand Down Expand Up @@ -906,9 +908,9 @@ describe("MsalInterceptor", () => {
sampleAccountInfo,
]);

httpClient.get("http://site.com/relative/me").subscribe();
httpClient.get("http://localhost:9876/relative/me").subscribe();
setTimeout(() => {
const request = httpMock.expectOne("http://site.com/relative/me");
const request = httpMock.expectOne("http://localhost:9876/relative/me");
request.flush({ data: "test" });
expect(request.request.headers.get("Authorization")).toEqual(
"Bearer access-token"
Expand Down Expand Up @@ -1122,4 +1124,44 @@ describe("MsalInterceptor", () => {
done();
}, 200);
});

it("attaches authorization header with access token when endpoint match is in HostNameAndPort instead of query string", (done) => {
const spy = spyOn(
PublicClientApplication.prototype,
"acquireTokenSilent"
).and.returnValue(
new Promise((resolve) => {
//@ts-ignore
resolve({
accessToken: "access-token",
});
})
);

spyOn(PublicClientApplication.prototype, "getAllAccounts").and.returnValue([
sampleAccountInfo,
]);

httpClient
.get(
"https://MY_API_SITE_1/api/sites?$filter=siteUrl eq 'https://MY_API_SITE_2'"
)
.subscribe();

setTimeout(() => {
const request = httpMock.expectOne(
"https://MY_API_SITE_1/api/sites?$filter=siteUrl eq 'https://MY_API_SITE_2'"
);
request.flush({ data: "test" });
expect(request.request.headers.get("Authorization")).toEqual(
"Bearer access-token"
);
expect(spy).toHaveBeenCalledWith({
account: sampleAccountInfo,
scopes: ["api://MY_API_SITE_1/as_user"],
});
httpMock.verify();
done();
}, 200);
});
});
78 changes: 38 additions & 40 deletions lib/msal-angular/src/msal.interceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
InteractionStatus,
InteractionType,
StringUtils,
UrlString,
} from "@azure/msal-browser";
import { Observable, EMPTY, of } from "rxjs";
import { switchMap, catchError, take, filter } from "rxjs/operators";
Expand All @@ -27,7 +26,6 @@ import {
MsalInterceptorAuthRequest,
MsalInterceptorConfiguration,
ProtectedResourceScopes,
MatchingResources,
} from "./msal.interceptor.config";
import { MsalBroadcastService } from "./msal.broadcast.service";
import { MSAL_INTERCEPTOR_CONFIG } from "./constants";
Expand Down Expand Up @@ -239,17 +237,10 @@ export class MsalInterceptor implements HttpInterceptor {
normalizedEndpoint
);

// Check absolute urls of resources first before checking relative to prevent incorrect matching where multiple resources have similar relative urls
if (matchingProtectedResources.absoluteResources.length > 0) {
if (matchingProtectedResources.length > 0) {
return this.matchScopesToEndpoint(
this.msalInterceptorConfig.protectedResourceMap,
matchingProtectedResources.absoluteResources,
httpMethod
);
} else if (matchingProtectedResources.relativeResources.length > 0) {
return this.matchScopesToEndpoint(
this.msalInterceptorConfig.protectedResourceMap,
matchingProtectedResources.relativeResources,
matchingProtectedResources,
httpMethod
);
}
Expand All @@ -266,46 +257,53 @@ export class MsalInterceptor implements HttpInterceptor {
private matchResourcesToEndpoint(
protectedResourcesEndpoints: string[],
endpoint: string
): MatchingResources {
const matchingResources: MatchingResources = {
absoluteResources: [],
relativeResources: [],
};
): Array<string> {
const matchingResources: Array<string> = [];

protectedResourcesEndpoints.forEach((key) => {
// Normalizes and adds resource to matchingResources.absoluteResources if key matches endpoint. StringUtils.matchPattern accounts for wildcards
const normalizedKey = this.location.normalize(key);
if (StringUtils.matchPattern(normalizedKey, endpoint)) {
matchingResources.absoluteResources.push(key);
}

// Get url components for relative urls
const absoluteKey = this.getAbsoluteUrl(key);
const keyComponents = new UrlString(absoluteKey).getUrlComponents();
// Get url components
const absoluteKey = this.getAbsoluteUrl(normalizedKey);
const keyComponents = new URL(absoluteKey);
const absoluteEndpoint = this.getAbsoluteUrl(endpoint);
const endpointComponents = new UrlString(
absoluteEndpoint
).getUrlComponents();

// Normalized key should include query strings if applicable
const relativeNormalizedKey = keyComponents.QueryString
? `${keyComponents.AbsolutePath}?${keyComponents.QueryString}`
: this.location.normalize(keyComponents.AbsolutePath);

// Add resource to matchingResources.relativeResources if same origin, relativeKey matches endpoint, and is not empty
if (
keyComponents.HostNameAndPort === endpointComponents.HostNameAndPort &&
StringUtils.matchPattern(relativeNormalizedKey, absoluteEndpoint) &&
relativeNormalizedKey !== "" &&
relativeNormalizedKey !== "/*"
) {
matchingResources.relativeResources.push(key);
const endpointComponents = new URL(absoluteEndpoint);

if (this.checkUrlComponents(keyComponents, endpointComponents)) {
matchingResources.push(key);
}
});

return matchingResources;
}

/**
* Compares URL segments between key and endpoint
* @param key
* @param endpoint
* @returns
*/
private checkUrlComponents(
keyComponents: URL,
endpointComponents: URL
): boolean {
// URL properties from https://developer.mozilla.org/en-US/docs/Web/API/URL
const urlProperties = ["protocol", "host", "pathname", "search", "hash"];

for (const property of urlProperties) {
if (keyComponents[property]) {
const decodedInput = decodeURIComponent(keyComponents[property]);
if (
!StringUtils.matchPattern(decodedInput, endpointComponents[property])
) {
return false;
}
}
}

return true;
}

/**
* Transforms relative urls to absolute urls
* @param url
Expand Down

0 comments on commit 8e4b664

Please sign in to comment.