Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MSAL Angular MsalInterceptor bug matching to query string #7137

Merged
merged 9 commits into from
Jun 3, 2024
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": "[email protected]",
"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();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This unit test needed to be fixed as it was providing a correct match where the host name and port were different in the case of a protected resource that was a relative URL.

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
Loading