Skip to content

AT Proof-Of-Possession #2: Handle proof-of-possession tokens correctly in responses #2153

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

Merged
merged 19 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/msal-browser/src/app/PublicClientApplication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
TrustedAuthority,
AuthorizationUrlRequest,
PersistentCacheKeys,
IdToken,
ProtocolUtils,
AuthorizationCodeRequest,
Constants,
Expand All @@ -31,7 +30,8 @@ import {
Logger,
ServerTelemetryManager,
ServerTelemetryRequest,
ServerAuthorizationCodeResponse
ServerAuthorizationCodeResponse,
AuthToken
} from "@azure/msal-common";
import { buildConfiguration, Configuration } from "../config/Configuration";
import { BrowserStorage } from "../cache/BrowserStorage";
Expand Down Expand Up @@ -681,7 +681,7 @@ export class PublicClientApplication implements IPublicClientApplication {
// Only check for adal token if no SSO params are being used
const adalIdTokenString = this.browserStorage.getItem(PersistentCacheKeys.ADAL_ID_TOKEN, CacheSchemaType.TEMPORARY) as string;
if (!StringUtils.isEmpty(adalIdTokenString)) {
const adalIdToken = new IdToken(adalIdTokenString, this.browserCrypto);
const adalIdToken = new AuthToken(adalIdTokenString, this.browserCrypto);
this.browserStorage.removeItem(PersistentCacheKeys.ADAL_ID_TOKEN);
if (adalIdToken.claims && adalIdToken.claims.upn) {
validatedRequest.loginHint = adalIdToken.claims.upn;
Expand Down
6 changes: 5 additions & 1 deletion lib/msal-browser/src/crypto/CryptoOps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/
import { ICrypto, PkceCodes } from "@azure/msal-common";
import { ICrypto, PkceCodes, SignedHttpRequest } from "@azure/msal-common";
import { GuidGenerator } from "./GuidGenerator";
import { Base64Encode } from "../encode/Base64Encode";
import { Base64Decode } from "../encode/Base64Decode";
Expand Down Expand Up @@ -74,4 +74,8 @@ export class CryptoOps implements ICrypto {
async generatePkceCodes(): Promise<PkceCodes> {
return this.pkceGenerator.generateCodes();
}

signJwt(payload: SignedHttpRequest, kid: string): Promise<string> {
throw new Error("Method not implemented.");
}
}
63 changes: 36 additions & 27 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lib/msal-browser/test/crypto/CryptoOps.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,5 @@ describe("CryptoOps.ts Unit Tests", () => {
expect(generateKeyPairSpy.calledWith(true, ["sign", "verify"]));
expect(exportJwkSpy.calledWith((await generateKeyPairSpy.returnValues[0]).publicKey));
expect(regExp.test(pkThumbprint)).to.be.true;
}).timeout(2500);
}).timeout(0);
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expect } from "chai";
import { InteractionHandler } from "../../src/interaction_handler/InteractionHandler";
import { PkceCodes, NetworkRequestOptions, LogLevel, AccountInfo, AuthorityFactory, AuthorizationCodeRequest, AuthenticationResult, CacheManager, AuthorizationCodeClient } from "@azure/msal-common";
import { PkceCodes, NetworkRequestOptions, LogLevel, AccountInfo, AuthorityFactory, AuthorizationCodeRequest, AuthenticationResult, CacheManager, AuthorizationCodeClient, AuthenticationScheme } from "@azure/msal-common";
import { Configuration, buildConfiguration } from "../../src/config/Configuration";
import { TEST_CONFIG, TEST_URIS, TEST_DATA_CLIENT_INFO, TEST_TOKENS, TEST_TOKEN_LIFETIMES, TEST_HASHES, TEST_POP_VALUES } from "../utils/StringConstants";
import { BrowserStorage } from "../../src/cache/BrowserStorage";
Expand Down Expand Up @@ -111,6 +111,9 @@ describe("InteractionHandler.ts Unit Tests", () => {
},
getPublicKeyThumbprint: async (): Promise<string> => {
return TEST_POP_VALUES.ENCODED_REQ_CNF;
},
signJwt: async (): Promise<string> => {
return "signedJwt";
}
},
storageInterface: new TestStorageInterface(),
Expand Down Expand Up @@ -187,7 +190,8 @@ describe("InteractionHandler.ts Unit Tests", () => {
idTokenClaims: idTokenClaims,
tenantId: idTokenClaims.tid,
uniqueId: idTokenClaims.oid,
state: "testState"
state: "testState",
tokenType: AuthenticationScheme.BEARER
};
sinon.stub(AuthorizationCodeClient.prototype, "handleFragmentResponse").returns(testCodeResponse);
const acquireTokenSpy = sinon.stub(AuthorizationCodeClient.prototype, "acquireToken").resolves(testTokenResponse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ describe("PopupHandler.ts Unit Tests", () => {
},
getPublicKeyThumbprint: async (): Promise<string> => {
return TEST_POP_VALUES.ENCODED_REQ_CNF;
},
signJwt: async (): Promise<string> => {
return "signedJwt";
}
},
storageInterface: new TestStorageInterface(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ chai.use(chaiAsPromised);
const expect = chai.expect;
import sinon from "sinon";
import { Configuration, buildConfiguration } from "../../src/config/Configuration";
import { PkceCodes, NetworkRequestOptions, LogLevel, AccountInfo, AuthorityFactory, AuthorizationCodeRequest, Constants, AuthenticationResult, CacheSchemaType, CacheManager, AuthorizationCodeClient } from "@azure/msal-common";
import { PkceCodes, NetworkRequestOptions, LogLevel, AccountInfo, AuthorityFactory, AuthorizationCodeRequest, Constants, AuthenticationResult, CacheSchemaType, CacheManager, AuthorizationCodeClient, AuthenticationScheme } from "@azure/msal-common";
import { TEST_CONFIG, TEST_URIS, TEST_TOKENS, TEST_DATA_CLIENT_INFO, RANDOM_TEST_GUID, TEST_HASHES, TEST_TOKEN_LIFETIMES, TEST_POP_VALUES } from "../utils/StringConstants";
import { BrowserStorage } from "../../src/cache/BrowserStorage";
import { RedirectHandler } from "../../src/interaction_handler/RedirectHandler";
Expand Down Expand Up @@ -75,6 +75,9 @@ describe("RedirectHandler.ts Unit Tests", () => {
},
getPublicKeyThumbprint: async (): Promise<string> => {
return TEST_POP_VALUES.ENCODED_REQ_CNF;
},
signJwt: async (): Promise<string> => {
return "signedJwt";
}
},
storageInterface: browserStorage,
Expand Down Expand Up @@ -211,7 +214,8 @@ describe("RedirectHandler.ts Unit Tests", () => {
expiresOn: new Date(Date.now() + (TEST_TOKEN_LIFETIMES.DEFAULT_EXPIRES_IN * 1000)),
idTokenClaims: idTokenClaims,
tenantId: idTokenClaims.tid,
uniqueId: idTokenClaims.oid
uniqueId: idTokenClaims.oid,
tokenType: AuthenticationScheme.BEARER
};
const browserCrypto = new CryptoOps();
const testAuthCodeRequest: AuthorizationCodeRequest = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ describe("SilentHandler.ts Unit Tests", () => {
},
getPublicKeyThumbprint: async (): Promise<string> => {
return TEST_POP_VALUES.ENCODED_REQ_CNF;
},
signJwt: async (): Promise<string> => {
return "signedJwt";
}
},
storageInterface: new TestStorageInterface(),
Expand Down
49 changes: 49 additions & 0 deletions lib/msal-common/src/account/AuthToken.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/
import { TokenClaims } from "./TokenClaims";
import { DecodedAuthToken } from "./DecodedAuthToken";
import { ClientAuthError } from "../error/ClientAuthError";
import { StringUtils } from "../utils/StringUtils";
import { ICrypto } from "../crypto/ICrypto";

/**
* JWT Token representation class. Parses token string and generates claims object.
*/
export class AuthToken {

// Raw Token string
rawToken: string;
// Claims inside token
claims: TokenClaims;
constructor(rawToken: string, crypto: ICrypto) {
if (StringUtils.isEmpty(rawToken)) {
throw ClientAuthError.createTokenNullOrEmptyError(rawToken);
}

this.rawToken = rawToken;
this.claims = AuthToken.extractTokenClaims(rawToken, crypto);
}

/**
* Extract token by decoding the rawToken
*
* @param encodedToken
*/
static extractTokenClaims(encodedToken: string, crypto: ICrypto): TokenClaims {
// token will be decoded to get the username
const decodedToken: DecodedAuthToken = StringUtils.decodeAuthToken(encodedToken);
if (!decodedToken) {
return null;
}
try {
const base64TokenPayload = decodedToken.JWSPayload;
// base64Decode() should throw an error if there is an issue
const base64Decoded = crypto.base64Decode(base64TokenPayload);
return JSON.parse(base64Decoded) as TokenClaims;
} catch (err) {
throw ClientAuthError.createTokenParsingError(err);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/**
* Interface for Decoded JWT tokens.
*/
export interface DecodedJwt {
export interface DecodedAuthToken {
header: string,
JWSPayload: string,
JWSSig: string
Expand Down
49 changes: 0 additions & 49 deletions lib/msal-common/src/account/IdToken.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/**
* Type which describes Id Token claims known by MSAL.
*/
export type IdTokenClaims = {
export type TokenClaims = {
iss?: string,
oid?: string,
sub?: string,
Expand All @@ -19,5 +19,8 @@ export type IdTokenClaims = {
exp?: number,
home_oid?: string,
sid?: string,
cloud_instance_host_name?: string
cloud_instance_host_name?: string,
cnf?: {
kid: string;
};
};
7 changes: 5 additions & 2 deletions lib/msal-common/src/cache/entities/AccessTokenEntity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
*/

import { CredentialEntity } from "./CredentialEntity";
import { CredentialType } from "../../utils/Constants";
import { CredentialType, AuthenticationScheme } from "../../utils/Constants";
import { TimeUtils } from "../../utils/TimeUtils";
import { StringUtils } from "../../utils/StringUtils";

/**
* ACCESS_TOKEN Credential Type
Expand Down Expand Up @@ -60,7 +61,8 @@ export class AccessTokenEntity extends CredentialEntity {
tenantId: string,
scopes: string,
expiresOn: number,
extExpiresOn: number
extExpiresOn: number,
tokenType?: string
): AccessTokenEntity {
const atEntity: AccessTokenEntity = new AccessTokenEntity();

Expand All @@ -81,6 +83,7 @@ export class AccessTokenEntity extends CredentialEntity {
atEntity.realm = tenantId;
atEntity.target = scopes;

atEntity.tokenType = StringUtils.isEmpty(tokenType) ? AuthenticationScheme.BEARER : tokenType;
return atEntity;
}
}
6 changes: 3 additions & 3 deletions lib/msal-common/src/cache/entities/AccountEntity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
CacheType,
} from "../../utils/Constants";
import { Authority } from "../../authority/Authority";
import { IdToken } from "../../account/IdToken";
import { AuthToken } from "../../account/AuthToken";
import { ICrypto } from "../../crypto/ICrypto";
import { buildClientInfo } from "../../account/ClientInfo";
import { StringUtils } from "../../utils/StringUtils";
Expand Down Expand Up @@ -125,7 +125,7 @@ export class AccountEntity {
static createAccount(
clientInfo: string,
authority: Authority,
idToken: IdToken,
idToken: AuthToken,
crypto: ICrypto
): AccountEntity {
const account: AccountEntity = new AccountEntity();
Expand Down Expand Up @@ -164,7 +164,7 @@ export class AccountEntity {
*/
static createADFSAccount(
authority: Authority,
idToken: IdToken
idToken: AuthToken
): AccountEntity {
const account: AccountEntity = new AccountEntity();

Expand Down
6 changes: 2 additions & 4 deletions lib/msal-common/src/client/AuthorizationCodeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ export class AuthorizationCodeClient extends BaseClient {

// Validate response. This function throws a server error if an error is returned by the server.
responseHandler.validateTokenResponse(response.body);
const tokenResponse = responseHandler.handleServerTokenResponse(response.body, this.authority, cachedNonce, cachedState);

return tokenResponse;
return await responseHandler.handleServerTokenResponse(response.body, this.authority, request.resourceRequestMethod, request.resourceRequestUri, cachedNonce, cachedState);
}

/**
Expand Down Expand Up @@ -176,7 +174,7 @@ export class AuthorizationCodeClient extends BaseClient {

if (request.authenticationScheme === AuthenticationScheme.POP) {
const popTokenGenerator = new PopTokenGenerator(this.cryptoUtils);
const cnfString = await popTokenGenerator.generateCnf();
const cnfString = await popTokenGenerator.generateCnf(request.resourceRequestMethod, request.resourceRequestUri);
parameterBuilder.addPopToken(cnfString);
}

Expand Down
8 changes: 4 additions & 4 deletions lib/msal-common/src/client/DeviceCodeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ export class DeviceCodeClient extends BaseClient {

// Validate response. This function throws a server error if an error is returned by the server.
responseHandler.validateTokenResponse(response);
const tokenResponse = responseHandler.handleServerTokenResponse(
return await responseHandler.handleServerTokenResponse(
response,
this.authority
this.authority,
request.resourceRequestMethod,
request.resourceRequestUri
);

return tokenResponse;
}

/**
Expand Down
4 changes: 1 addition & 3 deletions lib/msal-common/src/client/RefreshTokenClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,10 @@ export class RefreshTokenClient extends BaseClient {
);

responseHandler.validateTokenResponse(response.body);
const tokenResponse = responseHandler.handleServerTokenResponse(
return await responseHandler.handleServerTokenResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also pass in request.resourceRequestMethod and request.resourceRequestUri given that they were added to RefreshTokenRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've handled this in a later PR, this PR only covers the authorization code 2 legged flow, not the refresh flow.

response.body,
this.authority
);

return tokenResponse;
}

private async executeTokenRequest(request: RefreshTokenRequest, authority: Authority)
Expand Down
Loading