-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SAML NewDot] Add SAML flow for web, mweb, desktop #28372
Conversation
Lint is failing Is it ready for external review or wait until https://github.com/Expensify/Web-Expensify/pull/38972 is deployed? |
I think this should be on HOLD until Web-E PR is deployed. But yes, It can be reviewed |
@situchan you can review the portions under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing any config? I am trying to follow External step but I am not able to reach SAML page on web. I enabled permission in Permissions.ts
Screen.Recording.2023-10-02.at.7.48.12.AM.mov
|
||
function SAMLSignInPage(props) { | ||
useEffect(() => { | ||
window.open(`${CONFIG.EXPENSIFY.SAML_URL}?email=${props.credentials.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}`, '_self'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that this page can be accessed on native when newDotSAML
beta enabled and window.open
will crash.
App/src/pages/signin/SignInPage.js
Lines 92 to 96 in 4308871
// SAML is temporarily restricted to users on the beta or to users signing in on web and mweb | |
if (Permissions.canUseSAML() || platform === CONST.PLATFORM.WEB) { | |
shouldShowSAMLEnabledForm = hasAccount && hasLogin && isSAMLEnabled && !isSAMLRequired && isUsingSAMLLogin; | |
shouldInitiateSAMLLogin = hasAccount && hasLogin && isSAMLRequired; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 why would it crash? credentials.login
is the only portion we'd need to be defined and we'll only show the SAMLEnabledForm
(now ChooseSSOOrMagicCode
) or intiaite the SAML login (i.e. navigate to that URL immediately) if the login is set.
Updated once more! |
Code looks good. (lint failing) I started testing with private domain email.
Where can I enable this? |
In expensify.com under Settings > Domains > [Your Domain] > SAML |
I don't find Domains in Settings. Can you please share that url? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is open since long. So going to merge this now.
There is an issue with transitioning which will hopefully be fixed by #28984. If not, we can address it in a separate PR
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@NikkiWines @MonilBhavsar Are QA steps internal? |
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.85-0 🚀
|
@kavimuru since |
I'll also do some testing on staging just to try things out |
The redirect back to NewExpensify isn't working for me on staging, but I think it might be that we need to test this on prod 🤔 looking at the logs now |
Going to hide these changes behind a beta so we can test on prod without it causing issues for customers - #29798 |
I commented on the beta PR, but I don't think a beta is necessary. Ideally we can test this in a day or so, and I can't think of many downsides to having it accessible to real customers. Worst case, they run into errors and we fix the bug. |
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.86-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀
|
)(LogInWithShortLivedAuthTokenPage); | ||
export default withOnyx({ | ||
account: {key: ONYXKEYS.ACCOUNT}, | ||
session: {key: ONYXKEYS.SESSION}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session
prop appears unused. Do you know if it's needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove it most probably. Seems like we removed the usage, but forgot to remove this key here. /cc @NikkiWines to be double sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, we can remove this 👍 I can do that as part of #29526 unless you'd like to make a separate PR fro it @roryabraham
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikkiWines if you want to do it as part of #29526 that would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
const {translate} = useLocalize(); | ||
|
||
useEffect(() => { | ||
window.open(`${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}`, '_self'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.open
broke route history and user couldn't go back from SAML page. ref: #29825
Details
Adds support for signing in with SAML on web and mweb.
Fixed Issues
$ #28215
$ #28216
PROPOSAL: N/A Internal
Tests
(Internal)
(for Web, mWeb, desktop)
Go Back
option and confirm you're dropped back on the sign-in page with your email pre-filledContinue
Use Magic Code
option and confirm you get a magic code sent to your emailGo Back
and confirm you're back on the sign-in page with your email pre-filledContinue
Use Single Sign On
optionUse Single Sign On
option(for ios/android)
(External)
(for Web, mWeb, Desktop)
Go Back
option and confirm you're dropped back on the sign-in page with your email pre-filledContinue
Use Magic Code
option and confirm you get a magic code sent to your emailGo Back
and confirm you're back on the sign-in page with your email pre-filledContinue
Use Single Sign On
optionUse Single Sign On
option(for ios/android)
Offline tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
SAML Required Flow
web-required.mov
SAML Enabled Flow
web-enabled.mov
Mobile Web - Chrome
SAML Required Flow
SAML Enabled Flow
Simulator.Screen.Recording.-.iPhone.14.-.2023-09-28.at.17.01.20.mp4
Mobile Web - Safari
SAML Required Flow
mweb-required.mp4
SAML Enabled Flow
mweb-enabled.mp4
Desktop
SAML Required Flow
desktop-required.mov
SAML Enabled Flow
desktop-enabled.mov
iOS
SAML Required Flow
Simulator.Screen.Recording.-.iPhone.14.-.2023-10-12.at.18.36.53.mp4
(SAML Enabled is the same as the current magic code flow)
Android
SAML Required Flow
(SAML Enabled is the same as the current magic code flow)