-
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
Public channel - Can not login via Google SSO to a public channel #27937
Comments
Triggered auto assignment to @joekaufmanexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
Reproduced! |
Job added to Upwork: https://www.upwork.com/jobs/~019ae7b315de0f271b |
Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Public channel - Can not login via Google SSO to a public channel What is the root cause of that problem?Well the way we handle Signing parts of the application is different from flow we're doing in this issue. When we open any public room
Now when we try to login via google we are sending
Because we skip What changes do you think we should make in order to solve the problem?When sending App/src/libs/Network/enhanceParameters.js Line 24 in a07bb15
We need to add App/src/libs/Network/enhanceParameters.js Lines 13 to 15 in a07bb15
authToken parameter would skipped for this request
Points to Note Before
What alternative solutions did you explore? (Optional)Backend could handle this edge case if |
@aimane-chnaif Friendly bump ^^ on above proposal :) |
Still pending review of proposal |
@aimane-chnaif mind taking a look at the proposal here? |
Pending review of proposals |
Bumped in Slack |
@b4s36t4 can you please share link to discussion? |
@marcochavezf do we currently support Google / Apple sign in from anonymous user in backend? |
@aimane-chnaif #27198 I think there are few other issues (maybe 2-3) but this can do the work. |
Is there any difference between nrml login & with ThirdParty logins? |
We may need to hold this for #27198 |
@aimane-chnaif But why?
for this issue? or any other reason? |
Successful signin means app keeps working even after refresh app upon signin, doesn't it? |
Yes, also this requires backend changes as well as I mentioned backend is not sending all onyx data on login. Either we need to call ReconnectApp command or backend should solve this. |
@aimane-chnaif do you think this will be fixed by the issue you referenced above? |
Ok, understood. But I feel backend changes are not required here. I think excluding the particular command I request you to check the things again, I feel backend is nothing related to this issue. |
So google signin should work the same way normal login works on anonymous user |
Yes, it works same as normal signin by just doing the two changes. |
Normal signin didn't require those patches |
@srikarparsi will you have a chance to look into this this week? |
Pending review from @srikarparsi |
@srikarparsi mind taking a look at this and whether you think this is a backend issue? |
Bumped Srikar 1:1 |
Hi! Sorry, It's finals period at school and I've been meaning to look at this but don't have too much familiarity with Google SSO and have been a bit busy with tests. I spent some time looking through the code today though and also think that this is a backend fix. I'll check again tomorrow to make sure the backend change won't affect any other sign ins, mark this internal, and take it from there |
Hey @aimane-chnaif, do you think you could clarify why you feel like this proposal is a workaround? It looks like we're excluding regular sign in as well and I think a natural extension of that would be to remove GoogleSignIn and AppleSignIn as well. Also @b4s36t4, do you think you could clarify step 2 of your proposal. Why do we need to remove email from the parameters conditionally? |
If we send |
yes, this is the only thing to fix in frontend. Rest should be handled in backend same as normal login. As @b4s36t4 said, if we send bug.mov |
Still pending further discussion |
Same |
@srikarparsi is the next step here for you to work on the backend changes? |
Pending input from @srikarparsi |
Bump @srikarparsi mind confirming here if the next step for this one is your backend changes? |
Bumped Srikar 1:1 |
Hi! Yes, this would be the backend change I need to make. And on the frontend, we would need to exclude Google and Apple Sign in. I'll get started on this tomorrow and should have a PR by the end of the week. |
Great. TY! LMK if anything comes up you need help with as you work on the PR. |
@srikarparsi is the PR still pending here? |
Just retested since I saw some changes made to SSO and it looks like its working now. Closing this out but @joekaufmanexpensify if you notice anything not working please re-open and I can take a look. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
User is signed in to Expensify
Actual Result:
An error message is shown and user is still logged out
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.72-6
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Bug6208650_Recording__335a.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: