[msal-common]Combine Request module for node and browser components
#1682
Conversation
|
cc @pkanher617 |
| try { | ||
| // Create auth code request and generate PKCE params | ||
| const authCodeRequest: AuthorizationCodeRequest = await this.generateAuthorizationCodeRequest(validRequest); |
There was a problem hiding this comment.
This is good for now. When we add interactiveFlow which would need PKCE generation for msal-node we can think of migrating this to msal-common or have equivalent code in msal-node.
There was a problem hiding this comment.
cc @sangonzal we need to track this for msal-node
| } catch (err) { | ||
| throw BrowserAuthError.createTokenRequestCacheError(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
+1 for this. All browser specific cache handling now is clearly demarcated between msal-common and msal-browser
| // Acquire token with retrieved code. | ||
| return this.authModule.acquireToken(codeResponse); | ||
| const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, userState, cachedNonce); |
There was a problem hiding this comment.
let's discuss nonce, I wonder why this is not applicable to node. I do understand that redirect use case for msal-browser poses unique challenges w.r.t application memory. Lets revisit this.
| @@ -146,6 +147,9 @@ export class AuthorizationCodeClient extends BaseClient { | |||
| // add response_type = code | |||
| parameterBuilder.addResponseTypeCode(); | |||
|
|
|||
| // add library info parameters | |||
| parameterBuilder.addLibraryInfo(this.config.libraryInfo); | |||
|
|
|||
| if (request.codeChallenge) { | |||
| RequestValidator.validateCodeChallengeParams(request.codeChallenge, request.codeChallengeMethod); | |||
There was a problem hiding this comment.
Please move this validation also to ParameterBuilder as others.
| throw ClientAuthError.createEndpointDiscoveryIncompleteError(e); | ||
| } | ||
| // Initialize authority or use default, and perform discovery endpoint check. | ||
| const acquireTokenAuthority = (codeRequest && codeRequest.authority) ? AuthorityFactory.createInstance(codeRequest.authority, this.networkClient) : this.defaultAuthority; |
There was a problem hiding this comment.
This will change once authority creation is platform specific as msal-node does; cc @sangonzal to help @pkanher617 if needed
| this.spaCacheManager.resetTempCacheItems(); | ||
| this.account = null; | ||
| throw e; | ||
| // Get current cached tokens |
There was a problem hiding this comment.
FYI reviewers: This will change after silentFlow PR is up for msal-node.
github-actions
bot
added
msal-browser
sameerag
requested review from
DarylThayil,
jasonnutter,
jmckennon,
jo-arroyo and
hectormmg
as code owners
|
lots of files here, what needs to be reviewed? |
This PR aims to:
requestsfor auth-code flow support inmsal-nodeandmsal-browserSPAClient.tsas a wrapper on top ofAuthorizationCodeClient.tsServerCodeRequestParameters.tsandServerTokenRequestParameters.tsand haveSPAClient.tsreferenceRequestParameterBuilder.tsinstead