Skip to content

Commit

Permalink
- Address review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
konstantin-msft committed Oct 2, 2024
1 parent feb1025 commit 7424edf
Show file tree
Hide file tree
Showing 14 changed files with 296 additions and 166 deletions.
11 changes: 6 additions & 5 deletions lib/msal-browser/src/naa/mapping/NestedAppAuthAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,11 @@ export class NestedAppAuthAdapter {
);
}

const requestBuilder = new RequestParameterBuilder();
const correlationId =
request.correlationId !== undefined
? request.correlationId
: this.crypto.createNewGuid();
const requestBuilder = new RequestParameterBuilder(correlationId);
const claims = requestBuilder.addClientCapabilitiesToClaims(
request.claims,
this.clientCapabilities
Expand All @@ -83,10 +87,7 @@ export class NestedAppAuthAdapter {
clientId: this.clientId,
authority: request.authority,
scope: scopes.join(" "),
correlationId:
request.correlationId !== undefined
? request.correlationId
: this.crypto.createNewGuid(),
correlationId,
claims: !StringUtils.isEmptyObj(claims) ? claims : undefined,
state: request.state,
authenticationScheme:
Expand Down
19 changes: 11 additions & 8 deletions lib/msal-common/apiReview/msal-common.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ export type AuthOptions = {
azureCloudOptions?: AzureCloudOptions;
skipAuthorityMetadataCache?: boolean;
instanceAware?: boolean;
redirectUri?: string;
};

// Warning: (ae-internal-missing-underscore) The name "Authority" should be prefixed with an underscore because the declaration is marked as @internal
Expand Down Expand Up @@ -605,7 +606,7 @@ export type BaseAuthRequest = {
storeInCache?: StoreInCache;
scenarioId?: string;
popKid?: string;
brokerParameters?: BrokerParameters;
embeddedClientId?: string;
};

// Warning: (ae-internal-missing-underscore) The name "BaseClient" should be prefixed with an underscore because the declaration is marked as @internal
Expand Down Expand Up @@ -1587,7 +1588,6 @@ export type CommonEndSessionRequest = {
state?: string;
logoutHint?: string;
extraQueryParameters?: StringDict;
brokerParameters?: BrokerParameters;
};

// Warning: (ae-missing-release-tag) "CommonOnBehalfOfRequest" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down Expand Up @@ -3063,6 +3063,7 @@ export type PerformanceEvent = {
accountType?: "AAD" | "MSA" | "B2C";
retryError?: string;
embeddedClientId?: string;
embeddedRedirectUri?: string;
};

// Warning: (tsdoc-undefined-tag) The TSDoc tag "@export" is not defined in this configuration
Expand Down Expand Up @@ -3371,13 +3372,16 @@ const REQUESTED_TOKEN_USE = "requested_token_use";
//
// @internal (undocumented)
export class RequestParameterBuilder {
constructor(performanceClient?: IPerformanceClient);
constructor(correlationId: string, performanceClient?: IPerformanceClient);
// Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
addApplicationTelemetry(appTelemetry: ApplicationTelemetry): void;
// Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
addAuthorizationCode(code: string): void;
// (undocumented)
addBrokerParameters(params: BrokerParameters): void;
addBrokerParameters(params: {
brokerClientId: string;
brokerRedirectUri: string;
}): void;
// Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
addCcsOid(clientInfo: ClientInfo): void;
// Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
Expand Down Expand Up @@ -4283,9 +4287,9 @@ const X_MS_LIB_CAPABILITY = "x-ms-lib-capability";
// src/client/AuthorizationCodeClient.ts:228:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:229:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:307:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:508:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:726:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:789:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:512:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:735:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:795:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:193:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:277:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:278:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
Expand All @@ -4297,7 +4301,6 @@ const X_MS_LIB_CAPABILITY = "x-ms-lib-capability";
// src/index.ts:8:12 - (tsdoc-characters-after-block-tag) The token "@azure" looks like a TSDoc tag but contains an invalid character "/"; if it is not a tag, use a backslash to escape the "@"
// src/index.ts:8:4 - (tsdoc-undefined-tag) The TSDoc tag "@module" is not defined in this configuration
// src/request/AuthenticationHeaderParser.ts:74:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/request/BaseAuthRequest.ts:55:5 - (ae-forgotten-export) The symbol "BrokerParameters" needs to be exported by the entry point index.d.ts
// src/request/ScopeSet.ts:72:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}'
// src/request/ScopeSet.ts:73:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}'
// src/response/ResponseHandler.ts:430:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
Expand Down
40 changes: 23 additions & 17 deletions lib/msal-common/src/client/AuthorizationCodeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,11 +315,12 @@ export class AuthorizationCodeClient extends BaseClient {
);

const parameterBuilder = new RequestParameterBuilder(
request.correlationId,
this.performanceClient
);

parameterBuilder.addClientId(
request.brokerParameters?.embeddedClientId ||
request.embeddedClientId ||
request.tokenBodyParameters?.[AADServerParamKeys.CLIENT_ID] ||
this.config.authOptions.clientId
);
Expand Down Expand Up @@ -477,6 +478,13 @@ export class AuthorizationCodeClient extends BaseClient {
}
}

if (request.embeddedClientId) {
parameterBuilder.addBrokerParameters({
brokerClientId: this.config.authOptions.clientId,
brokerRedirectUri: this.config.authOptions.redirectUri,
});
}

if (request.tokenBodyParameters) {
parameterBuilder.addExtraQueryParameters(
request.tokenBodyParameters
Expand All @@ -496,10 +504,6 @@ export class AuthorizationCodeClient extends BaseClient {
});
}

if (request.brokerParameters) {
parameterBuilder.addBrokerParameters(request.brokerParameters);
}

return parameterBuilder.createQueryString();
}

Expand All @@ -510,17 +514,23 @@ export class AuthorizationCodeClient extends BaseClient {
private async createAuthCodeUrlQueryString(
request: CommonAuthorizationUrlRequest
): Promise<string> {
// generate the correlationId if not set by the user and add
const correlationId =
request.correlationId ||
this.config.cryptoInterface.createNewGuid();

this.performanceClient?.addQueueMeasurement(
PerformanceEvents.AuthClientCreateQueryString,
request.correlationId
correlationId
);

const parameterBuilder = new RequestParameterBuilder(
correlationId,
this.performanceClient
);

parameterBuilder.addClientId(
request.brokerParameters?.embeddedClientId ||
request.embeddedClientId ||
request.extraQueryParameters?.[AADServerParamKeys.CLIENT_ID] ||
this.config.authOptions.clientId
);
Expand All @@ -534,10 +544,6 @@ export class AuthorizationCodeClient extends BaseClient {
// validate the redirectUri (to be a non null value)
parameterBuilder.addRedirectUri(request.redirectUri);

// generate the correlationId if not set by the user and add
const correlationId =
request.correlationId ||
this.config.cryptoInterface.createNewGuid();
parameterBuilder.addCorrelationId(correlationId);

// add response_mode. If not passed in it defaults to query.
Expand Down Expand Up @@ -684,8 +690,11 @@ export class AuthorizationCodeClient extends BaseClient {
);
}

if (request.brokerParameters) {
parameterBuilder.addBrokerParameters(request.brokerParameters);
if (request.embeddedClientId) {
parameterBuilder.addBrokerParameters({
brokerClientId: this.config.authOptions.clientId,
brokerRedirectUri: this.config.authOptions.redirectUri,
});
}

this.addExtraQueryParams(request, parameterBuilder);
Expand Down Expand Up @@ -729,6 +738,7 @@ export class AuthorizationCodeClient extends BaseClient {
request: CommonEndSessionRequest
): string {
const parameterBuilder = new RequestParameterBuilder(
request.correlationId,
this.performanceClient
);

Expand All @@ -754,10 +764,6 @@ export class AuthorizationCodeClient extends BaseClient {
parameterBuilder.addLogoutHint(request.logoutHint);
}

if (request.brokerParameters) {
parameterBuilder.addBrokerParameters(request.brokerParameters);
}

this.addExtraQueryParams(request, parameterBuilder);

return parameterBuilder.createQueryString();
Expand Down
8 changes: 6 additions & 2 deletions lib/msal-common/src/client/BaseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,15 @@ export abstract class BaseClient {
*/
createTokenQueryParameters(request: BaseAuthRequest): string {
const parameterBuilder = new RequestParameterBuilder(
request.correlationId,
this.performanceClient
);

if (request.brokerParameters) {
parameterBuilder.addBrokerParameters(request.brokerParameters);
if (request.embeddedClientId) {
parameterBuilder.addBrokerParameters({
brokerClientId: this.config.authOptions.clientId,
brokerRedirectUri: this.config.authOptions.redirectUri,
});
}

if (request.tokenQueryParameters) {
Expand Down
14 changes: 9 additions & 5 deletions lib/msal-common/src/client/RefreshTokenClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,12 @@ export class RefreshTokenClient extends BaseClient {

const correlationId = request.correlationId;
const parameterBuilder = new RequestParameterBuilder(
correlationId,
this.performanceClient
);

parameterBuilder.addClientId(
request.brokerParameters?.embeddedClientId ||
request.embeddedClientId ||
request.tokenBodyParameters?.[AADServerParamKeys.CLIENT_ID] ||
this.config.authOptions.clientId
);
Expand Down Expand Up @@ -475,16 +476,19 @@ export class RefreshTokenClient extends BaseClient {
}
}

if (request.embeddedClientId) {
parameterBuilder.addBrokerParameters({
brokerClientId: this.config.authOptions.clientId,
brokerRedirectUri: this.config.authOptions.redirectUri,
});
}

if (request.tokenBodyParameters) {
parameterBuilder.addExtraQueryParameters(
request.tokenBodyParameters
);
}

if (request.brokerParameters) {
parameterBuilder.addBrokerParameters(request.brokerParameters);
}

return parameterBuilder.createQueryString();
}
}
2 changes: 2 additions & 0 deletions lib/msal-common/src/config/ClientConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export type AuthOptions = {
azureCloudOptions?: AzureCloudOptions;
skipAuthorityMetadataCache?: boolean;
instanceAware?: boolean;
redirectUri?: string;
};

/**
Expand Down Expand Up @@ -274,6 +275,7 @@ function buildAuthOptions(authOptions: AuthOptions): Required<AuthOptions> {
azureCloudOptions: DEFAULT_AZURE_CLOUD_OPTIONS,
skipAuthorityMetadataCache: false,
instanceAware: false,
redirectUri: "https://localhost",
...authOptions,
};
}
Expand Down
5 changes: 2 additions & 3 deletions lib/msal-common/src/request/BaseAuthRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { AzureCloudOptions } from "../config/ClientConfiguration.js";
import { StringDict } from "../utils/MsalTypes.js";
import { StoreInCache } from "./StoreInCache.js";
import { ShrOptions } from "../crypto/SignedHttpRequest.js";
import { BrokerParameters } from "./BrokerParameters.js";

/**
* BaseAuthRequest
Expand All @@ -30,7 +29,7 @@ import { BrokerParameters } from "./BrokerParameters.js";
* - storeInCache - Object containing boolean values indicating whether to store tokens in the cache or not (default is true)
* - scenarioId - Scenario id to track custom user prompts
* - popKid - Key ID to identify the public key for PoP token request
* - brokerParameters - Broker parameters
* - embeddedClientId - Embedded client id
*/
export type BaseAuthRequest = {
authority: string;
Expand All @@ -52,5 +51,5 @@ export type BaseAuthRequest = {
storeInCache?: StoreInCache;
scenarioId?: string;
popKid?: string;
brokerParameters?: BrokerParameters;
embeddedClientId?: string;
};
16 changes: 0 additions & 16 deletions lib/msal-common/src/request/BrokerParameters.ts

This file was deleted.

3 changes: 0 additions & 3 deletions lib/msal-common/src/request/CommonEndSessionRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import { AccountInfo } from "../account/AccountInfo.js";
import { StringDict } from "../utils/MsalTypes.js";
import { BrokerParameters } from "./BrokerParameters.js";

/**
* CommonEndSessionRequest
Expand All @@ -16,7 +15,6 @@ import { BrokerParameters } from "./BrokerParameters.js";
* - state - A value included in the request to the logout endpoint which will be returned in the query string upon post logout redirection
* - logoutHint - A string that specifies the account that is being logged out in order to skip the server account picker on logout
* - extraQueryParameters - String to string map of custom query parameters added to the /authorize call
* - brokerParameters - Broker parameters
*/
export type CommonEndSessionRequest = {
correlationId: string;
Expand All @@ -26,5 +24,4 @@ export type CommonEndSessionRequest = {
state?: string;
logoutHint?: string;
extraQueryParameters?: StringDict;
brokerParameters?: BrokerParameters;
};
Loading

0 comments on commit 7424edf

Please sign in to comment.