From 129f5e3031cf47e89eb94c43e7db998302fb99e0 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 23 May 2019 15:58:54 -0400 Subject: [PATCH 1/2] Rewrite function that builds form data for app environment roles form. - Adds a property to ApplicationRole model so that it knows its related EnvironmentRole models. - Rewrite the form data builder in the routes file so that it loops the application members and their environment roles to build the data structure. --- atst/domain/environments.py | 9 --- atst/models/application_role.py | 20 +++++- atst/routes/applications/settings.py | 92 +++++++++++++-------------- tests/domain/test_environments.py | 30 --------- tests/models/test_application_role.py | 12 +++- 5 files changed, 76 insertions(+), 87 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 515c7557..fd66d61d 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -113,15 +113,6 @@ class Environments(object): environment=environment, user=member, new_role=new_role ) - @classmethod - def get_members_by_role(cls, env, role): - return ( - db.session.query(EnvironmentRole) - .filter(EnvironmentRole.environment_id == env.id) - .filter(EnvironmentRole.role == role) - .all() - ) - @classmethod def revoke_access(cls, environment, target_user): EnvironmentRoles.delete(environment.id, target_user.id) diff --git a/atst/models/application_role.py b/atst/models/application_role.py index 7b343cd8..0a5ab672 100644 --- a/atst/models/application_role.py +++ b/atst/models/application_role.py @@ -1,12 +1,14 @@ from enum import Enum from sqlalchemy import Index, ForeignKey, Column, Enum as SQLAEnum, Table from sqlalchemy.dialects.postgresql import UUID -from sqlalchemy.orm import relationship +from sqlalchemy.orm import object_session, 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 @@ -91,6 +93,22 @@ 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/routes/applications/settings.py b/atst/routes/applications/settings.py index a50e04ae..30bd9f20 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -30,54 +30,54 @@ def get_environments_obj_for_app(application): return environments_obj -def serialize_members(member_list, role): - serialized_list = [] - - for member in member_list: - serialized_list.append( - { - "user_id": str(member.user_id), - "user_name": member.user.full_name, - "role_name": role, - } - ) - - return serialized_list - - -def sort_env_users_by_role(env): - users_list = [] - no_access_users = env.application.users - env.users - no_access_list = [ - {"user_id": str(user.id), "user_name": user.full_name, "role_name": NO_ACCESS} - for user in no_access_users - ] - users_list.append({"role": NO_ACCESS, "members": no_access_list}) - - for role in CSPRole: - users_list.append( - { - "role": role.value, - "members": serialize_members( - Environments.get_members_by_role(env, role.value), role.value - ), - } - ) - - return users_list - - def data_for_app_env_roles_form(application): - data = {"envs": []} - for environment in application.environments: - data["envs"].append( - { - "env_id": environment.id, - "team_roles": sort_env_users_by_role(environment), - } - ) + csp_roles = [role.value for role in CSPRole] + csp_roles.insert(0, NO_ACCESS) + # dictionary for sorting application members by environments + # and roles within those environments + environments_dict = { + e.id: {role_name: [] for role_name in csp_roles} + for e in application.environments + } + for member in application.members: + env_ids = set(environments_dict.keys()) + for env_role in member.environment_roles: + role_members_list = environments_dict[env_role.environment_id][ + env_role.role + ] + role_members_list.append( + { + "user_id": str(member.user.id), + "user_name": member.user_name, + "role_name": env_role.role, + } + ) + env_ids.remove(env_role.environment_id) - return data + # any leftover environment IDs are ones the app member + # does not have access to + for env_id in env_ids: + role_members_list = environments_dict[env_id][NO_ACCESS] + role_members_list.append( + { + "user_id": str(member.user.id), + "user_name": member.user_name, + "role_name": NO_ACCESS, + } + ) + + # transform the data into the shape the form needs + nested_data = [ + { + "env_id": env_id, + "team_roles": [ + {"role": role, "members": members} for role, members in roles.items() + ], + } + for env_id, roles in environments_dict.items() + ] + + return {"envs": nested_data} def check_users_are_in_application(user_ids, application): diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 577c5b93..65190157 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -136,36 +136,6 @@ def test_update_env_roles_by_member(): assert not EnvironmentRoles.get(user.id, testing.id) -def test_get_members_by_role(db): - environment = EnvironmentFactory.create() - env_role_1 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value - ) - env_role_2 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.TECHNICAL_READ.value - ) - env_role_3 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.TECHNICAL_READ.value - ) - rando_env = EnvironmentFactory.create() - rando_env_role = EnvironmentRoleFactory.create( - environment=rando_env, role=CSPRole.BASIC_ACCESS.value - ) - - basic_access_members = Environments.get_members_by_role( - environment, CSPRole.BASIC_ACCESS.value - ) - technical_read_members = Environments.get_members_by_role( - environment, CSPRole.TECHNICAL_READ.value - ) - assert basic_access_members == [env_role_1] - assert rando_env_role not in basic_access_members - assert technical_read_members == [env_role_2, env_role_3] - assert ( - Environments.get_members_by_role(environment, CSPRole.BUSINESS_READ.value) == [] - ) - - def test_get_scoped_environments(db): developer = UserFactory.create() portfolio = PortfolioFactory.create( diff --git a/tests/models/test_application_role.py b/tests/models/test_application_role.py index 5bfc102c..40216758 100644 --- a/tests/models/test_application_role.py +++ b/tests/models/test_application_role.py @@ -1,7 +1,7 @@ from atst.domain.permission_sets import PermissionSets from atst.models.audit_event import AuditEvent -from tests.factories import PortfolioFactory, UserFactory +from tests.factories import * def test_has_application_role_history(session): @@ -38,3 +38,13 @@ def test_has_application_role_history(session): old_state, new_state = changed_event.changed_state["permission_sets"] assert old_state == [PermissionSets.VIEW_APPLICATION] assert new_state == [PermissionSets.EDIT_APPLICATION_TEAM] + + +def test_environment_roles(): + application = ApplicationFactory.create() + environment = EnvironmentFactory.create(application=application) + user = UserFactory.create() + application_role = ApplicationRoleFactory.create(application=application, user=user) + environment_role = EnvironmentRoleFactory.create(environment=environment, user=user) + + assert application_role.environment_roles == [environment_role] From 43ea9222184b3720d6718e397313e36b6cb58679 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 24 May 2019 12:05:44 -0400 Subject: [PATCH 2/2] Simplify environment role updates in app settings. Use ApplicationRole.id instead of User.id in forms. This eliminates the need for the function that checks whether a user is in a given application, because looking up the application role will raise an error if the user is not. --- atst/domain/application_roles.py | 2 +- atst/domain/environments.py | 6 +- atst/forms/app_settings.py | 2 +- atst/routes/applications/settings.py | 33 ++------- .../__tests__/edit_environment_role.test.js | 18 ++--- js/components/forms/edit_environment_role.js | 4 +- .../edit_environment_team_form.html | 12 ++-- tests/domain/test_environments.py | 17 +++-- tests/routes/applications/test_settings.py | 68 +++++++------------ 9 files changed, 64 insertions(+), 98 deletions(-) diff --git a/atst/domain/application_roles.py b/atst/domain/application_roles.py index 1d5366c8..177d49f0 100644 --- a/atst/domain/application_roles.py +++ b/atst/domain/application_roles.py @@ -57,7 +57,7 @@ class ApplicationRoles(object): .one() ) except NoResultFound: - raise NotFoundError("portfolio_role") + raise NotFoundError("application_role") @classmethod def update_permission_sets(cls, application_role, new_perm_sets_names): diff --git a/atst/domain/environments.py b/atst/domain/environments.py index fd66d61d..25f82522 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -6,7 +6,7 @@ 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.users import Users +from atst.domain.application_roles import ApplicationRoles from .exceptions import NotFoundError @@ -99,9 +99,9 @@ class Environments(object): for member in team_roles: new_role = member["role_name"] - user = Users.get(member["user_id"]) + app_role = ApplicationRoles.get_by_id(member["application_role_id"]) Environments.update_env_role( - environment=environment, user=user, new_role=new_role + environment=environment, user=app_role.user, new_role=new_role ) @classmethod diff --git a/atst/forms/app_settings.py b/atst/forms/app_settings.py index 19801a10..805c1556 100644 --- a/atst/forms/app_settings.py +++ b/atst/forms/app_settings.py @@ -6,7 +6,7 @@ from .data import ENV_ROLES, ENV_ROLE_NO_ACCESS as NO_ACCESS class MemberForm(FlaskForm): - user_id = HiddenField() + application_role_id = HiddenField() user_name = StringField() role_name = RadioField(choices=ENV_ROLES, default=NO_ACCESS) diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 30bd9f20..f7a0f96f 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -10,7 +10,6 @@ from atst.forms.application import ApplicationForm, EditEnvironmentForm from atst.forms.data import ENV_ROLE_NO_ACCESS as NO_ACCESS from atst.domain.authz.decorator import user_can_access_decorator as user_can from atst.models.environment_role import CSPRole -from atst.domain.exceptions import NotFoundError from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash @@ -47,7 +46,7 @@ def data_for_app_env_roles_form(application): ] role_members_list.append( { - "user_id": str(member.user.id), + "application_role_id": str(member.id), "user_name": member.user_name, "role_name": env_role.role, } @@ -60,7 +59,7 @@ def data_for_app_env_roles_form(application): role_members_list = environments_dict[env_id][NO_ACCESS] role_members_list.append( { - "user_id": str(member.user.id), + "application_role_id": str(member.id), "user_name": member.user_name, "role_name": NO_ACCESS, } @@ -80,14 +79,6 @@ def data_for_app_env_roles_form(application): return {"envs": nested_data} -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 user_id in existing_ids: - raise NotFoundError("application user", user_id) - return True - - def render_settings_page(application, **kwargs): environments_obj = get_environments_obj_for_app(application=application) members_form = AppEnvRolesForm(data=data_for_app_env_roles_form(application)) @@ -210,22 +201,10 @@ def update_env_roles(environment_id): if form.validate(): env_data = [] - try: - for env in form.envs.data: - if env["env_id"] == str(environment.id): - for role in env["team_roles"]: - user_ids = [user["user_id"] for user in role["members"]] - check_users_are_in_application(user_ids, application) - env_data = env_data + role["members"] - except NotFoundError as err: - app.logger.warning( - "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}, - ) - - raise (err) + for env in form.envs.data: + if env["env_id"] == str(environment.id): + for role in env["team_roles"]: + env_data = env_data + role["members"] Environments.update_env_roles_by_environment( environment_id=environment_id, team_roles=env_data diff --git a/js/components/__tests__/edit_environment_role.test.js b/js/components/__tests__/edit_environment_role.test.js index 8242acb4..763db4a5 100644 --- a/js/components/__tests__/edit_environment_role.test.js +++ b/js/components/__tests__/edit_environment_role.test.js @@ -9,13 +9,13 @@ describe('EditEnvironmentRole', () => { { role: NO_ACCESS, members: [ - { role_name: null, user_id: '123' }, - { role_name: null, user_id: '456' }, + { role_name: null, application_role_id: '123' }, + { role_name: null, application_role_id: '456' }, ], }, { role: 'Basic Access', - members: [{ role_name: 'Basic Access', user_id: '789' }], + members: [{ role_name: 'Basic Access', application_role_id: '789' }], }, { role: 'Network Admin', @@ -24,13 +24,15 @@ describe('EditEnvironmentRole', () => { { role: 'Business Read-only', members: [ - { role_name: 'Business Read-only', user_id: '012' }, - { role_name: 'Business Read-only', user_id: '345' }, + { role_name: 'Business Read-only', application_role_id: '012' }, + { role_name: 'Business Read-only', application_role_id: '345' }, ], }, { role: 'Technical Read-only', - members: [{ role_name: 'Technical Read-only', user_id: '678' }], + members: [ + { role_name: 'Technical Read-only', application_role_id: '678' }, + ], }, ] @@ -53,7 +55,7 @@ describe('EditEnvironmentRole', () => { expect(member_data).toEqual({ role_name: 'Technical Read-only', - user_id: '678', + application_role_id: '678', }) }) @@ -73,7 +75,7 @@ describe('EditEnvironmentRole', () => { }) expect(techRole.members.length).toEqual(1) - wrapper.vm.addUser({ user_id: '901' }, 'Technical Read-only') + wrapper.vm.addUser({ application_role_id: '901' }, 'Technical Read-only') expect(techRole.members.length).toEqual(2) }) diff --git a/js/components/forms/edit_environment_role.js b/js/components/forms/edit_environment_role.js index e9081c34..c823758f 100644 --- a/js/components/forms/edit_environment_role.js +++ b/js/components/forms/edit_environment_role.js @@ -57,7 +57,7 @@ export const EditEnvironmentRole = { getUserInfo: function(userId) { for (let role of this.roleCategories) { for (let member of role.members) { - if (member.user_id === userId) { + if (member.application_role_id === userId) { return member } } @@ -67,7 +67,7 @@ export const EditEnvironmentRole = { removeUser: function(userId) { for (let role of this.roleCategories) { role.members = role.members.filter(member => { - return member.user_id !== userId + return member.application_role_id !== userId }) if (!role.members) { role.members = [] diff --git a/templates/fragments/applications/edit_environment_team_form.html b/templates/fragments/applications/edit_environment_team_form.html index 393fa4f9..ca74234e 100644 --- a/templates/fragments/applications/edit_environment_team_form.html +++ b/templates/fragments/applications/edit_environment_team_form.html @@ -36,11 +36,11 @@ v-bind:class="{'unassigned': checkNoAccess(member.role_name)}"> - + {{ Icon('edit', classes="icon--medium") }}
+ v-bind:value='member.application_role_id'> diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 65190157..d50f6c24 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -67,28 +67,35 @@ def test_update_env_roles_by_environment(): 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 + ) 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 + ) 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) + app_role_3 = ApplicationRoleFactory.create( + user=env_role_3.user, application=environment.application + ) team_roles = [ { - "user_id": env_role_1.user.id, + "application_role_id": app_role_1.id, "user_name": env_role_1.user.full_name, "role_name": CSPRole.BUSINESS_READ.value, }, { - "user_id": env_role_2.user.id, + "application_role_id": app_role_2.id, "user_name": env_role_2.user.full_name, "role_name": CSPRole.NETWORK_ADMIN.value, }, { - "user_id": env_role_3.user.id, + "application_role_id": app_role_3.id, "user_name": env_role_3.user.full_name, "role_name": None, }, diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index ff0b8d34..a3bbca91 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -10,7 +10,6 @@ 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 @@ -140,15 +139,19 @@ def test_data_for_app_env_roles_form(app, client, user_session): {"env"}, ) env = application.environments[0] - app_role = ApplicationRoleFactory.create(application=application) + app_role0 = ApplicationRoleFactory.create(application=application) env_role1 = EnvironmentRoleFactory.create( environment=env, role=CSPRole.BASIC_ACCESS.value ) - ApplicationRoleFactory.create(application=application, user=env_role1.user) + app_role1 = ApplicationRoleFactory.create( + application=application, user=env_role1.user + ) env_role2 = EnvironmentRoleFactory.create( environment=env, role=CSPRole.NETWORK_ADMIN.value ) - ApplicationRoleFactory.create(application=application, user=env_role2.user) + app_role2 = ApplicationRoleFactory.create( + application=application, user=env_role2.user + ) user_session(portfolio.owner) @@ -171,8 +174,8 @@ def test_data_for_app_env_roles_form(app, client, user_session): "role": NO_ACCESS, "members": [ { - "user_id": str(app_role.user_id), - "user_name": app_role.user.full_name, + "application_role_id": str(app_role0.id), + "user_name": app_role0.user.full_name, "role_name": None, } ], @@ -181,7 +184,7 @@ def test_data_for_app_env_roles_form(app, client, user_session): "role": CSPRole.BASIC_ACCESS.value, "members": [ { - "user_id": str(env_role1.user_id), + "application_role_id": str(app_role1.id), "user_name": env_role1.user.full_name, "role_name": CSPRole.BASIC_ACCESS.value, } @@ -191,7 +194,7 @@ def test_data_for_app_env_roles_form(app, client, user_session): "role": CSPRole.NETWORK_ADMIN.value, "members": [ { - "user_id": str(env_role2.user_id), + "application_role_id": str(app_role2.id), "user_name": env_role2.user.full_name, "role_name": CSPRole.NETWORK_ADMIN.value, } @@ -260,57 +263,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 = [str(app_user_1.id), str(app_user_2.id), str(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 + app_role_1 = ApplicationRoleFactory.create(application=application) env_role_1 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value + environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_1.user ) + app_role_2 = ApplicationRoleFactory.create(application=application) env_role_2 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value + environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_2.user ) + app_role_3 = ApplicationRoleFactory.create(application=application) env_role_3 = EnvironmentRoleFactory.create( - environment=environment, role=CSPRole.BASIC_ACCESS.value + environment=environment, role=CSPRole.BASIC_ACCESS.value, user=app_role_3.user ) - 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) + app_role_4 = ApplicationRoleFactory.create(application=application) form_data = { "envs-0-env_id": environment.id, - "envs-0-team_roles-0-members-0-user_id": app_role.user.id, + "envs-0-team_roles-0-members-0-application_role_id": app_role_4.id, "envs-0-team_roles-0-members-0-role_name": CSPRole.TECHNICAL_READ.value, - "envs-0-team_roles-1-members-0-user_id": env_role_1.user.id, + "envs-0-team_roles-1-members-0-application_role_id": app_role_1.id, "envs-0-team_roles-1-members-0-role_name": CSPRole.NETWORK_ADMIN.value, - "envs-0-team_roles-1-members-1-user_id": env_role_2.user.id, + "envs-0-team_roles-1-members-1-application_role_id": app_role_2.id, "envs-0-team_roles-1-members-1-role_name": CSPRole.BASIC_ACCESS.value, - "envs-0-team_roles-1-members-2-user_id": env_role_3.user.id, + "envs-0-team_roles-1-members-2-application_role_id": app_role_3.id, "envs-0-team_roles-1-members-2-role_name": NO_ACCESS, } @@ -325,7 +303,7 @@ def test_update_team_env_roles(client, user_session): 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.user.id, environment.id) + assert EnvironmentRoles.get(app_role_4.user.id, environment.id) def test_user_can_only_access_apps_in_their_portfolio(client, user_session):