From 93af6116c2ffc55ea79074f4e67cc808f6602d09 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 13 Dec 2018 15:55:57 -0500 Subject: [PATCH 1/5] Add conditional so workspace invitation is only revokable if the user is active --- atst/domain/workspaces/workspaces.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/atst/domain/workspaces/workspaces.py b/atst/domain/workspaces/workspaces.py index d8960a3d..bade9e08 100644 --- a/atst/domain/workspaces/workspaces.py +++ b/atst/domain/workspaces/workspaces.py @@ -146,7 +146,10 @@ class Workspaces(object): @classmethod def can_revoke_access_for(cls, workspace, workspace_role): - return workspace_role.user != workspace.owner + return ( + workspace_role.user != workspace.owner + and workspace_role.status == WorkspaceRoleStatus.ACTIVE + ) @classmethod def revoke_access(cls, user, workspace_id, workspace_role_id): From 9597966bcd78243c56e56a9b1e364a6f6e28d7e5 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 14 Dec 2018 10:03:41 -0500 Subject: [PATCH 2/5] Update function to determine when invite can be resent and added lots of tests --- atst/models/invitation.py | 5 +- atst/models/workspace_role.py | 13 ++- tests/domain/test_workspaces.py | 10 +- tests/routes/workspaces/test_members.py | 124 ++++++++++++++++++++---- 4 files changed, 127 insertions(+), 25 deletions(-) diff --git a/atst/models/invitation.py b/atst/models/invitation.py index 0b7d6686..6d471734 100644 --- a/atst/models/invitation.py +++ b/atst/models/invitation.py @@ -77,7 +77,10 @@ class Invitation(Base, TimestampsMixin, AuditableMixin): @property def is_expired(self): - return datetime.datetime.now(self.expiration_time.tzinfo) > self.expiration_time + return ( + datetime.datetime.now(self.expiration_time.tzinfo) > self.expiration_time + and not self.status == Status.ACCEPTED + ) @property def workspace(self): diff --git a/atst/models/workspace_role.py b/atst/models/workspace_role.py index 692cafbb..7a01dce9 100644 --- a/atst/models/workspace_role.py +++ b/atst/models/workspace_role.py @@ -117,6 +117,10 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): def role_displayname(self): return self.role.display_name + @property + def is_active(self): + return self.status == Status.ACTIVE + @property def num_environment_roles(self): return ( @@ -147,8 +151,13 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def can_resend_invitation(self): - return self.latest_invitation and ( - self.latest_invitation.is_rejected or self.latest_invitation.is_expired + return not self.is_active and ( + self.latest_invitation + and ( + self.latest_invitation.is_rejected + or self.latest_invitation.is_expired + or self.latest_invitation.is_revoked + ) ) diff --git a/tests/domain/test_workspaces.py b/tests/domain/test_workspaces.py index 63b408d9..a039a19a 100644 --- a/tests/domain/test_workspaces.py +++ b/tests/domain/test_workspaces.py @@ -304,9 +304,11 @@ def test_can_create_workspaces_with_matching_names(): Workspaces.create(RequestFactory.create(), name=workspace_name) -def test_can_revoke_workspace_access(): +def test_can_revoke_workspace_access_for_active_member(): workspace = WorkspaceFactory.create() - workspace_role = WorkspaceRoleFactory.create(workspace=workspace) + workspace_role = WorkspaceRoleFactory.create( + workspace=workspace, status=WorkspaceRoleStatus.ACTIVE + ) Workspaces.revoke_access(workspace.owner, workspace.id, workspace_role.id) assert Workspaces.for_user(workspace_role.user) == [] @@ -314,7 +316,9 @@ def test_can_revoke_workspace_access(): def test_can_revoke_access(): workspace = WorkspaceFactory.create() owner_role = workspace.roles[0] - workspace_role = WorkspaceRoleFactory.create(workspace=workspace) + workspace_role = WorkspaceRoleFactory.create( + workspace=workspace, status=WorkspaceRoleStatus.ACTIVE + ) assert Workspaces.can_revoke_access_for(workspace, workspace_role) assert not Workspaces.can_revoke_access_for(workspace, owner_role) diff --git a/tests/routes/workspaces/test_members.py b/tests/routes/workspaces/test_members.py index 44a63879..2b58665c 100644 --- a/tests/routes/workspaces/test_members.py +++ b/tests/routes/workspaces/test_members.py @@ -1,12 +1,19 @@ from flask import url_for -from tests.factories import UserFactory, WorkspaceFactory, WorkspaceRoleFactory +from tests.factories import ( + UserFactory, + WorkspaceFactory, + WorkspaceRoleFactory, + InvitationFactory, +) from atst.domain.workspaces import Workspaces from atst.domain.workspace_roles import WorkspaceRoles from atst.domain.projects import Projects from atst.domain.environments import Environments from atst.domain.environment_roles import EnvironmentRoles from atst.queue import queue +from atst.models.workspace_role import Status as WorkspaceRoleStatus +from atst.models.invitation import Status as InvitationStatus def test_user_with_permission_has_add_member_link(client, user_session): @@ -174,10 +181,12 @@ def test_update_member_environment_role_with_no_data(client, user_session): assert EnvironmentRoles.get(user.id, env1_id).role == "developer" -def test_revoke_member_access(client, user_session): +def test_revoke_active_member_access(client, user_session): workspace = WorkspaceFactory.create() user = UserFactory.create() - member = WorkspaceRoles.add(user, workspace.id, "developer") + member = WorkspaceRoleFactory.create( + workspace=workspace, user=user, status=WorkspaceRoleStatus.ACTIVE + ) Projects.create( workspace.owner, workspace, @@ -195,22 +204,7 @@ def test_revoke_member_access(client, user_session): assert WorkspaceRoles.get_by_id(member.id).num_environment_roles == 0 -def test_shows_revoke_button(client, user_session): - workspace = WorkspaceFactory.create() - user = UserFactory.create() - member = WorkspaceRoleFactory.create(user=user, workspace=workspace) - user_session(workspace.owner) - response = client.get( - url_for( - "workspaces.view_member", - workspace_id=workspace.id, - member_id=member.user.id, - ) - ) - assert "Remove Workspace Access" in response.data.decode() - - -def test_does_not_show_revoke_button(client, user_session): +def test_does_not_show_any_buttons_if_owner(client, user_session): workspace = WorkspaceFactory.create() user_session(workspace.owner) response = client.get( @@ -221,3 +215,95 @@ def test_does_not_show_revoke_button(client, user_session): ) ) assert "Remove Workspace Access" not in response.data.decode() + assert "Resend Invitation" not in response.data.decode() + assert "Revoke Invitation" not in response.data.decode() + + +def test_only_shows_revoke_access_button_if_active(client, user_session): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + member = WorkspaceRoleFactory.create( + user=user, workspace=workspace, status=WorkspaceRoleStatus.ACTIVE + ) + InvitationFactory.create( + user=workspace.owner, + workspace_role=member, + email=member.user.email, + status=InvitationStatus.ACCEPTED, + ) + user_session(workspace.owner) + response = client.get( + url_for( + "workspaces.view_member", + workspace_id=workspace.id, + member_id=member.user.id, + ) + ) + assert "Remove Workspace Access" in response.data.decode() + assert "Revoke Invitation" not in response.data.decode() + assert "Resend Invitation" not in response.data.decode() + + +def test_only_shows_revoke_invite_button_if_pending(client, user_session): + workspace = WorkspaceFactory.create() + member = WorkspaceRoleFactory.create( + workspace=workspace, status=WorkspaceRoleStatus.PENDING + ) + InvitationFactory.create( + user=workspace.owner, workspace_role=member, email=member.user.email + ) + user_session(workspace.owner) + response = client.get( + url_for( + "workspaces.view_member", + workspace_id=workspace.id, + member_id=member.user.id, + ) + ) + assert "Revoke Invitation" in response.data.decode() + assert "Remove Workspace Access" not in response.data.decode() + assert "Resend Invitation" not in response.data.decode() + + +def test_only_shows_resend_button_if_expired(client, user_session): + workspace = WorkspaceFactory.create() + member = WorkspaceRoleFactory.create(workspace=workspace) + InvitationFactory.create( + user=workspace.owner, + workspace_role=member, + email=member.user.email, + status=InvitationStatus.REJECTED_EXPIRED, + ) + user_session(workspace.owner) + response = client.get( + url_for( + "workspaces.view_member", + workspace_id=workspace.id, + member_id=member.user.id, + ) + ) + assert "Resend Invitation" in response.data.decode() + assert "Revoke Invitation" not in response.data.decode() + assert "Remove Workspace Access" not in response.data.decode() + + +def test_only_shows_resend_button_if_revoked(client, user_session): + workspace = WorkspaceFactory.create() + member = WorkspaceRoleFactory.create(workspace=workspace) + InvitationFactory.create( + user=workspace.owner, + workspace_role=member, + email=member.user.email, + status=InvitationStatus.REVOKED, + ) + user_session(workspace.owner) + response = client.get( + url_for( + "workspaces.view_member", + workspace_id=workspace.id, + member_id=member.user.id, + ) + ) + assert "Resend Invitation" in response.data.decode() + assert "Remove Workspace Access" not in response.data.decode() + assert "Revoke Invitation" not in response.data.decode() From fa2cad3291781628cb4454632d783ff01a3eb62e Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Tue, 18 Dec 2018 10:38:26 -0500 Subject: [PATCH 3/5] Move logic about invite into Invitation model --- atst/models/invitation.py | 8 ++++++++ atst/models/workspace_role.py | 7 +------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/atst/models/invitation.py b/atst/models/invitation.py index 6d471734..f1fafebf 100644 --- a/atst/models/invitation.py +++ b/atst/models/invitation.py @@ -82,6 +82,14 @@ class Invitation(Base, TimestampsMixin, AuditableMixin): and not self.status == Status.ACCEPTED ) + @property + def is_inactive(self): + return self.is_expired or self.status in [ + Status.REJECTED_WRONG_USER, + Status.REJECTED_EXPIRED, + Status.REVOKED, + ] + @property def workspace(self): if self.workspace_role: diff --git a/atst/models/workspace_role.py b/atst/models/workspace_role.py index 7a01dce9..580dc07f 100644 --- a/atst/models/workspace_role.py +++ b/atst/models/workspace_role.py @@ -152,12 +152,7 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def can_resend_invitation(self): return not self.is_active and ( - self.latest_invitation - and ( - self.latest_invitation.is_rejected - or self.latest_invitation.is_expired - or self.latest_invitation.is_revoked - ) + self.latest_invitation and self.latest_invitation.is_inactive ) From 54a88a9b94af11c75446976d0cb6e7bfe7bac5d6 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Wed, 19 Dec 2018 09:59:38 -0500 Subject: [PATCH 4/5] Update test names for clarity --- tests/domain/test_workspaces.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/domain/test_workspaces.py b/tests/domain/test_workspaces.py index a039a19a..38d20661 100644 --- a/tests/domain/test_workspaces.py +++ b/tests/domain/test_workspaces.py @@ -304,7 +304,7 @@ def test_can_create_workspaces_with_matching_names(): Workspaces.create(RequestFactory.create(), name=workspace_name) -def test_can_revoke_workspace_access_for_active_member(): +def test_able_to_revoke_workspace_access_for_active_member(): workspace = WorkspaceFactory.create() workspace_role = WorkspaceRoleFactory.create( workspace=workspace, status=WorkspaceRoleStatus.ACTIVE @@ -324,7 +324,7 @@ def test_can_revoke_access(): assert not Workspaces.can_revoke_access_for(workspace, owner_role) -def test_cant_revoke_owner_workspace_access(): +def test_unable_to_revoke_owner_workspace_access(): workspace = WorkspaceFactory.create() owner_workspace_role = workspace.roles[0] From 3ff86a1a6e7179b5f06e2081c2b1d04595588143 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Wed, 19 Dec 2018 11:30:51 -0500 Subject: [PATCH 5/5] Refactor tests --- tests/routes/workspaces/test_members.py | 63 +++++++++++++------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/tests/routes/workspaces/test_members.py b/tests/routes/workspaces/test_members.py index 2b58665c..c93e85c6 100644 --- a/tests/routes/workspaces/test_members.py +++ b/tests/routes/workspaces/test_members.py @@ -16,6 +16,26 @@ from atst.models.workspace_role import Status as WorkspaceRoleStatus from atst.models.invitation import Status as InvitationStatus +def create_workspace_and_invite_user( + ws_role="developer", + ws_status=WorkspaceRoleStatus.PENDING, + invite_status=InvitationStatus.PENDING, +): + workspace = WorkspaceFactory.create() + if ws_role != "owner": + user = UserFactory.create() + member = WorkspaceRoleFactory.create( + user=user, workspace=workspace, status=ws_status + ) + InvitationFactory.create( + user=workspace.owner, + workspace_role=member, + email=member.user.email, + status=invite_status, + ) + return workspace + + def test_user_with_permission_has_add_member_link(client, user_session): workspace = WorkspaceFactory.create() user_session(workspace.owner) @@ -220,18 +240,11 @@ def test_does_not_show_any_buttons_if_owner(client, user_session): def test_only_shows_revoke_access_button_if_active(client, user_session): - workspace = WorkspaceFactory.create() - user = UserFactory.create() - member = WorkspaceRoleFactory.create( - user=user, workspace=workspace, status=WorkspaceRoleStatus.ACTIVE - ) - InvitationFactory.create( - user=workspace.owner, - workspace_role=member, - email=member.user.email, - status=InvitationStatus.ACCEPTED, + workspace = create_workspace_and_invite_user( + ws_status=WorkspaceRoleStatus.ACTIVE, invite_status=InvitationStatus.ACCEPTED ) user_session(workspace.owner) + member = workspace.members[1] response = client.get( url_for( "workspaces.view_member", @@ -245,14 +258,11 @@ def test_only_shows_revoke_access_button_if_active(client, user_session): def test_only_shows_revoke_invite_button_if_pending(client, user_session): - workspace = WorkspaceFactory.create() - member = WorkspaceRoleFactory.create( - workspace=workspace, status=WorkspaceRoleStatus.PENDING - ) - InvitationFactory.create( - user=workspace.owner, workspace_role=member, email=member.user.email + workspace = create_workspace_and_invite_user( + ws_status=WorkspaceRoleStatus.PENDING, invite_status=InvitationStatus.PENDING ) user_session(workspace.owner) + member = workspace.members[1] response = client.get( url_for( "workspaces.view_member", @@ -266,15 +276,12 @@ def test_only_shows_revoke_invite_button_if_pending(client, user_session): def test_only_shows_resend_button_if_expired(client, user_session): - workspace = WorkspaceFactory.create() - member = WorkspaceRoleFactory.create(workspace=workspace) - InvitationFactory.create( - user=workspace.owner, - workspace_role=member, - email=member.user.email, - status=InvitationStatus.REJECTED_EXPIRED, + workspace = create_workspace_and_invite_user( + ws_status=WorkspaceRoleStatus.PENDING, + invite_status=InvitationStatus.REJECTED_EXPIRED, ) user_session(workspace.owner) + member = workspace.members[1] response = client.get( url_for( "workspaces.view_member", @@ -288,15 +295,11 @@ def test_only_shows_resend_button_if_expired(client, user_session): def test_only_shows_resend_button_if_revoked(client, user_session): - workspace = WorkspaceFactory.create() - member = WorkspaceRoleFactory.create(workspace=workspace) - InvitationFactory.create( - user=workspace.owner, - workspace_role=member, - email=member.user.email, - status=InvitationStatus.REVOKED, + workspace = create_workspace_and_invite_user( + ws_status=WorkspaceRoleStatus.PENDING, invite_status=InvitationStatus.REVOKED ) user_session(workspace.owner) + member = workspace.members[1] response = client.get( url_for( "workspaces.view_member",