[msal-node][msal-common] Client applications, authorization code and device code implementation #1409
Conversation
[msal-node] lerna bootstrap and lerna builds
sangonzal
requested review from
pkanher617,
sameerag,
jasonnutter,
DarylThayil,
jmckennon and
tnorling
sangonzal
changed the title
| @@ -0,0 +1,231 @@ | |||
| /* | |||
There was a problem hiding this comment.
Please ignore this file. It's here as placeholder, and @sameerag plans to update this with her cache work.
| @@ -0,0 +1,82 @@ | |||
| /* | |||
There was a problem hiding this comment.
Considering moving these methods to RequestUtils. Thoughts?
There was a problem hiding this comment.
IMO this file here is actually more representative of a RequestUtils file and the other (current) RequestUtils file is really just a bunch of helper/setter functions that should probably live where that request it being built
There was a problem hiding this comment.
I've refactored away from using the static methods and just created a "ParametersBuilder" that contains these methods, as they will be shared by all the clients in msal-common
| * While the user authenticates on a separate device, MSAL polls the the token endpoint of security token service for the interval | ||
| * specified in the device code resonse (usually 15 minutes). To stop polling and cancel the request, set cancellationToken.cancel = true. | ||
| * */ | ||
| export class CancellationToken { |
There was a problem hiding this comment.
I may just be ignorant here, but is the plan to add to this class? what's the purpose of having a class with just one field and nothing else?
There was a problem hiding this comment.
CancellationToken is a pattern used to be able to short-circuit a long running operation.
| @@ -0,0 +1,82 @@ | |||
| /* | |||
There was a problem hiding this comment.
IMO this file here is actually more representative of a RequestUtils file and the other (current) RequestUtils file is really just a bunch of helper/setter functions that should probably live where that request it being built
| * @type AuthorizationCodeRequest: Request object passed by user to acquire a token from the server exchanging a valid authorization code | ||
| * (second leg of OAuth2.0 Authorization Code flow) | ||
| * | ||
| * scopes: A space-separated array of scopes for the same resource. |
There was a problem hiding this comment.
Just wanted to say that I appreciate the effort that gets put into the readability of many of these comments, well done
| * @param paramsMap | ||
| * @param authority | ||
| */ | ||
| static createQueryString(paramsMap: Map<string, string>): string { |
There was a problem hiding this comment.
Similar to the comment on RequestValidator, I think every function in this file probably belongs somewhere in the request folder else except for this one as createQueryString
|
made lots of comments, but overall looking good. It is definitely a challenge to review in totality with the number of changes. In the future I would encourage thinking through how to do this in say 10 smaller prs. Maybe something like
That way you can add tests as you go for each stage and the mental over head of each pr is fairly small. Even if you end up altering previous pieces as you go its no big deal because the number of changes is fairly low. I also find that it helps me high level plan out my design better to do smaller prs. |
Adds implementation public APIs and implementation for authorization code flow and device code flow. Will add a separate PR for unit tests- won't merge this until that is out and reviewed.
Adds to msal-common:
response:
request:
Add to msal-node:
Adds to samples: