fix(sign-in): Fix redirect correct org after accepting invite #82005
Conversation
github-actions
bot
added
the
Scope: Backend
…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 87.54% 87.57% +0.02%
==========================================
Files 9409 9348 -61
Lines 537844 535689 -2155
Branches 21179 20746 -433
==========================================
- Hits 470874 469144 -1730
+ Misses 66622 66151 -471
- Partials 348 394 +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.
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.
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.
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.
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.
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
|
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
|
Since the 2FA test is passing, this looks okay to me! @markstory is there anything I might have missed? |
| membership | ||
| and membership.user_id is None | ||
| and membership.is_pending | ||
| and membership.invitation_link |
There was a problem hiding this comment.
Because this is a new attribute, it won't exist during deploys and will raise runtime errors. You could avoid that by using getattr(membership, 'invitation_link', None) or deploying the new RPC attribute first.
There was a problem hiding this comment.
I did not know that! I will use the first option. Thanks!!
| and membership.invitation_link | ||
| ): | ||
| accept_link_position = membership.invitation_link.find("/accept") | ||
| return self.redirect(membership.invitation_link[accept_link_position:]) |
There was a problem hiding this comment.
Why do you need to slice the URL?
There was a problem hiding this comment.
It doesn't work with the domain. For example: http://testserver/accept/13/2eef892424458f4b071e581e55d31698/ doesn't work."
There was a problem hiding this comment.
maybe because it has to contain the region? is there a function I can use?
There was a problem hiding this comment.
Instead of string slicing, we could use urlparse and extract the path + query from that structure.
There was a problem hiding this comment.
I applied this feedback #82005 (comment). would that work?
…edirected-to-correct-org
Co-authored-by: Simon Hellmayr <[email protected]>
…edirected-to-correct-org
priscilawebdev
deleted the
priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
branch
closes #43745
closes #67690