-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: use accountID
instead of email
in AuthScreens
logic
#20035
feat: use accountID
instead of email
in AuthScreens
logic
#20035
Conversation
@Beamanator @fedirjh One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
All you @fedirjh - please test when you get a chance 🙏 |
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.
Code looks good, I will test it
// If they do not match it might be due to encoding, so check the raw value | ||
// Capture the un-encoded text in the email param | ||
const accountIDParamRegex = /[?&]accountID=([^&]*)/g; | ||
const matches = accountIDParamRegex.exec(transitionURL); | ||
const linkedAccountID = lodashGet(matches, 1, null); | ||
return linkedAccountID !== sessionAccountID.toString(); |
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.
Not sure if this is really needed since the accountID
is a number
and doesn't contain special characters that require encoding.
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 paramsAccountID
is being returned as null above, so this part is necessary. We could fix it by passing window.location.search
instead of window.location.href
to the function, but this would require changes in LogOutPreviousUserPage
to also pass this in the future when the function usage is updated. Is this something we want to change?
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.
Thank for clarification ,That looks good.
@BeeMargarida Not sure if this apply for this PR but we can add an extra test for web :
|
@fedirjh @Beamanator Updated! Left a comment in one of the reviews above and also added the extra test to the PR description, thank you! |
@BeeMargarida It looks like there is a bug on IOS when we deep link auto-signed urls , It doesn’t seems to be related to this PR , I get these warning and the screens is stuck on loading , I have to close the app and reopen it: Steps to reproduce :
|
Reviewer Checklist
Screenshots/VideosWebTest.1.movTest.2.movMobile Web - Chromechrome.Test.1.movChrome.Test.2.movMobile Web - SafariIOS.Safari.Test.1.movIOS.Safari.Test.2.movDesktopDesktop.Test.1.movDesktop.Test.2.moviOSAndroid |
Tests are passing for All platforms except Native (IOS/Android). |
@fedirjh Regarding the issue you mentioned above, it's also happening in |
@BeeMargarida why it only affects native ? @Beamanator should we open a new issue and address it separately or should we fix it in this PR ? I think if the fix is simple then we can fix it in this PR. |
Investigating this now 🔎 not sure if it might be related to development setup. I've tried navigating from the OldDot directly in the mobile browser, but it seems deep linking is not triggering. Does this issue also happen with the NewDot prod app? |
Closing since this being done in another branch: #20157 (comment) |
Details
A part of the 'Secure Logins' issue, point 6.
Use accountID instead of
email
in AuthScreen auth logic. Made a new method calledisLoggingInAsNewUserWithAccountID
, since theisLoggingInAsNewUser
method is still being used in other place.Fixed Issues
$ #19007
#19007 (comment)
Tests
Only possible in Web:
Login into NewDot and verify that the default behavior is the same
Go to Old Dot
Click on the FAB button on the bottom left corner
Copy paste the link generated and switch to
localhost
but maintaining the same URL paramsCheck that the default behavior does not change, the page will redirect to the NewDot running locally
Login as user 1 in new dot
Open old dot and login as user 2
Click on the FAB button on the bottom left corner
Copy paste the link generated and switch to localhost but maintaining the same URL params
Check that the default behavior does not change, the page will redirect to the NewDot running locally
User 1 should be logged out, and User 2 should be automatically logged in.
Not in web:
Offline tests
Not applicable
QA Steps
Only possible in Web:
Login into NewDot and verify that the default behavior is the same
Go to Old Dot
Click on the FAB button on the bottom left corner
Check that the default behavior does not change
Login as user 1 in new dot
Open old dot and login as user 2
Click on the FAB button on the bottom left corner
Check that the default behavior does not change, the page will redirect to the NewDot
User 1 should be logged out, and User 2 should be automatically logged in.
Not in web:
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
web_t.mp4
Mobile Web - Chrome
mchrome.mov
Mobile Web - Safari
msafari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android_t.mp4