From a38c8fb6d3e9e60fac7d50611a968b220c3aa1e0 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Thu, 12 Dec 2024 11:07:21 +0100 Subject: [PATCH 1/9] fix(sign-in): Fix redirect correct org after accepting invite --- src/sentry/web/frontend/auth_login.py | 16 ++++ .../test_accept_organization_invite.py | 85 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/src/sentry/web/frontend/auth_login.py b/src/sentry/web/frontend/auth_login.py index 4fb8f4eb2fdbde..0ea5defd7e02a4 100644 --- a/src/sentry/web/frontend/auth_login.py +++ b/src/sentry/web/frontend/auth_login.py @@ -660,6 +660,22 @@ def handle_basic_auth(self, request: Request, **kwargs) -> HttpResponseBase: elif login_form.is_valid(): user = login_form.get_user() + # Check if the user is a member of the provided organization based on their email + membership = organization_service.check_membership_by_email( + email=user.email, organization_id=organization.id + ) + + # If the user is in a "pending invite acceptance" state and user_id is None, + # they are not yet fully associated with the organization. + # To ensure they are redirected to the correct organization after accepting the invite, + # they need to be added as a member. + if membership and membership.user_id is None and membership.is_pending: + organization_service.add_organization_member( + organization_id=organization.id, + default_org_role=organization.default_role, + user_id=user.id, + ) + self._handle_login(request, user, organization) metrics.incr( "login.attempt", instance="success", skip_internal=True, sample_rate=1.0 diff --git a/tests/acceptance/test_accept_organization_invite.py b/tests/acceptance/test_accept_organization_invite.py index 80bda384569d7e..b28ee98b4f4282 100644 --- a/tests/acceptance/test_accept_organization_invite.py +++ b/tests/acceptance/test_accept_organization_invite.py @@ -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 = "dummy@example.com" + 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 = "dummy@example.com" + 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}']") From 8917d0de94af23288e53dd5bfa7ca5c9c17b2572 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Thu, 12 Dec 2024 12:06:22 +0100 Subject: [PATCH 2/9] fix test --- tests/sentry/web/frontend/test_auth_organization_login.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/sentry/web/frontend/test_auth_organization_login.py b/tests/sentry/web/frontend/test_auth_organization_login.py index 9a43adb72152ad..c44eef74597ec1 100644 --- a/tests/sentry/web/frontend/test_auth_organization_login.py +++ b/tests/sentry/web/frontend/test_auth_organization_login.py @@ -882,7 +882,6 @@ def test_flow_as_authenticated_user_with_invite_joining(self): @with_feature({"organizations:create": False}) def test_basic_auth_flow_as_invited_user(self): user = self.create_user("foor@example.com") - self.create_member(organization=self.organization, email="foor@example.com") self.session["_next"] = reverse( "sentry-organization-settings", args=[self.organization.slug] @@ -898,7 +897,6 @@ def test_basic_auth_flow_as_invited_user(self): def test_basic_auth_flow_as_invited_user_not_single_org_mode(self): user = self.create_user("u2@example.com") - self.create_member(organization=self.organization, email="u2@example.com") resp = self.client.post( self.path, {"username": user, "password": "admin", "op": "login"}, follow=True ) From 2498903563c8d8f95cebafa0a9ce19db21a4d738 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Thu, 12 Dec 2024 13:14:16 +0100 Subject: [PATCH 3/9] update test name --- tests/sentry/web/frontend/test_auth_organization_login.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/sentry/web/frontend/test_auth_organization_login.py b/tests/sentry/web/frontend/test_auth_organization_login.py index c44eef74597ec1..2569be6af4454d 100644 --- a/tests/sentry/web/frontend/test_auth_organization_login.py +++ b/tests/sentry/web/frontend/test_auth_organization_login.py @@ -880,7 +880,7 @@ 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("foor@example.com") self.session["_next"] = reverse( @@ -891,11 +891,11 @@ 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("u2@example.com") resp = self.client.post( self.path, {"username": user, "password": "admin", "op": "login"}, follow=True From cb987bcdc6b92e7bf7499592f22f841724b9c885 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Fri, 13 Dec 2024 10:54:43 +0100 Subject: [PATCH 4/9] move code --- src/sentry/web/frontend/auth_login.py | 32 +++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/sentry/web/frontend/auth_login.py b/src/sentry/web/frontend/auth_login.py index 0ea5defd7e02a4..58f10c0448c86d 100644 --- a/src/sentry/web/frontend/auth_login.py +++ b/src/sentry/web/frontend/auth_login.py @@ -660,22 +660,6 @@ def handle_basic_auth(self, request: Request, **kwargs) -> HttpResponseBase: elif login_form.is_valid(): user = login_form.get_user() - # Check if the user is a member of the provided organization based on their email - membership = organization_service.check_membership_by_email( - email=user.email, organization_id=organization.id - ) - - # If the user is in a "pending invite acceptance" state and user_id is None, - # they are not yet fully associated with the organization. - # To ensure they are redirected to the correct organization after accepting the invite, - # they need to be added as a member. - if membership and membership.user_id is None and membership.is_pending: - organization_service.add_organization_member( - organization_id=organization.id, - default_org_role=organization.default_role, - user_id=user.id, - ) - self._handle_login(request, user, organization) metrics.incr( "login.attempt", instance="success", skip_internal=True, sample_rate=1.0 @@ -684,6 +668,22 @@ def handle_basic_auth(self, request: Request, **kwargs) -> HttpResponseBase: if not user.is_active: return self.redirect(reverse("sentry-reactivate-account")) if organization: + # Check if the user is a member of the provided organization based on their email + membership = organization_service.check_membership_by_email( + email=user.email, organization_id=organization.id + ) + + # If the user is in a "pending invite acceptance" state and user_id is None, + # they are not yet fully associated with the organization. + # To ensure they are redirected to the correct organization after accepting the invite, + # they need to be added as a member. + if membership and membership.user_id is None and membership.is_pending: + organization_service.add_organization_member( + organization_id=organization.id, + default_org_role=organization.default_role, + user_id=user.id, + ) + # Refresh the organization we fetched prior to login in order to check its login state. org_context = organization_service.get_organization_by_slug( user_id=request.user.id, From a662a8cd393ffd54e084739529d2ec319743f09e Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Fri, 13 Dec 2024 11:06:11 +0100 Subject: [PATCH 5/9] fix test --- tests/sentry/web/frontend/test_auth_organization_login.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/web/frontend/test_auth_organization_login.py b/tests/sentry/web/frontend/test_auth_organization_login.py index 2569be6af4454d..7ceac9bf6726d8 100644 --- a/tests/sentry/web/frontend/test_auth_organization_login.py +++ b/tests/sentry/web/frontend/test_auth_organization_login.py @@ -891,7 +891,7 @@ def test_basic_auth_flow_as_not_invited_user(self): resp = self.client.post( self.path, {"username": user, "password": "admin", "op": "login"}, follow=True ) - assert resp.redirect_chain == [("/auth/login/", 302), ("/organizations/foo/issues/", 302)] + assert resp.redirect_chain == [("/auth/login/", 302)] assert resp.status_code == 403 self.assertTemplateUsed(resp, "sentry/no-organization-access.html") From 7e3fb2332e81e1b14bfdea5d29203604733e0a10 Mon Sep 17 00:00:00 2001 From: Michelle Fu Date: Mon, 16 Dec 2024 11:39:14 -0800 Subject: [PATCH 6/9] test that the user sees the 2FA view after accepting invite with existing account --- .../test_accept_organization_invite.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/acceptance/test_accept_organization_invite.py b/tests/acceptance/test_accept_organization_invite.py index b28ee98b4f4282..9e9ab7284ed540 100644 --- a/tests/acceptance/test_accept_organization_invite.py +++ b/tests/acceptance/test_accept_organization_invite.py @@ -137,3 +137,43 @@ def test_not_authenticated_user_already_member_of_an_org_accept_invite_other_org # Check if the user is redirected to the correct organization self.browser.find_element(By.XPATH, f"//*[text() = '{self.org.name}']") + + @override_settings(SENTRY_SINGLE_ORGANIZATION=True) + def test_existing_user_invite_2fa_enforced_org(self): + """ + Test that a user who has an existing Sentry account can accept an invite to another organization + and is required to go through the 2FA configuration view. + """ + self.org.update(flags=F("flags").bitor(Organization.flags.require_2fa)) + # Setup: Create a second user and make them a member of an organization + email = "dummy@example.com" + 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], + ) + # 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.element_exists_by_test_id("2fa-warning") From 784649699af09d090c7c71ca33acc83f8b071aec Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Mon, 13 Jan 2025 10:17:08 +0100 Subject: [PATCH 7/9] users have to explicitly accept org invitation --- .../services/organization/model.py | 1 + .../services/organization/serial.py | 1 + src/sentry/web/frontend/auth_login.py | 18 +++++++++--------- .../test_accept_organization_invite.py | 15 +++++++++------ 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/sentry/organizations/services/organization/model.py b/src/sentry/organizations/services/organization/model.py index 418e714cb84a94..49626e113a4bd3 100644 --- a/src/sentry/organizations/services/organization/model.py +++ b/src/sentry/organizations/services/organization/model.py @@ -142,6 +142,7 @@ class RpcOrganizationMember(RpcOrganizationMemberSummary): token_expired: bool = False legacy_token: str = "" email: str = "" + invitation_link: str | None = None def get_audit_log_metadata(self, user_email: str | None = None) -> Mapping[str, Any]: from sentry.models.organizationmember import invite_status_names diff --git a/src/sentry/organizations/services/organization/serial.py b/src/sentry/organizations/services/organization/serial.py index c94bfbc4e0d132..a223920ea342ca 100644 --- a/src/sentry/organizations/services/organization/serial.py +++ b/src/sentry/organizations/services/organization/serial.py @@ -54,6 +54,7 @@ def serialize_member(member: OrganizationMember) -> RpcOrganizationMember: token_expired=member.token_expired, legacy_token=member.legacy_token, email=member.get_email(), + invitation_link=member.get_invite_link(), ) omts = OrganizationMemberTeam.objects.filter( diff --git a/src/sentry/web/frontend/auth_login.py b/src/sentry/web/frontend/auth_login.py index 173483b0ad0554..b76cb969dfced1 100644 --- a/src/sentry/web/frontend/auth_login.py +++ b/src/sentry/web/frontend/auth_login.py @@ -674,15 +674,15 @@ def handle_basic_auth(self, request: Request, **kwargs) -> HttpResponseBase: ) # If the user is in a "pending invite acceptance" state and user_id is None, - # they are not yet fully associated with the organization. - # To ensure they are redirected to the correct organization after accepting the invite, - # they need to be added as a member. - if membership and membership.user_id is None and membership.is_pending: - organization_service.add_organization_member( - organization_id=organization.id, - default_org_role=organization.default_role, - user_id=user.id, - ) + # they have to be redirected to the invitation page to explicitly accept the invite. + if ( + membership + and membership.user_id is None + and membership.is_pending + and membership.invitation_link + ): + accept_link_position = membership.invitation_link.find("/accept") + return self.redirect(membership.invitation_link[accept_link_position:]) # Refresh the organization we fetched prior to login in order to check its login state. org_context = organization_service.get_organization_by_slug( diff --git a/tests/acceptance/test_accept_organization_invite.py b/tests/acceptance/test_accept_organization_invite.py index 9e9ab7284ed540..fcaba231d04645 100644 --- a/tests/acceptance/test_accept_organization_invite.py +++ b/tests/acceptance/test_accept_organization_invite.py @@ -122,7 +122,7 @@ def test_not_authenticated_user_already_member_of_an_org_accept_invite_other_org 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 + # Choose to login with existing account self.browser.click('a[data-test-id="link-with-existing"]') self.browser.wait_until_not('[data-test-id="loading-indicator"]') @@ -131,12 +131,12 @@ def test_not_authenticated_user_already_member_of_an_org_accept_invite_other_org "document.addEventListener('invalid', function(e) { e.preventDefault(); }, true);" ) - # Login using existing credentials + # Login self._sign_in_user(email, password) - assert self.browser.wait_until('[aria-label="Create project"]') + self.browser.wait_until('[data-test-id="join-organization"]') - # Check if the user is redirected to the correct organization - self.browser.find_element(By.XPATH, f"//*[text() = '{self.org.name}']") + # Display the acceptance view for the invitation to join a new organization + assert self.browser.element_exists(f"[aria-label='Join the {self.org.slug} organization']") @override_settings(SENTRY_SINGLE_ORGANIZATION=True) def test_existing_user_invite_2fa_enforced_org(self): @@ -176,4 +176,7 @@ def test_existing_user_invite_2fa_enforced_org(self): # Login using existing credentials self._sign_in_user(email, password) - assert self.browser.element_exists_by_test_id("2fa-warning") + self.browser.wait_until('[data-test-id="2fa-warning"]') + + # Display the 2FA configuration view + assert self.browser.element_exists("[aria-label='Configure Two-Factor Auth']") From a8edc8e20a43780cfcc2c6cc139487308812135e Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Mon, 13 Jan 2025 10:30:04 +0100 Subject: [PATCH 8/9] update comment --- src/sentry/web/frontend/auth_login.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/web/frontend/auth_login.py b/src/sentry/web/frontend/auth_login.py index b76cb969dfced1..16d41505b8804d 100644 --- a/src/sentry/web/frontend/auth_login.py +++ b/src/sentry/web/frontend/auth_login.py @@ -673,8 +673,8 @@ def handle_basic_auth(self, request: Request, **kwargs) -> HttpResponseBase: email=user.email, organization_id=organization.id ) - # If the user is in a "pending invite acceptance" state and user_id is None, - # they have to be redirected to the invitation page to explicitly accept the invite. + # If the user is a member, the user_id is None, and they are in a "pending invite acceptance" state with a valid invitation link, + # we redirect them to the invitation page to explicitly accept the invite if ( membership and membership.user_id is None From 638f4816ce3a3fe60831c48abeb321d38f8ee728 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Mon, 13 Jan 2025 14:46:09 +0100 Subject: [PATCH 9/9] fix test --- tests/sentry/web/frontend/test_auth_organization_login.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/sentry/web/frontend/test_auth_organization_login.py b/tests/sentry/web/frontend/test_auth_organization_login.py index 7ceac9bf6726d8..9b280d926db1f0 100644 --- a/tests/sentry/web/frontend/test_auth_organization_login.py +++ b/tests/sentry/web/frontend/test_auth_organization_login.py @@ -991,7 +991,8 @@ def test_correct_redirect_as_2fa_user_single_org_invited(self): self.path, {"username": user, "password": "admin", "op": "login"}, follow=True ) - assert resp.redirect_chain == [("/auth/2fa/", 302)] + invitation_link = "/" + member.get_invite_link().split("/", 3)[-1] + assert resp.redirect_chain == [(invitation_link, 302)] def test_correct_redirect_as_2fa_user_invited(self): user = self.create_user("foor@example.com") @@ -1010,7 +1011,8 @@ def test_correct_redirect_as_2fa_user_invited(self): self.path, {"username": user, "password": "admin", "op": "login"}, follow=True ) - assert resp.redirect_chain == [("/auth/2fa/", 302)] + invitation_link = "/" + member.get_invite_link().split("/", 3)[-1] + assert resp.redirect_chain == [(invitation_link, 302)] @override_settings(SENTRY_SINGLE_ORGANIZATION=True) @with_feature({"organizations:create": False})