Custom auth/alban/react next sample app signin #7624

albanx · 2025-03-08T01:19:23Z

Completed Sign In sample with NextJs
[Bug] Created Sign Up but not working due to missing exposure of submitCode in the result.state
Removed correlationId from Fetch client because it is present already in the header
Removed HTTPS checks in SDK, it is not needed to be handled by JS and blocks local development

…ample-skeleton

Coded Sign Up but not working

…ple-app-signin # Conflicts: # lib/msal-custom-auth/src/core/network_client/http_client/FetchHttpClient.ts

lib/msal-custom-auth/.editorconfig

lib/msal-custom-auth/src/CustomAuthPublicClientApplication.ts

lib/msal-custom-auth/src/core/network_client/custom_auth_api/BaseApiClient.ts

lib/msal-custom-auth/src/core/network_client/http_client/FetchHttpClient.ts

shenj · 2025-03-10T14:54:09Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

+                password,
+            });
+
+            if (result.error) {


IMO, it is better to use following check in the if (xxx).
if (result.state?.type === SignInState.Failed)

If I check state as above this causes a type check error (for the reasons widely discussed in the chat today). This happens because the result.error is ambiguous due to the polymorphic type of result:

I didn't see any problem.

Check SignUp return result not signIn

Here is the signup:

shenj · 2025-03-10T14:55:20Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

+        setError("");
+        setLoading(true);
+
+        try {


There is no need the type catch here since the SDK is guaranteed no error thrown

there is a await promise resolve there, meaning that the try catch is a must of the promise rejects

Afaik, the promise rejection only occurs if an error is thrown and not caught inside the method. But as I said, any API in this SDK is guaranteed no error thrown.

A JavaScript promise has 2 statuses: resolve, reject.
The compiler or javascript cannot know that the SDK internally handles all errors of that away reject.
Without the try-catch, any error of the promise would cause the function to immediately throw an exception and potentially crash.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#handling_rejected_promises

I didn't get it. If we guarantee the API will never reject the promise or throw an error, is the try catch still necessary?

You can guarantee it, but JavaScript engine cannot know it. No developer can know it, unless the read the source of the SDK itself.
If I read await something -> that means it can throw an exception.

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

shenj · 2025-03-10T15:00:00Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

+    const [email, setEmail] = useState("");
+    const [code, setCode] = useState("");
+    const [error, setError] = useState("");
+    const [flowState, setFlowState] = useState<any>(null);


Instead of using any here, It can set as SignInCompleted | SignInFailed | SignInCodeRequired | SignInPasswordRequired.

This points out the point I was making, in terms of S of SOLID.
flowState has many responsibilities and if we update the SDK with a new state, the engineer have to update his integration code, resulting in breaking the backward compatibility exactly because it is breaking the O of open close.

I tried this does not work

Why it doesn't work? Also we can consider using the AuthFlowStateBase since it is better than any.

Similar suggestion: can we use the base class AuthFlowStateBase or SignUpCompleted | SignUpFailed | SignUpCodeRequired | SignUpPasswordRequired | SignUpAttributesRequired to replace any?

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

yarn.lock

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

…ple-app-signin

…ple-app-signin # Conflicts: # package-lock.json

…ple-app-signin

shenj · 2025-03-18T21:19:36Z

Do you think we can move this sample app into the repo Kaushik suggested? I am planning the create the PR to MSAL for JS team soon for checking in the changes into the dev branch. If it is possible, it is better to exclude the sample app in that PR.

albanx · 2025-03-18T21:24:45Z

Do you think we can move this sample app into the repo Kaushik suggested? I am planning the create the PR to MSAL for JS team soon for checking in the changes into the dev branch. If it is possible, it is better to exclude the sample app in that PR.

Yes I have moved them partially, not the last version. Otherwise I cannot run them in the repo until the package is published.

shenj · 2025-03-18T21:42:23Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

+                    setError("Incorrect password");
+                } else {
+                    setError("An error occurred during sign in");
+                }


Maybe I misunderstood, but any reason why you didn't use the handleError function you defined in the utils/index.ts?

shenj · 2025-03-18T21:44:05Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

+        setLoading(true);
+
+        try {
+            const result = await flowState.submitCode(code);


As discussed earlier, can you double check whether flowState.submitCode(code) is working?

I have ran the sign in different times, it works, unless there has been some recent updates.

shenj · 2025-03-18T21:46:03Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-in/page.tsx

+    const [password, setPassword] = useState("");
+    const [code, setCode] = useState("");
+    const [error, setError] = useState("");
+    const [flowState, setFlowState] = useState<any>(null);


Do you think it is better to use the state base class AuthFlowStateBase to replace any?

Or use SignInCompleted | SignInFailed | SignInCodeRequired | SignInPasswordRequired?

Can be AuthFlowStateBase , the later one I really do not recommend. This is how developer experience will look with the latest:
const [flowState, SetFlowState] = useState< checking docs

I see SignInCompleted , perfect
const [flowState, SetFlowState] = useState<SignInCompleted>(null)

Ahh wait there is more
const [flowState, SetFlowState] = useState<SignInCompleted | SignInFailed>(null)

no wait more types in or

const [flowState, SetFlowState] = useState<SignInCompleted | SignInFailed | SignInCodeRequired >(null)
(how many possible types in OR I should have, I have to check all of them and find out by reading a long boring documentation)

1 month after the SDK releseas version 2.0 ... Ah crap I have to update all my code again

const [flowState, SetFlowState] = useState<SignInCompleted | SignInFailed | SignInCodeRequired | **ANewSignInState** >(null)

if (flowState instanceOf SignInCompleted) if (flowState instanceOf SignInFailed ) if (flowState instanceOf SignInCodeRequired ) **if (flowState instanceOf ANewSignInState)**

shenj · 2025-03-18T21:51:35Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

+        setLoading(true);
+
+        try {
+            const app = await CustomAuthPublicClientApplication.create(customAuthConfig);


IMHO, the app can be shared cross the whole application for sign-in, sign-up and sspr. Is it possible we can use the single instance of app in this sample app?

There is no point and need to share the same app instance for different flows that are not connected in the lifecycle of an app. Imagine Sign Up , Sign In and SSPR as different flows that are separated in time and usage , as it happens in every app (a user does not go through the all flows in the same app cycle). Making this flows sharing the same instance it kind of tight them together for no reason.

shenj · 2025-03-18T21:57:17Z

samples/msal-custom-auth-samples/react-sample-nextjs/src/app/sign-up/page.tsx

+        <div style={styles.container}>
+            <h2>Sign Up</h2>
+            {flowState?.type === SignUpState.CodeRequired ? (
+                <CodeForm onSubmit={handleCodeSubmit} code={code} setCode={setCode} loading={loading} />


May I ask why we don't have the PasswordForm?

This is SignUp with OTP does not asks for password which I based on the past samples. That is the configuration of the sample tenant. I can add the password field but it will be optional

microsoft-github-policy-service · 2025-04-14T13:55:43Z

Reminder: This PR appears to be stale. If this PR is still a work in progress please mark as draft.

axhaferllari added 6 commits March 2, 2025 00:49

add base SignIn form with config _not working_

03c0f6a

Merge branch 'custom-auth/main' into custom-auth/alban/react-nextjs-s…

a99958c

…ample-skeleton

Add forms

9ccf8a4

refactor

a188b23

Complete Sign In

be58ed5

Coded Sign Up but not working

remove custom tenant config

dd168fd

albanx requested review from yongdiw, shenj, tnorling, sameerag, jo-arroyo, hectormmg, lalimasharda, peterzenz, bgavrilMS, Robbie-Microsoft, konstantin-msft and shylasummers as code owners March 8, 2025 01:19

github-actions bot added samples Related to the samples apps for the library. msal-custom-auth labels Mar 8, 2025

Merge branch 'custom-auth/main' into custom-auth/alban/react-next-sam…

85f7245

…ple-app-signin # Conflicts: # lib/msal-custom-auth/src/core/network_client/http_client/FetchHttpClient.ts