Add sign in with microsoft #13425
Add sign in with microsoft #13425
Conversation
PabloBuchu
requested review from
Frizi,
farmaazon,
vitvakatu,
kazcw,
AdRiley,
indiv0 and
somebody1234
as code owners
PabloBuchu
added
the
CI: No changelog needed
| // When using Microsoft we need to first invalidate auth0 and windows live sessions before calling cognito. | ||
| const session = await amplify.Auth.currentSession() | ||
| const identities = session.getIdToken().decodePayload()['identities'] | ||
| const providerName = identities?.length ? identities[0]['providerName'] : undefined |
There was a problem hiding this comment.
| const providerName = identities?.length ? identities[0]['providerName'] : undefined | |
| const providerName = identities?.[0]?.['providerName'] |
| /** | ||
| * Sign in via the Microsoft federated identity provider. | ||
| * | ||
| * This function will open the GitHub authentication page in the user's browser. The user will |
| const identities = session.getIdToken().decodePayload()['identities'] | ||
| const providerName = identities?.length ? identities[0]['providerName'] : undefined | ||
| if (providerName === MICROSOFT_PROVIDER) { | ||
| window.open($config.MICROSOFT_SIGN_OUT_URL, '_blank') |
There was a problem hiding this comment.
Not calling Auth.signOut() for microsoft provider here looks incorrect. This at least needs an comment explaining that the expected outcome of opening that window is that we will eventually hit a //auth/federated deeplink handler.
I believe this logic now introduces a subtle but consequential bug - the promise returned from this function will resolve too early. It won't actually wait for the signOut to happen. The microsoft branch should return a promise that actually waits for the expected deeplink handler to be hit and processed properly. At that point the actual signOut logic could actually continue running in this function, instead of inside the deeplink handler. That would avoid duplication of following code.
Also, what happens if the user closes that window before it had a chance to do anything?
There was a problem hiding this comment.
I will revert this change
app/gui/.dev-env
Outdated
There was a problem hiding this comment.
This reverts a submodule update. Probably by accident?
somebody1234
removed
the
CI: Ready to merge
| * This provider alone requires a string because it is not a standard provider, and thus has no | ||
| * constant defined in the AWS Amplify library. | ||
| */ |
There was a problem hiding this comment.
This provider alone ...
There's also Github so both are not alone.
* wip * microsoft sign in integration * fix some docstrings * prettier * update ymls * hide microsoft sign in behind the flag * fix formatting * pr review comments * add cognito error handling * revert microsoft signout url * remove signin feature flag (cherry picked from commit e43b3c3)
Pull Request Description
This PR adds a button for users to signing with Microsoft Entra ID.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.