-
-
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
Open
priscilawebdev
wants to merge
21
commits into
master
Choose a base branch
from
priscila/fix/sign-in/after-accepting-not-redirected-to-correct-org
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+152
−6
Open
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
a38c8fb
fix(sign-in): Fix redirect correct org after accepting invite
priscilawebdev e3400ba
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev 8917d0d
fix test
priscilawebdev 01ae5c6
Merge branch 'priscila/fix/sign-in/after-accepting-not-redirected-to-…
priscilawebdev 2498903
update test name
priscilawebdev 1bb57c9
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev cb987bc
move code
priscilawebdev a662a8c
fix test
priscilawebdev 7e3fb23
test that the user sees the 2FA view after accepting invite with exis…
mifu67 3f809cd
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev c1e2f11
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev 0194109
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev 7278406
Merge branch 'priscila/fix/sign-in/after-accepting-not-redirected-to-…
priscilawebdev a2d2f4e
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev e103635
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev 7846496
users have to explicitly accept org invitation
priscilawebdev a6553f6
Merge branch 'priscila/fix/sign-in/after-accepting-not-redirected-to-…
priscilawebdev aa1ed41
Merge branch 'master' into priscila/fix/sign-in/after-accepting-not-r…
priscilawebdev a8edc8e
update comment
priscilawebdev a34916e
Merge branch 'priscila/fix/sign-in/after-accepting-not-redirected-to-…
priscilawebdev 638f481
fix test
priscilawebdev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
from django.db.models import F | ||
from django.test import override_settings | ||
from selenium.webdriver.common.by import By | ||
|
||
from sentry.models.authprovider import AuthProvider | ||
from sentry.models.organization import Organization | ||
|
@@ -23,6 +25,14 @@ def setUp(self): | |
teams=[self.team], | ||
) | ||
|
||
def _sign_in_user(self, email, password): | ||
""" | ||
Helper method to sign in a user with given email and password. | ||
""" | ||
self.browser.find_element(By.ID, "id_username").send_keys(email) | ||
self.browser.find_element(By.ID, "id_password").send_keys(password) | ||
self.browser.find_element(By.XPATH, "//button[contains(text(), 'Sign In')]").click() | ||
|
||
def test_invite_simple(self): | ||
self.login_as(self.user) | ||
self.browser.get(self.member.get_invite_link().split("/", 3)[-1]) | ||
|
@@ -52,3 +62,78 @@ def test_invite_sso_org(self): | |
self.browser.wait_until('[data-test-id="accept-invite"]') | ||
assert self.browser.element_exists_by_test_id("action-info-sso") | ||
assert self.browser.element_exists('[data-test-id="sso-login"]') | ||
|
||
@override_settings(SENTRY_SINGLE_ORGANIZATION=True) | ||
def test_authenticated_user_already_member_of_an_org_accept_invite_other_org(self): | ||
""" | ||
Test that an authenticated user already part of an organization can accept an invite to another organization. | ||
""" | ||
|
||
# Setup: Create a second user and make them a member of an organization | ||
email = "[email protected]" | ||
password = "dummy" | ||
user2 = self.create_user(email=email) | ||
user2.set_password(password) | ||
user2.save() | ||
self.create_organization(name="Second Org", owner=user2) | ||
|
||
# Action: Invite User2 to the first organization | ||
new_member = self.create_member( | ||
user=None, | ||
email=user2.email, | ||
organization=self.org, | ||
role="owner", | ||
teams=[self.team], | ||
) | ||
|
||
self.login_as(user2) | ||
|
||
# Simulate the user accessing the invite link | ||
self.browser.get(new_member.get_invite_link().split("/", 3)[-1]) | ||
self.browser.wait_until('[data-test-id="accept-invite"]') | ||
|
||
self.browser.click('button[data-test-id="join-organization"]') | ||
assert self.browser.wait_until('[aria-label="Create project"]') | ||
|
||
@override_settings(SENTRY_SINGLE_ORGANIZATION=True) | ||
def test_not_authenticated_user_already_member_of_an_org_accept_invite_other_org(self): | ||
""" | ||
Test that a not authenticated user already part of an organization can accept an invite to another organization. | ||
""" | ||
|
||
# Setup: Create a second user and make them a member of an organization | ||
email = "[email protected]" | ||
password = "dummy" | ||
user2 = self.create_user(email=email) | ||
user2.set_password(password) | ||
user2.save() | ||
self.create_organization(name="Second Org", owner=user2) | ||
|
||
# Action: Invite User2 to the first organization | ||
new_member = self.create_member( | ||
user=None, | ||
email=user2.email, | ||
organization=self.org, | ||
role="member", | ||
teams=[self.team], | ||
) | ||
|
||
# Simulate the user accessing the invite link | ||
self.browser.get(new_member.get_invite_link().split("/", 3)[-1]) | ||
self.browser.wait_until('[data-test-id="accept-invite"]') | ||
|
||
# Accept the invitation using the existing account | ||
self.browser.click('a[data-test-id="link-with-existing"]') | ||
self.browser.wait_until_not('[data-test-id="loading-indicator"]') | ||
|
||
# Handle form validation: Prevent default invalid event blocking | ||
self.browser.driver.execute_script( | ||
"document.addEventListener('invalid', function(e) { e.preventDefault(); }, true);" | ||
) | ||
|
||
# Login using existing credentials | ||
self._sign_in_user(email, password) | ||
assert self.browser.wait_until('[aria-label="Create project"]') | ||
|
||
# Check if the user is redirected to the correct organization | ||
self.browser.find_element(By.XPATH, f"//*[text() = '{self.org.name}']") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -880,9 +880,8 @@ def test_flow_as_authenticated_user_with_invite_joining(self): | |
|
||
@override_settings(SENTRY_SINGLE_ORGANIZATION=True) | ||
@with_feature({"organizations:create": False}) | ||
def test_basic_auth_flow_as_invited_user(self): | ||
def test_basic_auth_flow_as_not_invited_user(self): | ||
user = self.create_user("[email protected]") | ||
self.create_member(organization=self.organization, email="[email protected]") | ||
|
||
self.session["_next"] = reverse( | ||
"sentry-organization-settings", args=[self.organization.slug] | ||
|
@@ -892,13 +891,12 @@ def test_basic_auth_flow_as_invited_user(self): | |
resp = self.client.post( | ||
self.path, {"username": user, "password": "admin", "op": "login"}, follow=True | ||
) | ||
assert resp.redirect_chain == [("/auth/login/", 302)] | ||
assert resp.redirect_chain == [("/auth/login/", 302), ("/organizations/foo/issues/", 302)] | ||
assert resp.status_code == 403 | ||
self.assertTemplateUsed(resp, "sentry/no-organization-access.html") | ||
|
||
def test_basic_auth_flow_as_invited_user_not_single_org_mode(self): | ||
def test_basic_auth_flow_as_not_invited_user_not_single_org_mode(self): | ||
user = self.create_user("[email protected]") | ||
self.create_member(organization=self.organization, email="[email protected]") | ||
resp = self.client.post( | ||
self.path, {"username": user, "password": "admin", "op": "login"}, follow=True | ||
) | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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: