From df06d1b62fa47657c5389a53f7ae946371097593 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 29 May 2019 16:11:58 -0400 Subject: [PATCH] Use application_role_id on environment_roles. In the future, an `application_invitation1 will not refer to a `user` until someone accepts the invitation; they'll only reference an `application_role`. When a user is invited to an application, the inviter can specify the environments the invitee should have access to. For this to be possible, an `environment_role` should reference an `application_role`, because no `user` entity will be known at that time. In addition to updating all the models and domain methods necessary for this change, this commit deletes unused code and tests that were dependent on `environment_roles` having a `user_id` foreign key. --- ...7dca_environment_roles_has_relation_to_.py | 54 +++++++ atst/domain/applications.py | 13 +- atst/domain/csp/cloud.py | 2 +- atst/domain/environment_roles.py | 42 ++++-- atst/domain/environments.py | 38 ++--- atst/models/application_role.py | 20 +-- atst/models/environment.py | 2 +- atst/models/environment_role.py | 14 +- atst/models/portfolio_role.py | 32 ---- atst/routes/applications/__init__.py | 6 +- atst/routes/applications/team.py | 6 +- atst/routes/portfolios/members.py | 15 -- script/seed_sample.py | 6 +- tests/domain/test_applications.py | 13 +- tests/domain/test_audit_log.py | 4 +- tests/domain/test_environment_roles.py | 90 +++++++++-- tests/domain/test_environments.py | 140 ++++-------------- tests/domain/test_portfolios.py | 23 --- tests/factories.py | 23 +-- tests/models/test_application_role.py | 4 +- tests/models/test_environments.py | 18 +-- tests/models/test_portfolio_role.py | 72 ++------- tests/routes/applications/test_init.py | 33 +---- tests/routes/applications/test_settings.py | 45 +++--- tests/routes/applications/test_team.py | 15 +- tests/test_access.py | 18 +-- 26 files changed, 314 insertions(+), 434 deletions(-) create mode 100644 alembic/versions/d2390c547dca_environment_roles_has_relation_to_.py diff --git a/alembic/versions/d2390c547dca_environment_roles_has_relation_to_.py b/alembic/versions/d2390c547dca_environment_roles_has_relation_to_.py new file mode 100644 index 00000000..4c212f27 --- /dev/null +++ b/alembic/versions/d2390c547dca_environment_roles_has_relation_to_.py @@ -0,0 +1,54 @@ +"""environment_roles has relation to application_roles + +Revision ID: d2390c547dca +Revises: ab1167fc8260 +Create Date: 2019-05-29 12:34:45.928219 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = 'd2390c547dca' +down_revision = 'ab1167fc8260' +branch_labels = None +depends_on = None + + +def upgrade(): + conn = op.get_bind() + op.add_column('environment_roles', sa.Column('application_role_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.drop_index('environments_role_user_environment', table_name='environment_roles') + op.create_index('environments_role_user_environment', 'environment_roles', ['application_role_id', 'environment_id'], unique=True) + op.drop_constraint('environment_roles_user_id_fkey', 'environment_roles', type_='foreignkey') + op.create_foreign_key("environment_roles_application_roles_fkey", 'environment_roles', 'application_roles', ['application_role_id'], ['id']) + conn.execute(""" +UPDATE environment_roles er +SET application_role_id = application_roles.id +FROM environments, applications, application_roles +WHERE + environment_id = environments.id AND + applications.id = environments.application_id AND + application_roles.application_id = applications.id AND + er.user_id = application_roles.user_id; +""") + op.alter_column('environment_roles', "application_role_id", nullable=False) + op.drop_column('environment_roles', 'user_id') + + +def downgrade(): + conn = op.get_bind() + op.add_column('environment_roles', sa.Column('user_id', postgresql.UUID(), autoincrement=False, nullable=True)) + op.drop_constraint("environment_roles_application_roles_fkey", 'environment_roles', type_='foreignkey') + op.create_foreign_key('environment_roles_user_id_fkey', 'environment_roles', 'users', ['user_id'], ['id']) + op.drop_index('environments_role_user_environment', table_name='environment_roles') + op.create_index('environments_role_user_environment', 'environment_roles', ['user_id', 'environment_id'], unique=True) + conn.execute(""" +UPDATE environment_roles +SET user_id = application_roles.user_id +FROM application_roles +WHERE application_role_id = application_roles.id +""") + op.alter_column('environment_roles', "user_id", nullable=False) + op.drop_column('environment_roles', 'application_role_id') diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 7205a24a..968bbb91 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -98,7 +98,9 @@ class Applications(BaseDomainClass): role = env_role_data.get("role") if role: environment = Environments.get(env_role_data.get("environment_id")) - Environments.add_member(environment, user, env_role_data.get("role")) + Environments.add_member( + environment, application_role, env_role_data.get("role") + ) return application_role @@ -110,8 +112,11 @@ class Applications(BaseDomainClass): application_role.status = ApplicationRoleStatus.DISABLED application_role.deleted = True - db.session.add(application_role) - db.session.commit() for env in application.environments: - EnvironmentRoles.delete(user_id=user_id, environment_id=env.id) + EnvironmentRoles.delete( + application_role_id=application_role.id, environment_id=env.id + ) + + db.session.add(application_role) + db.session.commit() diff --git a/atst/domain/csp/cloud.py b/atst/domain/csp/cloud.py index c498a1fe..5fa322e2 100644 --- a/atst/domain/csp/cloud.py +++ b/atst/domain/csp/cloud.py @@ -75,7 +75,7 @@ class MockCloudProvider(CloudProviderInterface): def get_access_token(self, environment_role): # for now, just create a mock token using the user and environment # cloud IDs and the name of the role in the environment - user_id = environment_role.user.cloud_id or "" + user_id = environment_role.application_role.user.cloud_id or "" env_id = environment_role.environment.cloud_id or "" role_details = environment_role.role return "::".join([user_id, env_id, role_details]) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index f7ddda23..9ae57782 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -1,24 +1,27 @@ from flask import current_app as app from atst.database import db -from atst.models import EnvironmentRole, Application, Environment +from atst.models import EnvironmentRole, ApplicationRole 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) + def create(cls, application_role, environment, role): + env_role = EnvironmentRole( + application_role=application_role, environment=environment, role=role + ) + # TODO: move cloud_id behavior to invitation acceptance + # 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): + def get(cls, application_role_id, environment_id): existing_env_role = ( db.session.query(EnvironmentRole) .filter( - EnvironmentRole.user_id == user_id, + EnvironmentRole.application_role_id == application_role_id, EnvironmentRole.environment_id == environment_id, ) .one_or_none() @@ -26,8 +29,21 @@ class EnvironmentRoles(object): return existing_env_role @classmethod - def delete(cls, user_id, environment_id): - existing_env_role = EnvironmentRoles.get(user_id, environment_id) + def get_by_user_and_environment(cls, user_id, environment_id): + existing_env_role = ( + db.session.query(EnvironmentRole) + .join(ApplicationRole) + .filter( + ApplicationRole.user_id == user_id, + EnvironmentRole.environment_id == environment_id, + ) + .one_or_none() + ) + return existing_env_role + + @classmethod + def delete(cls, application_role_id, environment_id): + existing_env_role = EnvironmentRoles.get(application_role_id, environment_id) if existing_env_role: app.csp.cloud.delete_role(existing_env_role) db.session.delete(existing_env_role) @@ -37,14 +53,10 @@ class EnvironmentRoles(object): return False @classmethod - def get_for_application_and_user(cls, user_id, application_id): + def get_for_application_member(cls, application_role_id): return ( db.session.query(EnvironmentRole) - .join(Environment) - .join(Application, Environment.application_id == Application.id) - .filter(EnvironmentRole.user_id == user_id) - .filter(Application.id == application_id) - .filter(EnvironmentRole.environment_id == Environment.id) + .filter(EnvironmentRole.application_role_id == application_role_id) .filter(EnvironmentRole.deleted != True) .all() ) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 25f82522..3428f711 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -3,8 +3,6 @@ from sqlalchemy.orm.exc import NoResultFound 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.environment_roles import EnvironmentRoles from atst.domain.application_roles import ApplicationRoles @@ -31,24 +29,13 @@ class Environments(object): return environments @classmethod - def add_member(cls, environment, user, role): + def add_member(cls, environment, application_role, role): environment_user = EnvironmentRoles.create( - user=user, environment=environment, role=role + application_role=application_role, environment=environment, role=role ) db.session.add(environment_user) return environment - @classmethod - def for_user(cls, user, application): - return ( - db.session.query(Environment) - .join(EnvironmentRole) - .join(Application) - .filter(EnvironmentRole.user_id == user.id) - .filter(Environment.application_id == application.id) - .all() - ) - @classmethod def update(cls, environment, name=None): if name is not None: @@ -70,20 +57,22 @@ class Environments(object): return env @classmethod - def update_env_role(cls, environment, user, new_role): + def update_env_role(cls, environment, application_role, new_role): updated = False if new_role is None: - updated = EnvironmentRoles.delete(user.id, environment.id) + updated = EnvironmentRoles.delete(application_role.id, environment.id) else: - env_role = EnvironmentRoles.get(user.id, environment.id) + env_role = EnvironmentRoles.get(application_role.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 + application_role=application_role, + environment=environment, + role=new_role, ) updated = True db.session.add(env_role) @@ -101,16 +90,7 @@ class Environments(object): new_role = member["role_name"] app_role = ApplicationRoles.get_by_id(member["application_role_id"]) Environments.update_env_role( - environment=environment, user=app_role.user, new_role=new_role - ) - - @classmethod - def update_env_roles_by_member(cls, member, env_roles): - for env_roles in env_roles: - new_role = env_roles["role"] - environment = Environments.get(env_roles["id"]) - Environments.update_env_role( - environment=environment, user=member, new_role=new_role + environment=environment, application_role=app_role, new_role=new_role ) @classmethod diff --git a/atst/models/application_role.py b/atst/models/application_role.py index 0a5ab672..7b343cd8 100644 --- a/atst/models/application_role.py +++ b/atst/models/application_role.py @@ -1,14 +1,12 @@ from enum import Enum from sqlalchemy import Index, ForeignKey, Column, Enum as SQLAEnum, Table from sqlalchemy.dialects.postgresql import UUID -from sqlalchemy.orm import object_session, relationship +from sqlalchemy.orm import relationship from sqlalchemy.event import listen from atst.utils import first_or_none from atst.models import Base, mixins from atst.models.mixins.auditable import record_permission_sets_updates -from atst.models.environment import Environment -from atst.models.environment_role import EnvironmentRole from .types import Id @@ -93,22 +91,6 @@ class ApplicationRole( "portfolio": self.application.portfolio.name, } - @property - def environment_roles(self): - if getattr(self, "_environment_roles", None) is None: - roles = ( - object_session(self) - .query(EnvironmentRole) - .join(Environment, Environment.application_id == self.application_id) - .filter(EnvironmentRole.environment_id == Environment.id) - .filter(EnvironmentRole.user_id == self.user_id) - .all() - ) - - setattr(self, "_environment_roles", roles) - - return self._environment_roles - Index( "application_role_user_application", diff --git a/atst/models/environment.py b/atst/models/environment.py index e15373b4..bbd34615 100644 --- a/atst/models/environment.py +++ b/atst/models/environment.py @@ -21,7 +21,7 @@ class Environment( @property def users(self): - return {r.user for r in self.roles} + return {r.application_role.user for r in self.roles} @property def num_users(self): diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index 5ad67f6c..eaa7052b 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -26,12 +26,14 @@ class EnvironmentRole( role = Column(String()) - user_id = Column(UUID(as_uuid=True), ForeignKey("users.id"), nullable=False) - user = relationship("User", backref="environment_roles") + application_role_id = Column( + UUID(as_uuid=True), ForeignKey("application_roles.id"), nullable=False + ) + application_role = relationship("ApplicationRole", backref="environment_roles") def __repr__(self): return "".format( - self.role, self.user.full_name, self.environment.name, self.id + self.role, self.application_role.user_name, self.environment.name, self.id ) @property @@ -53,8 +55,8 @@ class EnvironmentRole( @property def event_details(self): return { - "updated_user_name": self.user.displayname, - "updated_user_id": str(self.user_id), + "updated_user_name": self.application_role.user_name, + "updated_application_role_id": str(self.application_role_id), "role": self.role, "environment": self.environment.displayname, "environment_id": str(self.environment_id), @@ -67,7 +69,7 @@ class EnvironmentRole( Index( "environments_role_user_environment", - EnvironmentRole.user_id, + EnvironmentRole.application_role_id, EnvironmentRole.environment_id, unique=True, ) diff --git a/atst/models/portfolio_role.py b/atst/models/portfolio_role.py index 2d26f636..639680eb 100644 --- a/atst/models/portfolio_role.py +++ b/atst/models/portfolio_role.py @@ -7,11 +7,7 @@ from sqlalchemy.event import listen from atst.models import Base, mixins from .types import Id -from atst.database import db from atst.utils import first_or_none -from atst.models.environment_role import EnvironmentRole -from atst.models.application import Application -from atst.models.environment import Environment from atst.models.mixins.auditable import record_permission_sets_updates @@ -126,34 +122,6 @@ class PortfolioRole( def is_active(self): return self.status == Status.ACTIVE - @property - def num_environment_roles(self): - return ( - db.session.query(EnvironmentRole) - .join(EnvironmentRole.environment) - .join(Environment.application) - .join(Application.portfolio) - .filter(Application.portfolio_id == self.portfolio_id) - .filter(EnvironmentRole.user_id == self.user_id) - .count() - ) - - @property - def environment_roles(self): - return ( - db.session.query(EnvironmentRole) - .join(EnvironmentRole.environment) - .join(Environment.application) - .join(Application.portfolio) - .filter(Application.portfolio_id == self.portfolio_id) - .filter(EnvironmentRole.user_id == self.user_id) - .all() - ) - - @property - def has_environment_roles(self): - return self.num_environment_roles > 0 - @property def can_resend_invitation(self): return not self.is_active and ( diff --git a/atst/routes/applications/__init__.py b/atst/routes/applications/__init__.py index aa2e2bb6..6ba4bda0 100644 --- a/atst/routes/applications/__init__.py +++ b/atst/routes/applications/__init__.py @@ -17,7 +17,7 @@ applications_bp.context_processor(portfolio_context_processor) def wrap_environment_role_lookup(user, environment_id=None, **kwargs): - env_role = EnvironmentRoles.get(user.id, environment_id) + env_role = EnvironmentRoles.get_by_user_and_environment(user.id, environment_id) if not env_role: raise UnauthorizedError(user, "access environment {}".format(environment_id)) @@ -27,7 +27,9 @@ def wrap_environment_role_lookup(user, environment_id=None, **kwargs): @applications_bp.route("/environments//access") @user_can(None, override=wrap_environment_role_lookup, message="access environment") def access_environment(environment_id): - env_role = EnvironmentRoles.get(g.current_user.id, environment_id) + env_role = EnvironmentRoles.get_by_user_and_environment( + g.current_user.id, environment_id + ) token = app.csp.cloud.get_access_token(env_role) return redirect(url_for("atst.csp_environment_access", token=token)) diff --git a/atst/routes/applications/team.py b/atst/routes/applications/team.py index 1b14d39f..b48c82a0 100644 --- a/atst/routes/applications/team.py +++ b/atst/routes/applications/team.py @@ -38,9 +38,7 @@ def get_team_form(application): member, PermissionSets.DELETE_APPLICATION_ENVIRONMENTS ), } - roles = EnvironmentRoles.get_for_application_and_user( - member.user.id, application.id - ) + roles = EnvironmentRoles.get_for_application_member(member.id) environment_roles = [ { "environment_id": str(role.environment.id), @@ -110,7 +108,7 @@ def update_team(application_id): environment_role_form.environment_id.data ) Environments.update_env_role( - environment, app_role.user, environment_role_form.data.get("role") + environment, app_role, environment_role_form.data.get("role") ) flash("updated_application_team_settings", application_name=application.name) diff --git a/atst/routes/portfolios/members.py b/atst/routes/portfolios/members.py index f25f0918..a823eac1 100644 --- a/atst/routes/portfolios/members.py +++ b/atst/routes/portfolios/members.py @@ -11,21 +11,6 @@ from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash -def serialize_portfolio_role(portfolio_role): - return { - "name": portfolio_role.user_name, - "status": portfolio_role.display_status, - "id": portfolio_role.user_id, - "role": "admin", - "num_env": portfolio_role.num_environment_roles, - "edit_link": url_for( - "portfolios.view_member", - portfolio_id=portfolio_role.portfolio_id, - member_id=portfolio_role.user_id, - ), - } - - @portfolios_bp.route("/portfolios//members/new", methods=["POST"]) @user_can(Permissions.CREATE_PORTFOLIO_USERS, message="create new portfolio member") def create_member(portfolio_id): diff --git a/script/seed_sample.py b/script/seed_sample.py index 247fa47c..1cbd1a51 100644 --- a/script/seed_sample.py +++ b/script/seed_sample.py @@ -225,7 +225,7 @@ def add_applications_to_portfolio(portfolio): last_name=user_data["last_name"], ) - ApplicationRoles.create( + app_role = ApplicationRoles.create( user=user, application=application, permission_set_names=[PermissionSets.EDIT_APPLICATION_TEAM], @@ -237,7 +237,9 @@ def add_applications_to_portfolio(portfolio): ) for env in user_environments: role = random.choice([e.value for e in CSPRole]) - EnvironmentRoles.create(user=user, environment=env, role=role) + EnvironmentRoles.create( + application_role=app_role, environment=env, role=role + ) def create_demo_portfolio(name, data): diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index ccd3c5e9..46da9bde 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -128,7 +128,7 @@ def test_create_member(): # view application AND edit application team assert len(member_role.permission_sets) == 2 - env_roles = member_role.user.environment_roles + env_roles = member_role.environment_roles assert len(env_roles) == 1 assert env_roles[0].environment == env1 @@ -165,7 +165,9 @@ def test_remove_member(): user = UserFactory.create() member_role = ApplicationRoleFactory.create(application=application, user=user) environment = EnvironmentFactory.create(application=application) - environment_role = EnvironmentRoleFactory.create(user=user, environment=environment) + environment_role = EnvironmentRoleFactory.create( + application_role=member_role, environment=environment + ) assert member_role == ApplicationRoles.get( user_id=user.id, application_id=application.id @@ -181,4 +183,9 @@ def test_remove_member(): # # TODO: Why does above raise NotFoundError and this returns None # - assert EnvironmentRoles.get(user_id=user.id, environment_id=environment.id) == None + assert ( + EnvironmentRoles.get( + application_role_id=member_role.id, environment_id=environment.id + ) + is None + ) diff --git a/tests/domain/test_audit_log.py b/tests/domain/test_audit_log.py index da74e7b0..a4a03f0b 100644 --- a/tests/domain/test_audit_log.py +++ b/tests/domain/test_audit_log.py @@ -84,7 +84,7 @@ def test_get_portfolio_events_includes_app_and_env_events(): # add environment level events env = EnvironmentFactory.create(application=application) - env_role = EnvironmentRoleFactory.create(environment=env, user=app_role.user) + env_role = EnvironmentRoleFactory.create(environment=env, application_role=app_role) portfolio_app_and_env_events = AuditLog.get_portfolio_events(portfolio) assert len(portfolio_and_app_events) < len(portfolio_app_and_env_events) @@ -106,7 +106,7 @@ def test_get_application_events(): app_role = ApplicationRoleFactory.create(application=application) app_invite = ApplicationInvitationFactory.create(role=app_role) env = EnvironmentFactory.create(application=application) - env_role = EnvironmentRoleFactory.create(environment=env, user=app_role.user) + env_role = EnvironmentRoleFactory.create(environment=env, application_role=app_role) # add rando app rando_app = ApplicationFactory.create(portfolio=portfolio) diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 83794852..b50d352d 100644 --- a/tests/domain/test_environment_roles.py +++ b/tests/domain/test_environment_roles.py @@ -1,26 +1,90 @@ +import pytest +from unittest.mock import MagicMock + from atst.domain.environment_roles import EnvironmentRoles from tests.factories import * -def test_get_for_application_and_user(): +@pytest.fixture +def application_role(): user = UserFactory.create() application = ApplicationFactory.create() - env1 = EnvironmentFactory.create(application=application) - EnvironmentFactory.create(application=application) - EnvironmentRoleFactory.create(user=user, environment=env1) + return ApplicationRoleFactory.create(application=application, user=user) - roles = EnvironmentRoles.get_for_application_and_user(user.id, application.id) + +@pytest.fixture +def environment(application_role): + return EnvironmentFactory.create(application=application_role.application) + + +def test_create(application_role, environment, monkeypatch): + mock_create_role = MagicMock() + monkeypatch.setattr( + "atst.domain.environment_roles.app.csp.cloud.create_role", mock_create_role + ) + + environment_role = EnvironmentRoles.create( + application_role, environment, "network admin" + ) + assert environment_role.application_role == application_role + assert environment_role.environment == environment + assert environment_role.role == "network admin" + mock_create_role.assert_called_with(environment_role) + + +def test_get(application_role, environment): + EnvironmentRoleFactory.create( + application_role=application_role, environment=environment + ) + + environment_role = EnvironmentRoles.get(application_role.id, environment.id) + assert environment_role + assert environment_role.application_role == application_role + assert environment_role.environment == environment + + +def test_get_by_user_and_environment(application_role, environment): + expected_role = EnvironmentRoleFactory.create( + application_role=application_role, environment=environment + ) + actual_role = EnvironmentRoles.get_by_user_and_environment( + application_role.user.id, environment.id + ) + assert expected_role == actual_role + + +def test_delete(application_role, environment, monkeypatch): + mock_delete_role = MagicMock() + monkeypatch.setattr( + "atst.domain.environment_roles.app.csp.cloud.delete_role", mock_delete_role + ) + + environment_role = EnvironmentRoleFactory.create( + application_role=application_role, environment=environment + ) + assert EnvironmentRoles.delete(application_role.id, environment.id) + mock_delete_role.assert_called_with(environment_role) + assert not EnvironmentRoles.delete(application_role.id, environment.id) + + +def test_get_for_application_member(application_role, environment): + EnvironmentRoleFactory.create( + application_role=application_role, environment=environment + ) + + roles = EnvironmentRoles.get_for_application_member(application_role.id) assert len(roles) == 1 - assert roles[0].environment == env1 - assert roles[0].user == user + assert roles[0].environment == environment + assert roles[0].application_role == application_role -def test_get_for_application_and_user_does_not_return_deleted(): - user = UserFactory.create() - application = ApplicationFactory.create() - env1 = EnvironmentFactory.create(application=application) - EnvironmentRoleFactory.create(user=user, environment=env1, deleted=True) +def test_get_for_application_member_does_not_return_deleted( + application_role, environment +): + EnvironmentRoleFactory.create( + application_role=application_role, environment=environment, deleted=True + ) - roles = EnvironmentRoles.get_for_application_and_user(user.id, application.id) + roles = EnvironmentRoles.get_for_application_member(application_role.id) assert len(roles) == 0 diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index d50f6c24..412aca96 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -25,22 +25,22 @@ 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 Environments.update_env_role( + env_role.environment, env_role.application_role, new_role + ) assert env_role.role == new_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) + assert Environments.update_env_role( + env_role.environment, env_role.application_role, None + ) + assert not EnvironmentRoles.get( + env_role.application_role.id, env_role.environment.id + ) def test_update_env_role_no_change(): @@ -48,55 +48,45 @@ def test_update_env_role_no_change(): new_role = CSPRole.BASIC_ACCESS.value assert not Environments.update_env_role( - env_role.environment, env_role.user, new_role + env_role.environment, env_role.application_role, new_role ) -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) - assert user.cloud_id is not None - - def test_update_env_roles_by_environment(): environment = EnvironmentFactory.create() + app_role_1 = ApplicationRoleFactory.create(application=environment.application) env_role_1 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value - ) - app_role_1 = ApplicationRoleFactory.create( - user=env_role_1.user, application=environment.application + application_role=app_role_1, + environment=environment, + role=CSPRole.BASIC_ACCESS.value, ) + app_role_2 = ApplicationRoleFactory.create(application=environment.application) env_role_2 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.NETWORK_ADMIN.value - ) - app_role_2 = ApplicationRoleFactory.create( - user=env_role_2.user, application=environment.application + application_role=app_role_2, + environment=environment, + role=CSPRole.NETWORK_ADMIN.value, ) + app_role_3 = ApplicationRoleFactory.create(application=environment.application) env_role_3 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.TECHNICAL_READ.value - ) - app_role_3 = ApplicationRoleFactory.create( - user=env_role_3.user, application=environment.application + application_role=app_role_3, + environment=environment, + role=CSPRole.TECHNICAL_READ.value, ) team_roles = [ { "application_role_id": app_role_1.id, - "user_name": env_role_1.user.full_name, + "user_name": app_role_1.user_name, "role_name": CSPRole.BUSINESS_READ.value, }, { "application_role_id": app_role_2.id, - "user_name": env_role_2.user.full_name, + "user_name": app_role_2.user_name, "role_name": CSPRole.NETWORK_ADMIN.value, }, { "application_role_id": app_role_3.id, - "user_name": env_role_3.user.full_name, + "user_name": app_role_3.user_name, "role_name": None, }, ] @@ -104,80 +94,7 @@ def test_update_env_roles_by_environment(): Environments.update_env_roles_by_environment(environment.id, team_roles) assert env_role_1.role == CSPRole.BUSINESS_READ.value assert env_role_2.role == CSPRole.NETWORK_ADMIN.value - assert not EnvironmentRoles.get(env_role_3.user.id, environment.id) - - -def test_update_env_roles_by_member(): - user = UserFactory.create() - application = ApplicationFactory.create( - environments=[ - { - "name": "dev", - "members": [{"user": user, "role_name": CSPRole.BUSINESS_READ.value}], - }, - { - "name": "staging", - "members": [{"user": user, "role_name": CSPRole.BUSINESS_READ.value}], - }, - {"name": "prod"}, - { - "name": "testing", - "members": [{"user": user, "role_name": CSPRole.BUSINESS_READ.value}], - }, - ] - ) - - dev, staging, prod, testing = application.environments - env_roles = [ - {"id": dev.id, "role": CSPRole.NETWORK_ADMIN.value}, - {"id": staging.id, "role": CSPRole.BUSINESS_READ.value}, - {"id": prod.id, "role": CSPRole.TECHNICAL_READ.value}, - {"id": testing.id, "role": None}, - ] - - Environments.update_env_roles_by_member(user, env_roles) - - assert EnvironmentRoles.get(user.id, dev.id).role == CSPRole.NETWORK_ADMIN.value - assert EnvironmentRoles.get(user.id, staging.id).role == CSPRole.BUSINESS_READ.value - assert EnvironmentRoles.get(user.id, prod.id).role == CSPRole.TECHNICAL_READ.value - assert not EnvironmentRoles.get(user.id, testing.id) - - -def test_get_scoped_environments(db): - developer = UserFactory.create() - portfolio = PortfolioFactory.create( - members=[{"user": developer, "role_name": "developer"}], - applications=[ - { - "name": "application1", - "environments": [ - { - "name": "application1 dev", - "members": [{"user": developer, "role_name": "developer"}], - }, - {"name": "application1 staging"}, - {"name": "application1 prod"}, - ], - }, - { - "name": "application2", - "environments": [ - {"name": "application2 dev"}, - { - "name": "application2 staging", - "members": [{"user": developer, "role_name": "developer"}], - }, - {"name": "application2 prod"}, - ], - }, - ], - ) - - application1_envs = Environments.for_user(developer, portfolio.applications[0]) - assert [env.name for env in application1_envs] == ["application1 dev"] - - application2_envs = Environments.for_user(developer, portfolio.applications[1]) - assert [env.name for env in application2_envs] == ["application2 staging"] + assert not EnvironmentRoles.get(app_role_3.id, environment.id) def test_get_excludes_deleted(): @@ -190,7 +107,10 @@ def test_get_excludes_deleted(): def test_delete_environment(session): env = EnvironmentFactory.create(application=ApplicationFactory.create()) - env_role = EnvironmentRoleFactory.create(user=UserFactory.create(), environment=env) + env_role = EnvironmentRoleFactory.create( + application_role=ApplicationRoleFactory.create(application=env.application), + environment=env, + ) assert not env.deleted assert not env_role.deleted Environments.delete(env) diff --git a/tests/domain/test_portfolios.py b/tests/domain/test_portfolios.py index bf6a415e..1d28b013 100644 --- a/tests/domain/test_portfolios.py +++ b/tests/domain/test_portfolios.py @@ -111,29 +111,6 @@ def test_scoped_portfolio_for_admin_missing_view_apps_perms(portfolio_owner, por assert len(scoped_portfolio.applications) == 0 -@pytest.mark.skip(reason="should be reworked pending application member changes") -def test_scoped_portfolio_only_returns_a_users_applications_and_environments( - portfolio, portfolio_owner -): - new_application = Applications.create( - portfolio, "My Application", "My application", ["dev", "staging", "prod"] - ) - Applications.create( - portfolio, "My Application 2", "My application 2", ["dev", "staging", "prod"] - ) - developer = UserFactory.create() - dev_environment = Environments.add_member( - new_application.environments[0], developer, "developer" - ) - - scoped_portfolio = Portfolios.get(developer, portfolio.id) - - # Should only return the application and environment in which the user has an - # environment role. - assert scoped_portfolio.applications == [new_application] - assert scoped_portfolio.applications[0].environments == [dev_environment] - - def test_scoped_portfolio_returns_all_applications_for_portfolio_admin( portfolio, portfolio_owner ): diff --git a/tests/factories.py b/tests/factories.py index d9de9c97..bb7625da 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -163,20 +163,6 @@ class ApplicationFactory(Base): with_environments = kwargs.pop("environments", []) application = super()._create(model_class, *args, **kwargs) - # need to create application roles for environment users - app_members_from_envs = set() - for env in with_environments: - with_members = env.get("members", []) - for member_data in with_members: - member = member_data.get("user", UserFactory.create()) - app_members_from_envs.add(member) - # set for environments in case we just created the - # user for the application - member_data["user"] = member - - for member in app_members_from_envs: - ApplicationRoleFactory.create(application=application, user=member) - environments = [ EnvironmentFactory.create(application=application, **e) for e in with_environments @@ -200,9 +186,14 @@ class EnvironmentFactory(Base): for member in with_members: user = member.get("user", UserFactory.create()) + application_role = ApplicationRoleFactory.create( + application=environment.application, user=user + ) role_name = member["role_name"] EnvironmentRoleFactory.create( - environment=environment, role=role_name, user=user + environment=environment, + role=role_name, + application_role=application_role, ) return environment @@ -234,7 +225,7 @@ class EnvironmentRoleFactory(Base): environment = factory.SubFactory(EnvironmentFactory) role = random.choice([e.value for e in CSPRole]) - user = factory.SubFactory(UserFactory) + application_role = factory.SubFactory(ApplicationRoleFactory) class PortfolioInvitationFactory(Base): diff --git a/tests/models/test_application_role.py b/tests/models/test_application_role.py index 40216758..35bb81c2 100644 --- a/tests/models/test_application_role.py +++ b/tests/models/test_application_role.py @@ -45,6 +45,8 @@ def test_environment_roles(): environment = EnvironmentFactory.create(application=application) user = UserFactory.create() application_role = ApplicationRoleFactory.create(application=application, user=user) - environment_role = EnvironmentRoleFactory.create(environment=environment, user=user) + environment_role = EnvironmentRoleFactory.create( + environment=environment, application_role=application_role + ) assert application_role.environment_roles == [environment_role] diff --git a/tests/models/test_environments.py b/tests/models/test_environments.py index a3c6deee..9bc1c8b5 100644 --- a/tests/models/test_environments.py +++ b/tests/models/test_environments.py @@ -3,13 +3,7 @@ from atst.models.environment_role import CSPRole from atst.domain.environments import Environments from atst.domain.applications import Applications -from tests.factories import ( - PortfolioFactory, - UserFactory, - EnvironmentFactory, - ApplicationFactory, - ApplicationRoleFactory, -) +from tests.factories import * def test_add_user_to_environment(): @@ -22,9 +16,13 @@ def test_add_user_to_environment(): ) dev_environment = application.environments[0] - ApplicationRoleFactory.create(user=developer, application=application) - dev_environment = Environments.add_member( - dev_environment, developer, CSPRole.BASIC_ACCESS.value + application_role = ApplicationRoleFactory.create( + user=developer, application=application + ) + EnvironmentRoleFactory.create( + application_role=application_role, + environment=dev_environment, + role=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 6438d2b1..1f586db3 100644 --- a/tests/models/test_portfolio_role.py +++ b/tests/models/test_portfolio_role.py @@ -8,16 +8,7 @@ from atst.domain.applications import Applications from atst.domain.permission_sets import PermissionSets from atst.models import AuditEvent, InvitationStatus, PortfolioRoleStatus, CSPRole -from tests.factories import ( - UserFactory, - PortfolioInvitationFactory, - PortfolioRoleFactory, - EnvironmentFactory, - EnvironmentRoleFactory, - ApplicationFactory, - ApplicationRoleFactory, - PortfolioFactory, -) +from tests.factories import * from atst.domain.portfolio_roles import PortfolioRoles @@ -97,8 +88,9 @@ def test_has_no_env_role_history(session): application=application, name="new environment!" ) + app_role = ApplicationRoleFactory.create(user=user, application=application) env_role = EnvironmentRoleFactory.create( - user=user, environment=environment, role="developer" + application_role=app_role, environment=environment, role="developer" ) create_event = ( session.query(AuditEvent) @@ -110,22 +102,24 @@ def test_has_no_env_role_history(session): def test_has_env_role_history(session): - owner = UserFactory.create() user = UserFactory.create() - portfolio = PortfolioFactory.create(owner=owner) - portfolio_role = PortfolioRoleFactory.create(portfolio=portfolio, user=user) - application = ApplicationFactory.create(portfolio=portfolio) - ApplicationRoleFactory.create(user=user, application=application) + application = ApplicationFactory.create() + app_role = ApplicationRoleFactory.create(user=user, application=application) environment = EnvironmentFactory.create( application=application, name="new environment!" ) env_role = EnvironmentRoleFactory.create( - user=user, environment=environment, role="developer" - ) - Environments.update_env_roles_by_member( - user, [{"role": "admin", "id": environment.id}] + application_role=app_role, environment=environment, role="developer" ) + session.add(env_role) + session.commit() + session.refresh(env_role) + + env_role.role = "admin" + session.add(env_role) + session.commit() + changed_events = ( session.query(AuditEvent) .filter(AuditEvent.resource_id == env_role.id, AuditEvent.action == "update") @@ -147,44 +141,6 @@ def test_event_details(): assert portfolio_role.event_details["updated_user_id"] == str(user.id) -def test_has_no_environment_roles(): - owner = UserFactory.create() - developer_data = { - "dod_id": "1234567890", - "first_name": "Test", - "last_name": "User", - "email": "test.user@mail.com", - "portfolio_role": "developer", - } - - portfolio = PortfolioFactory.create(owner=owner) - portfolio_role = Portfolios.create_member(portfolio, developer_data) - - assert not portfolio_role.has_environment_roles - - -def test_has_environment_roles(): - owner = UserFactory.create() - developer_data = { - "dod_id": "1234567890", - "first_name": "Test", - "last_name": "User", - "email": "test.user@mail.com", - "portfolio_role": "developer", - } - - portfolio = PortfolioFactory.create(owner=owner) - portfolio_role = Portfolios.create_member(portfolio, developer_data) - 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, CSPRole.BASIC_ACCESS.value - ) - assert portfolio_role.has_environment_roles - - def test_status_when_member_is_active(): portfolio_role = PortfolioRoleFactory.create(status=PortfolioRoleStatus.ACTIVE) assert portfolio_role.display_status == "Active" diff --git a/tests/routes/applications/test_init.py b/tests/routes/applications/test_init.py index 1e5857ec..b014bf27 100644 --- a/tests/routes/applications/test_init.py +++ b/tests/routes/applications/test_init.py @@ -1,33 +1,16 @@ from flask import url_for, get_flashed_messages -from tests.factories import ( - UserFactory, - PortfolioFactory, - PortfolioRoleFactory, - EnvironmentRoleFactory, - EnvironmentFactory, - ApplicationFactory, -) - -from atst.domain.applications import Applications -from atst.domain.portfolios import Portfolios -from atst.models.portfolio_role import Status as PortfolioRoleStatus - -from tests.utils import captured_templates - - -def create_environment(user): - portfolio = PortfolioFactory.create() - portfolio_role = PortfolioRoleFactory.create(portfolio=portfolio, user=user) - application = ApplicationFactory.create(portfolio=portfolio) - return EnvironmentFactory.create(application=application, name="new environment!") +from tests.factories import * def test_environment_access_with_env_role(client, user_session): user = UserFactory.create() - environment = create_environment(user) - env_role = EnvironmentRoleFactory.create( - user=user, environment=environment, role="developer" + environment = EnvironmentFactory.create() + app_role = ApplicationRoleFactory.create( + user=user, application=environment.application + ) + EnvironmentRoleFactory.create( + application_role=app_role, environment=environment, role="developer" ) user_session(user) response = client.get( @@ -39,7 +22,7 @@ def test_environment_access_with_env_role(client, user_session): def test_environment_access_with_no_role(client, user_session): user = UserFactory.create() - environment = create_environment(user) + environment = EnvironmentFactory.create() user_session(user) response = client.get( url_for("applications.access_environment", environment_id=environment.id) diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index a3bbca91..b22db934 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -98,15 +98,14 @@ def test_edit_application_environments_obj(app, client, user_session): {"env"}, ) env = application.environments[0] - app_role = ApplicationRoleFactory.create(application=application) + app_role1 = ApplicationRoleFactory.create(application=application) env_role1 = EnvironmentRoleFactory.create( - environment=env, role=CSPRole.BASIC_ACCESS.value + application_role=app_role1, environment=env, role=CSPRole.BASIC_ACCESS.value ) - ApplicationRoleFactory.create(application=application, user=env_role1.user) + app_role2 = ApplicationRoleFactory.create(application=application) env_role2 = EnvironmentRoleFactory.create( - environment=env, role=CSPRole.NETWORK_ADMIN.value + application_role=app_role2, environment=env, role=CSPRole.NETWORK_ADMIN.value ) - ApplicationRoleFactory.create(application=application, user=env_role2.user) user_session(portfolio.owner) @@ -125,7 +124,7 @@ def test_edit_application_environments_obj(app, client, user_session): assert isinstance(env_obj["edit_form"], EditEnvironmentForm) assert ( env_obj["members"].sort() - == [env_role1.user.full_name, env_role2.user.full_name].sort() + == [app_role1.user_name, app_role2.user_name].sort() ) assert isinstance(context["audit_events"], Paginator) @@ -140,17 +139,13 @@ def test_data_for_app_env_roles_form(app, client, user_session): ) env = application.environments[0] app_role0 = ApplicationRoleFactory.create(application=application) + app_role1 = ApplicationRoleFactory.create(application=application) env_role1 = EnvironmentRoleFactory.create( - environment=env, role=CSPRole.BASIC_ACCESS.value - ) - app_role1 = ApplicationRoleFactory.create( - application=application, user=env_role1.user + application_role=app_role1, environment=env, role=CSPRole.BASIC_ACCESS.value ) + app_role2 = ApplicationRoleFactory.create(application=application) env_role2 = EnvironmentRoleFactory.create( - environment=env, role=CSPRole.NETWORK_ADMIN.value - ) - app_role2 = ApplicationRoleFactory.create( - application=application, user=env_role2.user + application_role=app_role2, environment=env, role=CSPRole.NETWORK_ADMIN.value ) user_session(portfolio.owner) @@ -175,7 +170,7 @@ def test_data_for_app_env_roles_form(app, client, user_session): "members": [ { "application_role_id": str(app_role0.id), - "user_name": app_role0.user.full_name, + "user_name": app_role0.user_name, "role_name": None, } ], @@ -185,7 +180,7 @@ def test_data_for_app_env_roles_form(app, client, user_session): "members": [ { "application_role_id": str(app_role1.id), - "user_name": env_role1.user.full_name, + "user_name": app_role1.user_name, "role_name": CSPRole.BASIC_ACCESS.value, } ], @@ -195,7 +190,7 @@ def test_data_for_app_env_roles_form(app, client, user_session): "members": [ { "application_role_id": str(app_role2.id), - "user_name": env_role2.user.full_name, + "user_name": app_role2.user_name, "role_name": CSPRole.NETWORK_ADMIN.value, } ], @@ -268,15 +263,21 @@ def test_update_team_env_roles(client, user_session): application = environment.application app_role_1 = ApplicationRoleFactory.create(application=application) env_role_1 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_1.user + environment=environment, + role=CSPRole.BASIC_ACCESS.value, + application_role=app_role_1, ) app_role_2 = ApplicationRoleFactory.create(application=application) env_role_2 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_2.user + environment=environment, + role=CSPRole.BASIC_ACCESS.value, + application_role=app_role_2, ) app_role_3 = ApplicationRoleFactory.create(application=application) env_role_3 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_3.user + environment=environment, + role=CSPRole.BASIC_ACCESS.value, + application_role=app_role_3, ) app_role_4 = ApplicationRoleFactory.create(application=application) @@ -302,8 +303,8 @@ def test_update_team_env_roles(client, user_session): assert response.status_code == 200 assert env_role_1.role == CSPRole.NETWORK_ADMIN.value assert env_role_2.role == CSPRole.BASIC_ACCESS.value - assert not EnvironmentRoles.get(env_role_3.user.id, environment.id) - assert EnvironmentRoles.get(app_role_4.user.id, environment.id) + assert not EnvironmentRoles.get(app_role_3.id, environment.id) + assert EnvironmentRoles.get(app_role_4.id, environment.id) def test_user_can_only_access_apps_in_their_portfolio(client, user_session): diff --git a/tests/routes/applications/test_team.py b/tests/routes/applications/test_team.py index a90bbeae..fa725695 100644 --- a/tests/routes/applications/test_team.py +++ b/tests/routes/applications/test_team.py @@ -94,7 +94,9 @@ def test_update_team_environment_roles(client, user_session): ) environment = EnvironmentFactory.create(application=application) env_role = EnvironmentRoleFactory.create( - user=app_role.user, environment=environment, role=CSPRole.NETWORK_ADMIN.value + application_role=app_role, + environment=environment, + role=CSPRole.NETWORK_ADMIN.value, ) user_session(owner) response = client.post( @@ -121,7 +123,9 @@ def test_update_team_revoke_environment_access(client, user_session, db, session ) environment = EnvironmentFactory.create(application=application) env_role = EnvironmentRoleFactory.create( - user=app_role.user, environment=environment, role=CSPRole.BASIC_ACCESS.value + application_role=app_role, + environment=environment, + role=CSPRole.BASIC_ACCESS.value, ) user_session(owner) response = client.post( @@ -177,8 +181,11 @@ def test_create_member(client, user_session): assert response.location == expected_url assert len(user.application_roles) == 1 assert user.application_roles[0].application == application - assert len(user.environment_roles) == 1 - assert user.environment_roles[0].environment == env + environment_roles = [ + er for ar in user.application_roles for er in ar.environment_roles + ] + assert len(environment_roles) == 1 + assert environment_roles[0].environment == env def test_remove_member_success(client, user_session): diff --git a/tests/test_access.py b/tests/test_access.py index 2d7874a4..ce3de14f 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -10,18 +10,7 @@ from atst.domain.auth import UNPROTECTED_ROUTES as _NO_LOGIN_REQUIRED from atst.domain.permission_sets import PermissionSets from atst.models import CSPRole, PortfolioRoleStatus, ApplicationRoleStatus -from tests.factories import ( - AttachmentFactory, - ApplicationFactory, - ApplicationRoleFactory, - EnvironmentFactory, - EnvironmentRoleFactory, - PortfolioInvitationFactory, - PortfolioFactory, - PortfolioRoleFactory, - TaskOrderFactory, - UserFactory, -) +from tests.factories import * _NO_ACCESS_CHECK_REQUIRED = _NO_LOGIN_REQUIRED + [ "task_orders.get_started", # all users can start a new TO @@ -265,11 +254,6 @@ def test_application_settings_access(get_url_assert_status): applications=[{"name": "Mos Eisley", "description": "Where Han shot first"}], ) app = portfolio.applications[0] - env = EnvironmentFactory.create(application=app) - env_role = EnvironmentRoleFactory.create( - environment=env, role=CSPRole.NETWORK_ADMIN.value - ) - ApplicationRoleFactory.create(application=app, user=env_role.user) url = url_for("applications.settings", application_id=app.id) get_url_assert_status(ccpo, url, 200)