From c9deaeec7238366ed42c8502d10b835d9c87f6a7 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 23 Apr 2019 13:58:38 -0400 Subject: [PATCH 01/11] Add is_app_member() to user model --- atst/models/user.py | 3 +++ tests/models/test_user.py | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/atst/models/user.py b/atst/models/user.py index 83f34326..3f980453 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -80,6 +80,9 @@ class User( def displayname(self): return self.full_name + def is_app_member(self, application): + return self in application.users + def __repr__(self): return "".format( self.full_name, self.dod_id, self.email, self.has_portfolios, self.id diff --git a/tests/models/test_user.py b/tests/models/test_user.py index 2a3ae51e..438c53e6 100644 --- a/tests/models/test_user.py +++ b/tests/models/test_user.py @@ -26,6 +26,19 @@ def test_cannot_update_dod_id(session): session.commit() +def test_is_app_member(): + user = UserFactory.create() + app = ApplicationFactory.create() + ApplicationRoleFactory.create(user=user, application=app) + assert user.is_app_member(app) + + +def test_is_not_app_member(): + user = UserFactory.create() + app = ApplicationFactory.create() + assert not user.is_app_member(app) + + def test_deleted_application_roles_are_ignored(session): user = UserFactory.create() app = ApplicationFactory.create() From 3e0a332ffca4b76c07c8b01fe7c2758cfcc83911 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 23 Apr 2019 16:31:55 -0400 Subject: [PATCH 02/11] Default permission_sets to VIEW_APPLICATION on creating an application role --- atst/domain/application_roles.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/atst/domain/application_roles.py b/atst/domain/application_roles.py index 0177374a..2a5f7960 100644 --- a/atst/domain/application_roles.py +++ b/atst/domain/application_roles.py @@ -9,7 +9,9 @@ class ApplicationRoles(object): return PermissionSets.get_many(set_names) @classmethod - def create(cls, user, application, permission_set_names): + def create( + cls, user, application, permission_set_names=[PermissionSets.VIEW_APPLICATION] + ): application_role = ApplicationRole(user=user, application_id=application.id) application_role.permission_sets = ApplicationRoles._permission_sets_for_names( From 6822680bc831c60fc9d9b322a681e7c26acd4b73 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 23 Apr 2019 16:37:29 -0400 Subject: [PATCH 03/11] Ensure that a member is an application member before adding the user to an environment --- atst/domain/environment_roles.py | 11 +++---- atst/domain/environments.py | 35 ++++++++++++---------- tests/domain/test_environments.py | 10 +++++++ tests/models/test_portfolio_role.py | 2 ++ tests/routes/applications/test_settings.py | 3 ++ 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 99728467..a34aa233 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -7,11 +7,12 @@ from atst.models import EnvironmentRole class EnvironmentRoles(object): @classmethod def create(cls, user, environment, role): - env_role = EnvironmentRole(user=user, environment=environment, role=role) - if not user.cloud_id: - user.cloud_id = app.csp.cloud.create_user(user) - app.csp.cloud.create_role(env_role) - return env_role + if user.is_app_member(environment.application): + env_role = EnvironmentRole(user=user, environment=environment, role=role) + if not user.cloud_id: + user.cloud_id = app.csp.cloud.create_user(user) + app.csp.cloud.create_role(env_role) + return env_role @classmethod def get(cls, user_id, environment_id): diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 44eaad9a..51a29321 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -5,6 +5,7 @@ from atst.database import db from atst.models.environment import Environment from atst.models.environment_role import EnvironmentRole from atst.models.application import Application +from atst.domain.application_roles import ApplicationRoles from atst.domain.environment_roles import EnvironmentRoles from atst.domain.users import Users @@ -32,6 +33,7 @@ class Environments(object): @classmethod def add_member(cls, environment, user, role): + ApplicationRoles.create(user=user, application=environment.application) environment_user = EnvironmentRoles.create( user=user, environment=environment, role=role ) @@ -66,23 +68,24 @@ class Environments(object): def update_env_role(cls, environment, user, new_role): updated = False - if new_role is None: - updated = EnvironmentRoles.delete(user.id, environment.id) - else: - env_role = EnvironmentRoles.get(user.id, environment.id) - if env_role and env_role.role != new_role: - env_role.role = new_role - updated = True - db.session.add(env_role) - elif not env_role: - env_role = EnvironmentRoles.create( - user=user, environment=environment, role=new_role - ) - updated = True - db.session.add(env_role) + if user.is_app_member(environment.application): + if new_role is None: + updated = EnvironmentRoles.delete(user.id, environment.id) + else: + env_role = EnvironmentRoles.get(user.id, environment.id) + if env_role and env_role.role != new_role: + env_role.role = new_role + updated = True + db.session.add(env_role) + elif not env_role: + env_role = EnvironmentRoles.create( + user=user, environment=environment, role=new_role + ) + updated = True + db.session.add(env_role) - if updated: - db.session.commit() + if updated: + db.session.commit() return updated diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index af8bb2a4..b6d15ca1 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -11,6 +11,7 @@ from tests.factories import ( PortfolioFactory, EnvironmentFactory, EnvironmentRoleFactory, + ApplicationRoleFactory, ) @@ -24,6 +25,9 @@ def test_create_environments(): def test_update_env_role(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) new_role = CSPRole.TECHNICAL_READ.value + ApplicationRoleFactory.create( + user=env_role.user, application=env_role.environment.application + ) assert Environments.update_env_role(env_role.environment, env_role.user, new_role) assert env_role.role == new_role @@ -31,6 +35,9 @@ def test_update_env_role(): def test_update_env_role_no_access(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) + ApplicationRoleFactory.create( + user=env_role.user, application=env_role.environment.application + ) assert Environments.update_env_role(env_role.environment, env_role.user, None) assert not EnvironmentRoles.get(env_role.user.id, env_role.environment.id) @@ -48,6 +55,7 @@ def test_update_env_role_no_change(): def test_update_env_role_creates_cloud_id_for_new_member(session): user = UserFactory.create() env = EnvironmentFactory.create() + ApplicationRoleFactory.create(user=user, application=env.application) assert not user.cloud_id assert Environments.update_env_role(env, user, CSPRole.TECHNICAL_READ.value) assert EnvironmentRoles.get(user.id, env.id) @@ -65,6 +73,8 @@ def test_update_env_roles_by_environment(): env_role_3 = EnvironmentRoleFactory.create( environment=environment, role=CSPRole.TECHNICAL_READ.value ) + for user in [env_role_1.user, env_role_2.user, env_role_3.user]: + ApplicationRoleFactory.create(user=user, application=environment.application) team_roles = [ { diff --git a/tests/models/test_portfolio_role.py b/tests/models/test_portfolio_role.py index 4b5e6601..a0af25e5 100644 --- a/tests/models/test_portfolio_role.py +++ b/tests/models/test_portfolio_role.py @@ -17,6 +17,7 @@ from tests.factories import ( EnvironmentFactory, EnvironmentRoleFactory, ApplicationFactory, + ApplicationRoleFactory, PortfolioFactory, ) from atst.domain.portfolio_roles import PortfolioRoles @@ -116,6 +117,7 @@ def test_has_env_role_history(session): portfolio = PortfolioFactory.create(owner=owner) portfolio_role = PortfolioRoleFactory.create(portfolio=portfolio, user=user) application = ApplicationFactory.create(portfolio=portfolio) + ApplicationRoleFactory.create(user=user, application=application) environment = EnvironmentFactory.create( application=application, name="new environment!" ) diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 2b9ec47e..88e97c84 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -178,6 +178,9 @@ def test_update_team_env_roles(client, user_session): env_role_3 = EnvironmentRoleFactory.create( environment=environment, role=CSPRole.BASIC_ACCESS.value ) + for user in [env_role_1.user, env_role_2.user, env_role_3.user]: + ApplicationRoleFactory.create(user=user, application=application) + app_role = ApplicationRoleFactory.create(application=application) form_data = { "env_id": environment.id, From fec4687c02f6e0b69b3f9434331e4081ef947ad7 Mon Sep 17 00:00:00 2001 From: Montana Date: Wed, 24 Apr 2019 09:09:29 -0400 Subject: [PATCH 04/11] Check for member in application function, not user function --- atst/domain/environment_roles.py | 2 +- atst/domain/environments.py | 2 +- atst/models/application.py | 3 +++ atst/models/user.py | 3 --- tests/models/test_application.py | 14 ++++++++++++++ tests/models/test_user.py | 13 ------------- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index a34aa233..366b61b2 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -7,7 +7,7 @@ from atst.models import EnvironmentRole class EnvironmentRoles(object): @classmethod def create(cls, user, environment, role): - if user.is_app_member(environment.application): + if environment.application.has_member(user): env_role = EnvironmentRole(user=user, environment=environment, role=role) if not user.cloud_id: user.cloud_id = app.csp.cloud.create_user(user) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 51a29321..f604808f 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -68,7 +68,7 @@ class Environments(object): def update_env_role(cls, environment, user, new_role): updated = False - if user.is_app_member(environment.application): + if environment.application.has_member(user): if new_role is None: updated = EnvironmentRoles.delete(user.id, environment.id) else: diff --git a/atst/models/application.py b/atst/models/application.py index 8512c570..43c667c3 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -52,6 +52,9 @@ class Application( def displayname(self): return self.name + def has_member(self, user): + return user in self.users + def __repr__(self): # pragma: no cover return "".format( self.name, self.description, self.portfolio.name, self.id diff --git a/atst/models/user.py b/atst/models/user.py index 3f980453..83f34326 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -80,9 +80,6 @@ class User( def displayname(self): return self.full_name - def is_app_member(self, application): - return self in application.users - def __repr__(self): return "".format( self.full_name, self.dod_id, self.email, self.has_portfolios, self.id diff --git a/tests/models/test_application.py b/tests/models/test_application.py index d90e4cd6..ae73f4a6 100644 --- a/tests/models/test_application.py +++ b/tests/models/test_application.py @@ -4,6 +4,7 @@ from tests.factories import ( ApplicationFactory, ApplicationRoleFactory, EnvironmentFactory, + UserFactory, ) @@ -38,3 +39,16 @@ def test_audit_event_for_application_deletion(session): ) assert update_event.changed_state.get("deleted") assert update_event.changed_state["deleted"] == [False, True] + + +def test_has_app_member(): + user = UserFactory.create() + app = ApplicationFactory.create() + ApplicationRoleFactory.create(user=user, application=app) + assert app.has_member(user) + + +def test_does_not_have_app_member(): + user = UserFactory.create() + app = ApplicationFactory.create() + assert not app.has_member(user) diff --git a/tests/models/test_user.py b/tests/models/test_user.py index 438c53e6..2a3ae51e 100644 --- a/tests/models/test_user.py +++ b/tests/models/test_user.py @@ -26,19 +26,6 @@ def test_cannot_update_dod_id(session): session.commit() -def test_is_app_member(): - user = UserFactory.create() - app = ApplicationFactory.create() - ApplicationRoleFactory.create(user=user, application=app) - assert user.is_app_member(app) - - -def test_is_not_app_member(): - user = UserFactory.create() - app = ApplicationFactory.create() - assert not user.is_app_member(app) - - def test_deleted_application_roles_are_ignored(session): user = UserFactory.create() app = ApplicationFactory.create() From 19a09b792e4f7a649ce8acba4dec920e9acd7395 Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 29 Apr 2019 11:04:17 -0400 Subject: [PATCH 05/11] Use user.id to check if user is in application --- atst/domain/environment_roles.py | 2 +- atst/domain/environments.py | 2 +- atst/models/application.py | 6 +++--- atst/routes/applications/settings.py | 2 ++ tests/models/test_application.py | 4 ++-- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 366b61b2..dab6d8b8 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -7,7 +7,7 @@ from atst.models import EnvironmentRole class EnvironmentRoles(object): @classmethod def create(cls, user, environment, role): - if environment.application.has_member(user): + if environment.application.has_member(user.id): env_role = EnvironmentRole(user=user, environment=environment, role=role) if not user.cloud_id: user.cloud_id = app.csp.cloud.create_user(user) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index f604808f..76873a60 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -68,7 +68,7 @@ class Environments(object): def update_env_role(cls, environment, user, new_role): updated = False - if environment.application.has_member(user): + if environment.application.has_member(user.id): if new_role is None: updated = EnvironmentRoles.delete(user.id, environment.id) else: diff --git a/atst/models/application.py b/atst/models/application.py index 43c667c3..95c03a43 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -9,7 +9,6 @@ from atst.models.application_role import ( ApplicationRole, Status as ApplicationRoleStatuses, ) - from atst.database import db @@ -52,8 +51,9 @@ class Application( def displayname(self): return self.name - def has_member(self, user): - return user in self.users + def has_member(self, user_id): + user_ids = set(str(user.id) for user in self.users) + return str(user_id) in user_ids def __repr__(self): # pragma: no cover return "".format( diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 169af8c9..12194ecf 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -7,6 +7,8 @@ from atst.domain.applications import Applications from atst.forms.application import ApplicationForm from atst.forms.app_settings import EnvironmentRolesForm from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.domain.exceptions import UnauthorizedError + from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash diff --git a/tests/models/test_application.py b/tests/models/test_application.py index ae73f4a6..b576eb18 100644 --- a/tests/models/test_application.py +++ b/tests/models/test_application.py @@ -45,10 +45,10 @@ def test_has_app_member(): user = UserFactory.create() app = ApplicationFactory.create() ApplicationRoleFactory.create(user=user, application=app) - assert app.has_member(user) + assert app.has_member(user.id) def test_does_not_have_app_member(): user = UserFactory.create() app = ApplicationFactory.create() - assert not app.has_member(user) + assert not app.has_member(user.id) From 0736b229bfd81cb9ad2a01b7d92920b719c8c107 Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 29 Apr 2019 11:30:43 -0400 Subject: [PATCH 06/11] Do not use mutable kwarg to set default permission set on application role --- atst/domain/application_roles.py | 5 ++--- atst/domain/environments.py | 4 +++- atst/routes/applications/settings.py | 1 - tests/domain/test_application_roles.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/atst/domain/application_roles.py b/atst/domain/application_roles.py index 2a5f7960..f1aa36f7 100644 --- a/atst/domain/application_roles.py +++ b/atst/domain/application_roles.py @@ -6,12 +6,11 @@ from atst.domain.permission_sets import PermissionSets class ApplicationRoles(object): @classmethod def _permission_sets_for_names(cls, set_names): + set_names = set(set_names).union({PermissionSets.VIEW_APPLICATION}) return PermissionSets.get_many(set_names) @classmethod - def create( - cls, user, application, permission_set_names=[PermissionSets.VIEW_APPLICATION] - ): + def create(cls, user, application, permission_set_names): application_role = ApplicationRole(user=user, application_id=application.id) application_role.permission_sets = ApplicationRoles._permission_sets_for_names( diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 76873a60..0ad704a3 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -33,7 +33,9 @@ class Environments(object): @classmethod def add_member(cls, environment, user, role): - ApplicationRoles.create(user=user, application=environment.application) + ApplicationRoles.create( + user=user, application=environment.application, permission_set_names=[] + ) environment_user = EnvironmentRoles.create( user=user, environment=environment, role=role ) diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 12194ecf..8adc4a02 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -7,7 +7,6 @@ from atst.domain.applications import Applications from atst.forms.application import ApplicationForm from atst.forms.app_settings import EnvironmentRolesForm from atst.domain.authz.decorator import user_can_access_decorator as user_can -from atst.domain.exceptions import UnauthorizedError from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash diff --git a/tests/domain/test_application_roles.py b/tests/domain/test_application_roles.py index ceac10e1..bde4a1d5 100644 --- a/tests/domain/test_application_roles.py +++ b/tests/domain/test_application_roles.py @@ -14,7 +14,7 @@ def test_create_application_role(): ) assert application_role.permission_sets == PermissionSets.get_many( - [PermissionSets.EDIT_APPLICATION_TEAM] + [PermissionSets.EDIT_APPLICATION_TEAM, PermissionSets.VIEW_APPLICATION] ) assert application_role.application == application assert application_role.user == user From 60b4c50819385586c2d8cc8ac5e17025570ae004 Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 29 Apr 2019 14:07:17 -0400 Subject: [PATCH 07/11] Check that all users with changed data are app users before updating env roles --- atst/domain/environment_roles.py | 11 ++++---- atst/domain/environments.py | 33 +++++++++++----------- atst/routes/applications/settings.py | 21 ++++++++++++++ tests/routes/applications/test_settings.py | 30 ++++++++++++++++++++ tests/test_access.py | 4 ++- 5 files changed, 75 insertions(+), 24 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index dab6d8b8..99728467 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -7,12 +7,11 @@ from atst.models import EnvironmentRole class EnvironmentRoles(object): @classmethod def create(cls, user, environment, role): - if environment.application.has_member(user.id): - env_role = EnvironmentRole(user=user, environment=environment, role=role) - if not user.cloud_id: - user.cloud_id = app.csp.cloud.create_user(user) - app.csp.cloud.create_role(env_role) - return env_role + env_role = EnvironmentRole(user=user, environment=environment, role=role) + if not user.cloud_id: + user.cloud_id = app.csp.cloud.create_user(user) + app.csp.cloud.create_role(env_role) + return env_role @classmethod def get(cls, user_id, environment_id): diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 0ad704a3..aeba40d2 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -70,24 +70,23 @@ class Environments(object): def update_env_role(cls, environment, user, new_role): updated = False - if environment.application.has_member(user.id): - if new_role is None: - updated = EnvironmentRoles.delete(user.id, environment.id) - else: - env_role = EnvironmentRoles.get(user.id, environment.id) - if env_role and env_role.role != new_role: - env_role.role = new_role - updated = True - db.session.add(env_role) - elif not env_role: - env_role = EnvironmentRoles.create( - user=user, environment=environment, role=new_role - ) - updated = True - db.session.add(env_role) + if new_role is None: + updated = EnvironmentRoles.delete(user.id, environment.id) + else: + env_role = EnvironmentRoles.get(user.id, environment.id) + if env_role and env_role.role != new_role: + env_role.role = new_role + updated = True + db.session.add(env_role) + elif not env_role: + env_role = EnvironmentRoles.create( + user=user, environment=environment, role=new_role + ) + updated = True + db.session.add(env_role) - if updated: - db.session.commit() + if updated: + db.session.commit() return updated diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 8adc4a02..ad68a8d4 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -7,6 +7,7 @@ from atst.domain.applications import Applications from atst.forms.application import ApplicationForm from atst.forms.app_settings import EnvironmentRolesForm from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.domain.exceptions import NotFoundError from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash @@ -43,6 +44,13 @@ def serialize_env_member_form_data(application): return environments_list +def check_users_are_in_application(user_ids, application): + for user_id in user_ids: + if not application.has_member(user_id): + raise NotFoundError("application user {}".format(user_id)) + return True + + @applications_bp.route("/applications//settings") @user_can(Permissions.VIEW_APPLICATION, message="view application edit form") def settings(application_id): @@ -101,6 +109,19 @@ def update_env_roles(environment_id): env_roles_form = EnvironmentRolesForm(http_request.form) if env_roles_form.validate(): + + try: + user_ids = [user["user_id"] for user in env_roles_form.data["team_roles"]] + check_users_are_in_application(user_ids, application) + except NotFoundError as err: + app.logger.warning( + "Security violation for user {}, {} {}".format( + g.current_user.id, request.method, request.path + ), + extra={"tags": ["update", "failure"], "security_warning": True}, + ) + + raise (err) env_data = env_roles_form.data Environments.update_env_roles_by_environment( environment_id=environment_id, team_roles=env_data["team_roles"] diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 88e97c84..354fcbb6 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -1,3 +1,4 @@ +import pytest from flask import url_for, get_flashed_messages from tests.factories import ( @@ -9,12 +10,15 @@ from tests.factories import ( ApplicationFactory, ApplicationRoleFactory, ) +from atst.routes.applications.settings import check_users_are_in_application from atst.domain.applications import Applications from atst.domain.environment_roles import EnvironmentRoles from atst.domain.environments import Environments from atst.domain.permission_sets import PermissionSets from atst.domain.portfolios import Portfolios +from atst.domain.exceptions import NotFoundError + from atst.models.environment_role import CSPRole from atst.models.portfolio_role import Status as PortfolioRoleStatus @@ -166,6 +170,32 @@ def test_user_without_permission_cannot_update_application(client, user_session) assert application.description == "Cool stuff happening here!" +def test_check_users_are_in_application_raises_NotFoundError(): + application = ApplicationFactory.create() + app_user_1 = UserFactory.create() + app_user_2 = UserFactory.create() + for user in [app_user_1, app_user_2]: + ApplicationRoleFactory.create(user=user, application=application) + + non_app_user = UserFactory.create() + user_ids = [app_user_1.id, app_user_2.id, non_app_user.id] + with pytest.raises(NotFoundError): + check_users_are_in_application(user_ids, application) + + +def test_check_users_are_in_application(): + application = ApplicationFactory.create() + app_user_1 = UserFactory.create() + app_user_2 = UserFactory.create() + app_user_3 = UserFactory.create() + + for user in [app_user_1, app_user_2, app_user_3]: + ApplicationRoleFactory.create(user=user, application=application) + + user_ids = [app_user_1.id, app_user_2.id, app_user_3.id] + assert check_users_are_in_application(user_ids, application) + + def test_update_team_env_roles(client, user_session): environment = EnvironmentFactory.create() application = environment.application diff --git a/tests/test_access.py b/tests/test_access.py index 9839902c..2f47db0d 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -196,7 +196,9 @@ def test_applications_update_team_env_roles(post_url_assert_status): ] ), ) - ApplicationRoleFactory.create(user=app_member) + ApplicationRoleFactory.create(user=app_member, application=application) + ApplicationRoleFactory.create(user=ccpo, application=application) + ApplicationRoleFactory.create(user=owner, application=application) url = url_for("applications.update_env_roles", environment_id=environment.id) post_url_assert_status(ccpo, url, 302) From 1222220452d7ae70b78713929a44f395bf87ca8c Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 29 Apr 2019 14:08:54 -0400 Subject: [PATCH 08/11] Do not add member to application when adding to environment --- atst/domain/environments.py | 4 ---- tests/models/test_environments.py | 7 ++++++- tests/models/test_portfolio_role.py | 4 +++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index aeba40d2..44eaad9a 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -5,7 +5,6 @@ from atst.database import db from atst.models.environment import Environment from atst.models.environment_role import EnvironmentRole from atst.models.application import Application -from atst.domain.application_roles import ApplicationRoles from atst.domain.environment_roles import EnvironmentRoles from atst.domain.users import Users @@ -33,9 +32,6 @@ class Environments(object): @classmethod def add_member(cls, environment, user, role): - ApplicationRoles.create( - user=user, application=environment.application, permission_set_names=[] - ) environment_user = EnvironmentRoles.create( user=user, environment=environment, role=role ) diff --git a/tests/models/test_environments.py b/tests/models/test_environments.py index bccfdb94..a3c6deee 100644 --- a/tests/models/test_environments.py +++ b/tests/models/test_environments.py @@ -1,4 +1,5 @@ from atst.models import AuditEvent +from atst.models.environment_role import CSPRole from atst.domain.environments import Environments from atst.domain.applications import Applications @@ -7,6 +8,7 @@ from tests.factories import ( UserFactory, EnvironmentFactory, ApplicationFactory, + ApplicationRoleFactory, ) @@ -20,7 +22,10 @@ def test_add_user_to_environment(): ) dev_environment = application.environments[0] - dev_environment = Environments.add_member(dev_environment, developer, "developer") + ApplicationRoleFactory.create(user=developer, application=application) + dev_environment = Environments.add_member( + dev_environment, developer, CSPRole.BASIC_ACCESS.value + ) assert developer in dev_environment.users diff --git a/tests/models/test_portfolio_role.py b/tests/models/test_portfolio_role.py index a0af25e5..110df0eb 100644 --- a/tests/models/test_portfolio_role.py +++ b/tests/models/test_portfolio_role.py @@ -10,6 +10,7 @@ from atst.models.portfolio_role import Status from atst.models.invitation import Status as InvitationStatus from atst.models.audit_event import AuditEvent from atst.models.portfolio_role import Status as PortfolioRoleStatus +from atst.models.environment_role import CSPRole from tests.factories import ( UserFactory, InvitationFactory, @@ -180,8 +181,9 @@ def test_has_environment_roles(): application = Applications.create( portfolio, "my test application", "It's mine.", ["dev", "staging", "prod"] ) + ApplicationRoleFactory.create(user=portfolio_role.user, application=application) Environments.add_member( - application.environments[0], portfolio_role.user, "developer" + application.environments[0], portfolio_role.user, CSPRole.BASIC_ACCESS.value ) assert portfolio_role.has_environment_roles From 78a82013238fa5626e61eb2679cfa56a68431892 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 30 Apr 2019 10:11:10 -0400 Subject: [PATCH 09/11] Send resource id to NotFoundError as well as resource name. --- atst/domain/exceptions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atst/domain/exceptions.py b/atst/domain/exceptions.py index b141d0b3..8c2b0caf 100644 --- a/atst/domain/exceptions.py +++ b/atst/domain/exceptions.py @@ -1,6 +1,7 @@ class NotFoundError(Exception): - def __init__(self, resource_name): + def __init__(self, resource_name, resource_id=None): self.resource_name = resource_name + self.resource_id = resource_id @property def message(self): From a99c795319b27e09d63ddf5793ec5f7d9d160e52 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 30 Apr 2019 10:12:05 -0400 Subject: [PATCH 10/11] Update logger message for application user not found --- atst/routes/applications/settings.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index ad68a8d4..aa017fa2 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -47,7 +47,7 @@ def serialize_env_member_form_data(application): def check_users_are_in_application(user_ids, application): for user_id in user_ids: if not application.has_member(user_id): - raise NotFoundError("application user {}".format(user_id)) + raise NotFoundError("application user", user_id) return True @@ -115,8 +115,8 @@ def update_env_roles(environment_id): check_users_are_in_application(user_ids, application) except NotFoundError as err: app.logger.warning( - "Security violation for user {}, {} {}".format( - g.current_user.id, request.method, request.path + "User {} requested environment role change for unauthorized user {} in application {}.".format( + g.current_user.id, err.resource_id, application.id ), extra={"tags": ["update", "failure"], "security_warning": True}, ) From 94e3dc637ab2b46700a806c5ffc5733a0f1aaad1 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 30 Apr 2019 10:27:15 -0400 Subject: [PATCH 11/11] Remove has_member function from application model --- atst/models/application.py | 4 ---- atst/routes/applications/settings.py | 3 ++- tests/models/test_application.py | 13 ------------- tests/routes/applications/test_settings.py | 2 +- 4 files changed, 3 insertions(+), 19 deletions(-) diff --git a/atst/models/application.py b/atst/models/application.py index 95c03a43..9edb5166 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -51,10 +51,6 @@ class Application( def displayname(self): return self.name - def has_member(self, user_id): - user_ids = set(str(user.id) for user in self.users) - return str(user_id) in user_ids - def __repr__(self): # pragma: no cover return "".format( self.name, self.description, self.portfolio.name, self.id diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index aa017fa2..06f8d005 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -45,8 +45,9 @@ def serialize_env_member_form_data(application): def check_users_are_in_application(user_ids, application): + existing_ids = [str(role.user_id) for role in application.roles] for user_id in user_ids: - if not application.has_member(user_id): + if not user_id in existing_ids: raise NotFoundError("application user", user_id) return True diff --git a/tests/models/test_application.py b/tests/models/test_application.py index b576eb18..5c8fbb79 100644 --- a/tests/models/test_application.py +++ b/tests/models/test_application.py @@ -39,16 +39,3 @@ def test_audit_event_for_application_deletion(session): ) assert update_event.changed_state.get("deleted") assert update_event.changed_state["deleted"] == [False, True] - - -def test_has_app_member(): - user = UserFactory.create() - app = ApplicationFactory.create() - ApplicationRoleFactory.create(user=user, application=app) - assert app.has_member(user.id) - - -def test_does_not_have_app_member(): - user = UserFactory.create() - app = ApplicationFactory.create() - assert not app.has_member(user.id) diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 354fcbb6..932420be 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -192,7 +192,7 @@ def test_check_users_are_in_application(): for user in [app_user_1, app_user_2, app_user_3]: ApplicationRoleFactory.create(user=user, application=application) - user_ids = [app_user_1.id, app_user_2.id, app_user_3.id] + user_ids = [str(app_user_1.id), str(app_user_2.id), str(app_user_3.id)] assert check_users_are_in_application(user_ids, application)