Preserve information about saml session when logging out previous user V2#90449
Conversation
…when loging out previous user""
melvin-bot
Bot
requested review from
flaviadefaria and
pecanoro
and removed request for
a team
jnowakow
requested review from
a team and
AndrewGable
and removed request for
pecanoro
|
PR got revert last time because of problems on Staging. I'm working setting up account/domain that I could use to test SSO flow on staging |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@jnowakow Is it a straight revert? If it's straight revert maybe we don't need full review. Why did we revert it on the first place? There are a couple of things linked |
|
@jnowakow This was already reverted, so I think we can close this PR. Do we have any plans to open a new PR for a different fix? |
|
@pecanoro, @huult, it's straight revert. Original PR was reverted to fix this issue. Problem is that it was bug was found on staging and I can't test it on this environment. I can only test on dev where everything works fine. I've tried setting up domain that works on staging but without success now. #90114 looks like backend redirects to wrong URL so it's not issue introduced by this PR. I suspect that it's because applausetester+saml1@applause.expensifail.com is redirected back to |
|
However, I'll take a look at the links configuration and will see if something can be improved/simplified |
|
@jnowakow What are next steps then? I am bit confused about the last message 😄 |
|
Yes, we resolved this in the Slack thread: @jnowakow Please close this PR. |
@huult it's different issue |
|
@huult, title updated, testing steps remain the same |
|
|
||
| // Even though we are on staging the backend will redirect us to production url | ||
| if (isWeb && CONFIG.IS_IN_STAGING) { | ||
| expectedURL = expectedURL.replace('staging.', ''); |
There was a problem hiding this comment.
It looks like a hack to me. Do we plan to create a URL for staging? Since staging is not supported by the backend, I think we should either add it to beta or disable it on staging instead of replacing the URL with production.
There was a problem hiding this comment.
It looks like a hack because it is hack 😅 I only saw one domain (@applause.expensifail.com) that works on staging. I don't have access to it so I don't know how it's configured. If we disable SSO on staging, QA team won't be able to test it. I don't think it's an option.
There was a problem hiding this comment.
Yeah let's not do this. We can provide QA team a known working account for production, right?
There was a problem hiding this comment.
@AndrewGable I can set up one account and share it with you and QA for testing.
@jnowakow Can I confirm that QA does not need to test this on staging? If so, do we still need to merge this PR?
There was a problem hiding this comment.
Yeah let's not do this. We can provide QA team a known working account for production, right?
I believe QA team already has account that works on production. Does this mean they should stop testing SSO things on staging?
There was a problem hiding this comment.
Andrew is OOO. I'll look for someone else to take it over
There was a problem hiding this comment.
I think this can be relevant also: https://expensify.slack.com/archives/C08CZDJFJ77/p1779813981996219
There was a problem hiding this comment.
I replied on the Slack thread; it's the same convo here, right? We are just deciding whether to skip QA on staging?
| proxy: [ | ||
| { | ||
| context: ['/api', '/staging', '/chat-attachments', '/receipts'], | ||
| context: ['/api', '/staging', '/chat-attachments', '/receipts', '/authentication'], |
There was a problem hiding this comment.
Could you explain why we added authentication to the proxy context?
There was a problem hiding this comment.
When running dev build SSO requests were blocked by CORS. Internals have IdP redirecting back to dev web app and lack of this entry broke testing
There was a problem hiding this comment.
@jnowakow This seems out of scope for this ticket, as I don't see anything about it in the issue description. If we need this change, could you ask the internal team to confirm whether it is required? If they confirm it's needed, we can keep it. otherwise, I'd prefer to remove it.
| CONCIERGE_URL: `${expensifyURL}concierge/`, | ||
| RECEIPTS_URL: `${expensifyURL}receipts/`, | ||
| SAML_URL: `${expensifyURL}authentication/saml`, | ||
| SAML_URL: `${expensifyURLRoot}authentication/saml`, |
There was a problem hiding this comment.
What issue causes us to change the URL to expensifyURLRoot?
There was a problem hiding this comment.
Aim of this change is to enable SSO testing when App connects with backend via ngrok tunnel
There was a problem hiding this comment.
@jnowakow Can this be used in production as well?
There was a problem hiding this comment.
Yes, on production it will be just production url
There was a problem hiding this comment.
I'm not sure about that. @pecanoro, do you agree that we should change this URL?
@jnowakow Thanks for the update. If possible, could you please add a more detailed explanation of this PR? |
Screen.Recording.2026-05-20.at.10.37.30.movI see the issue is happening again. Is this included in this fix? |
|
Could you sync with main? |
It looks like navigation related issue not connected this one. Looks like when |
Done! |
|
@huult Friendly bump! |
@pecanoro @jnowakow has a question for the internal team and is waiting for their input: #90449 (comment) |
|
@huult All yours again! |
| const isWeb = getPlatform() === CONST.PLATFORM.WEB; | ||
| const queryString = isWeb ? `referer=ecash&authToken=${authToken}` : `appversion=${pkg.version}&referer=ecash&authToken=${authToken}`; | ||
| const expectedURL = isWeb ? CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL : CONST.SAML_REDIRECT_URL; | ||
|
|
There was a problem hiding this comment.
Please remove this space, it's unnecessary.
| // Even if the user was already authenticated in NewDot, we need to reauthenticate them with shortLivedAuthToken, | ||
| // because the old authToken stored in Onyx may be invalid. | ||
| signInWithShortLivedAuthToken(shortLivedAuthToken); | ||
| signInWithShortLivedAuthToken(shortLivedAuthToken, !!isSAML); |
There was a problem hiding this comment.
I see this change is similar to your previous change, but it is causing an issue: #90114. Could you please test this?
There was a problem hiding this comment.
It's crucial line of the PR. #90114 is caused by configuration issue on staging. We were discussing it
There was a problem hiding this comment.
We were discussing it
If possible, could you please mention this discussion here as well? Thanks. It will make future investigations easier if we encounter a similar issue again.
|
Could you check the failed checks? |
| // Even if the user was already authenticated in NewDot, we need to reauthenticate them with shortLivedAuthToken, | ||
| // because the old authToken stored in Onyx may be invalid. | ||
| signInWithShortLivedAuthToken(shortLivedAuthToken); | ||
| signInWithShortLivedAuthToken(shortLivedAuthToken, !!isSAML); |
There was a problem hiding this comment.
| signInWithShortLivedAuthToken(shortLivedAuthToken, !!isSAML); | |
| const {isProduction} = useEnvironment(); | |
| ... | |
| const shouldPassSAML = isProduction ? !!isSAML : undefined; | |
| signInWithShortLivedAuthToken(shortLivedAuthToken, shouldPassSAML); |
@jnowakow I think this is only needed for production, so we should add a check like above. Also, please add a comment explaining why we need to pass SAML in production.
There was a problem hiding this comment.
Why is it needed only for production? We shouldn't ignore this information on any environment
There was a problem hiding this comment.
If we use this option for staging, will this issue come back? #90114
There was a problem hiding this comment.
If IdP configuration won't be fixed then probably yes. But adding this check means "fix the bug on production and leave it on staging/dev" so I think it's even more hacky than what I propose earlier
There was a problem hiding this comment.
I think it's different because we don’t currently support staging and dev, so isSAML is only passed in production when calling signInWithShortLivedAuthToken. Adding this as suggested seems reasonable. But if there are concerns, we should also check with @pecanoro and @AndrewGable.
There was a problem hiding this comment.
Dev works if you have correct IdP, so I don't think we should block. But let's wait for @pecanoro or @AndrewGable for their opinion
Failing tests don't look related. Merging main should fix it |
|
I think the next step is for @AndrewGable to share his thoughts on this. |
4 participants
Explanation of Change
SSO login works by redirecting user to www.expensify.com then to IdP and back to App. All necessary information are passed in the URL. One of parameter is
isSAML. It's important information stored on session object. Without it logout is not working properly because we do not logout user via SSO. One piece of logic in the OD -> ND redirection is navigation toLogoutPreviousUserPage. This screen ignoredisSAMLparameter from URL causing problem with logout.Fixed Issues
$ #86416 (comment)
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
SLO-fixed.mov