-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(sign-in): Fix redirect correct org after accepting invite #82005
base: master
Are you sure you want to change the base?
fix(sign-in): Fix redirect correct org after accepting invite #82005
Conversation
…edirected-to-correct-org
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82005 +/- ##
==========================================
+ Coverage 84.95% 87.57% +2.62%
==========================================
Files 9487 9420 -67
Lines 539524 535697 -3827
Branches 21177 20799 -378
==========================================
+ Hits 458357 469152 +10795
+ Misses 80726 66150 -14576
+ Partials 441 395 -46 |
…correct-org' of github.com:getsentry/sentry into priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
…edirected-to-correct-org
organization_id=organization.id, | ||
default_org_role=organization.default_role, | ||
user_id=user.id, |
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.
Doesn't this bypass the membership invite acceptance views? We would also be skipping important steps of that process like enforcing MFA and SSO.
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.
Doesn't this bypass the membership invite acceptance views?
No, it doesn't. If the user is not logged in and clicks on the invitation, they will first see the invitation view, where they can either "Create a new account" or "Login using an existing account."
Currently, after submitting the form to create a new account, the invitation is accepted, and the user is redirected to the organization associated with the invite. However, this doesn't happen when the user chooses to login using an existing account. This is what this PR is trying to fix
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 would also be skipping important steps of that process like enforcing MFA and SSO.
I’ve moved the code to what I believe is a more appropriate location, and I was wondering if we have existing tests for the use cases you mentioned. The only tests I could find are these, which are still passing.
Could you confirm if these cover the scenarios we need?
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.
I just pulled your branch and tested things out. I think that we do bypass some of the member invite acceptance views by taking the user directly to the org. What I think should happen instead is that after the user logs in, they are redirected back to the initial member invite screen, where they have an active session and can accept the invitation with their currently signed-in account.
We're planning to implement this new member invite flow as part of our upcoming refactor of the authentication system.
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 scenario I'm thinking of is:
- An existing sentry user is invited to a new org.
- The user visits the organization login page
- If the user logs in, they'll implicitly accept the membership invite (via these changes) and bypass SSO setup and MFA setup requirements to gain membership.
…edirected-to-correct-org
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
…edirected-to-correct-org
…edirected-to-correct-org
…correct-org' of github.com:getsentry/sentry into priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
…edirected-to-correct-org
…edirected-to-correct-org
…correct-org' of github.com:getsentry/sentry into priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
…edirected-to-correct-org
Hi @markstory and @mifu67 . Thanks for the valuable feedback! 🙏 I've updated the code to check if the user is not already a member of a specific organization and has an invitation link. If both conditions are met, they are redirected to the invitation page to explicitly accept the invite. Would this work until we get everything refactored? The tests are passing now |
…correct-org' of github.com:getsentry/sentry into priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
closes #43745
closes #67690