Initial skeleton and design document for Native Auth features #7414
Conversation
github-actions
bot
added
documentation
| @@ -0,0 +1,107 @@ | |||
| 'use strict'; | |||
| (function (global, factory) { | |||
There was a problem hiding this comment.
this looks like another sample, do we need this here?
| SignUpOptions, | ||
| } from "./NativeAuthActionOptions.js"; | ||
|
|
||
| export interface INativeAuthPublicClientApplication { |
There was a problem hiding this comment.
Suggestion on the naming of the interface (not sure though what is the followed standard):
NativeAuthClient
| * Licensed under the MIT License. | ||
| */ | ||
|
|
||
| export const GrantTypeConstants = { |
There was a problem hiding this comment.
Maybe the Constants postfix should not be needed
| import { NativeAuthOperatingContext } from "./operating_context/NativeAuthOperatingContext.js"; | ||
|
|
||
| export class NativeAuthPublicClientApplication | ||
| extends PublicClientApplication |
There was a problem hiding this comment.
Why does this extends PublicClientApplicatoin from msal-browser?
| // Code is required. | ||
| // Collect code from customer. | ||
| const code = "test-code"; | ||
| const submitCodeResult = await signInResult.nextStateHandler.submitCode(code); |
There was a problem hiding this comment.
I think this not the ideal design, because it starts from the assumption that the signInResults is a a specific type and has a submitCode function. I think as per Single Responsibility, signInResults object should contain only the state of the app.signIn call. The submit function instead should be exposed in the app object, so that is not tight to the result type.
There was a problem hiding this comment.
I think an alternative approach to expose the SDK functions can be like (high level design):
const authApp= NativeAuthPublicClientApplication.create(config);
async function startSignIn() {
const signInResult = await app.signIn(signInOptions);
if (signInResult .state === SignInState.CodeRequired) {
navigate("/code-required");
} else if (signInResult .state === SignInState.PasswordRequired) {
navigate("/password-required");
} else if (signInResult .state === SignInState.Error) {
// show error
}
}
async function onCodeSubmit(code) {
const result= await app.signInCodeSubmit(code);
if (signInResult .state === SignInState.CodeRequired) {
navigate("/code-required");
} else if (signInResult .state === SignInState.PasswordRequired) {
navigate("/password-required");
} else if (signInResult .state === SignInState.Error) {
// show error
}else if (signInResult .state === SignInState.Completed) {
// fetch token
}
}
By exposing the functions to authApp instance allows decoupling, making more flexible to consume the library.
|
|
||
| const accountInfo: AccountInfo | undefined; | ||
|
|
||
| if (signInResult.isFlowCompleted()) { |
There was a problem hiding this comment.
signInResult should only contain the state of the function call, in this case isFlowCompleted can be converted to a state.
| * Creates a new instance of a PublicClientApplication with the given configuration. | ||
| * @param config - A configuration object for the PublicClientApplication instance | ||
| */ | ||
| static create( |
There was a problem hiding this comment.
There should be a method to retrive as well the created instance
| /* | ||
| * Reset password handler for the state of code required. | ||
| */ | ||
| export class ResetPasswordCodeRequiredStateHandler extends ResetPasswordStateHandler { |
There was a problem hiding this comment.
why this class is extending ResetPasswordStateHandler?
| try { | ||
| // The authority URL need to be revisited to ensure it is correct. | ||
| const authorityUrl = this.config.auth.authority; | ||
|
|
||
| // start the signin flow | ||
| const signInStartParams: SignInStartParams = new SignInStartParams( | ||
| authorityUrl, | ||
| this.config.auth.clientId, | ||
| correlationId, | ||
| this.nativeAuthConfig.nativeAuth.challengeTypes ?? [], | ||
| signInOptions.scopes ?? [], | ||
| signInOptions.username, | ||
| signInOptions.password | ||
| ); | ||
|
|
||
| const startResult = await this.signInClient.start( | ||
| signInStartParams | ||
| ); | ||
|
|
||
| if (startResult instanceof SignInWithContinuationTokenResult) { | ||
| // require password | ||
| if (!signInOptions.password) { | ||
| return new SignInResult( | ||
| undefined, | ||
| new SignInPasswordRequiredStateHandler( | ||
| this.signInClient, | ||
| correlationId, | ||
| startResult.continuationToken, | ||
| this.nativeAuthConfig, | ||
| signInOptions.scopes | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| // if the password is provided, then try to get token silently. | ||
| const signInSubmitPasswordParams = | ||
| new SignInSubmitPasswordParams( | ||
| authorityUrl, | ||
| this.config.auth.clientId, | ||
| correlationId, | ||
| this.nativeAuthConfig.nativeAuth.challengeTypes ?? [], | ||
| signInOptions.scopes ?? [], | ||
| startResult.continuationToken, | ||
| signInOptions.password | ||
| ); | ||
|
|
||
| const completedResult = await this.signInClient.submitPassword( | ||
| signInSubmitPasswordParams | ||
| ); | ||
|
|
||
| const accountManager = new AccountInfo( | ||
| completedResult.authenticationResult.account, | ||
| correlationId, | ||
| this.nativeAuthConfig | ||
| ); | ||
|
|
||
| return new SignInResult(accountManager); | ||
| } else if (startResult instanceof SignInCodeSendResponse) { | ||
| // require code | ||
| return new SignInResult( | ||
| undefined, | ||
| new SignInCodeRequiredStateHandler( | ||
| this.signInClient, | ||
| correlationId, | ||
| startResult.continuationToken, | ||
| this.nativeAuthConfig, | ||
| signInOptions.scopes | ||
| ) | ||
| ); | ||
| } else { | ||
| throw new UnexpectedError("Unknow SignInStartResult type"); | ||
| } | ||
| } catch (error) { | ||
| return SignInResult.createWithError(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
My suggestion here is to refactor this method because there are early returns disconected from each other, making the code hard to read and test.
| signInOptions.password | ||
| ); | ||
|
|
||
| const completedResult = await this.signInClient.submitPassword( |
There was a problem hiding this comment.
The submitPassword method here might not necessarily return a complete, Can the server might respond with another challenge?
| * @param signInOptions - Options for signing in the user. | ||
| * @returns The result of the operation. | ||
| */ | ||
| async signIn(signInOptions: SignInInputs): Promise<SignInResult> { |
There was a problem hiding this comment.
as discussed, IMHO, this call should return the signInClient instead of a signInResult, or eventually allow the user to specify the flow
No description provided.