diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index f2ed2988..add92091 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -29,6 +29,7 @@ class EnvironmentRoles(object): EnvironmentRole.application_role_id == application_role_id, EnvironmentRole.environment_id == environment_id, EnvironmentRole.deleted == False, + EnvironmentRole.status != EnvironmentRole.Status.DISABLED, ) .one_or_none() ) @@ -57,6 +58,14 @@ class EnvironmentRoles(object): ) return existing_env_role + @classmethod + def _update_status(cls, environment_role, new_status): + environment_role.status = new_status + db.session.add(environment_role) + db.session.commit() + + return environment_role + @classmethod def delete(cls, application_role_id, environment_id): existing_env_role = EnvironmentRoles.get(application_role_id, environment_id) @@ -104,3 +113,15 @@ class EnvironmentRoles(object): db.session.commit() return environment_role + + @classmethod + def get_for_update(cls, application_role_id, environment_id): + existing_env_role = ( + db.session.query(EnvironmentRole) + .filter( + EnvironmentRole.application_role_id == application_role_id, + EnvironmentRole.environment_id == environment_id, + ) + .one_or_none() + ) + return existing_env_role diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 1526f6dc..2114ce21 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -4,10 +4,17 @@ from typing import List from uuid import UUID from atst.database import db -from atst.models import Environment, Application, Portfolio, TaskOrder, CLIN +from atst.models import ( + Environment, + Application, + Portfolio, + TaskOrder, + CLIN, + EnvironmentRole, +) from atst.domain.environment_roles import EnvironmentRoles -from .exceptions import NotFoundError +from .exceptions import NotFoundError, DisabledError class Environments(object): @@ -50,29 +57,31 @@ class Environments(object): @classmethod def update_env_role(cls, environment, application_role, new_role): - updated = False + env_role = EnvironmentRoles.get_for_update(application_role.id, environment.id) + if env_role and ( + env_role.status == EnvironmentRole.Status.DISABLED or env_role.deleted + ): + raise DisabledError("environment_role", env_role.id) - if new_role is None: - updated = EnvironmentRoles.delete(application_role.id, environment.id) - else: - 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( - application_role=application_role, - environment=environment, - role=new_role, - ) - updated = True - db.session.add(env_role) + if ( + env_role + and env_role.role != new_role + and env_role.status != EnvironmentRole.Status.DISABLED + ): + env_role.role = new_role + db.session.add(env_role) + elif not env_role and new_role: + env_role = EnvironmentRoles.create( + application_role=application_role, + environment=environment, + role=new_role, + ) + db.session.add(env_role) - if updated: - db.session.commit() + if env_role and not new_role: + EnvironmentRoles.disable(env_role.id) - return updated + db.session.commit() @classmethod def revoke_access(cls, environment, target_user): diff --git a/atst/domain/exceptions.py b/atst/domain/exceptions.py index a89aa986..ab60ad61 100644 --- a/atst/domain/exceptions.py +++ b/atst/domain/exceptions.py @@ -53,3 +53,13 @@ class ClaimFailedException(Exception): f"Could not acquire claim for {resource.__class__.__name__} {resource.id}." ) super().__init__(message) + + +class DisabledError(Exception): + def __init__(self, resource_name, resource_id=None): + self.resource_name = resource_name + self.resource_id = resource_id + + @property + def message(self): + return f"Cannot update disabled {self.resource_name} {self.resource_id}." diff --git a/atst/forms/application_member.py b/atst/forms/application_member.py index ce2b6edd..ec873f77 100644 --- a/atst/forms/application_member.py +++ b/atst/forms/application_member.py @@ -16,8 +16,9 @@ class EnvironmentForm(Form): environment_name, choices=ENV_ROLES, default=NO_ACCESS, - filters=[lambda x: None if x == "None" else x], + filters=[lambda x: NO_ACCESS if x == "None" else x], ) + disabled = BooleanField("Revoke Access", default=False) @property def data(self): diff --git a/atst/models/application_role.py b/atst/models/application_role.py index f0e20be1..9468896c 100644 --- a/atst/models/application_role.py +++ b/atst/models/application_role.py @@ -1,5 +1,5 @@ from enum import Enum -from sqlalchemy import and_, Index, ForeignKey, Column, Enum as SQLAEnum, Table +from sqlalchemy import Index, ForeignKey, Column, Enum as SQLAEnum, Table from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.orm import relationship from sqlalchemy.event import listen @@ -8,7 +8,6 @@ from atst.utils import first_or_none from atst.models.base import Base import atst.models.mixins as mixins import atst.models.types as types -from atst.models.environment_role import EnvironmentRole from atst.models.mixins.auditable import record_permission_sets_updates @@ -55,9 +54,7 @@ class ApplicationRole( environment_roles = relationship( "EnvironmentRole", - primaryjoin=and_( - EnvironmentRole.application_role_id == id, EnvironmentRole.deleted == False - ), + primaryjoin="and_(EnvironmentRole.application_role_id == ApplicationRole.id, EnvironmentRole.deleted == False)", ) @property diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 05d906ab..16d227ed 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -14,6 +14,7 @@ from atst.forms.application import NameAndDescriptionForm, EditEnvironmentForm from atst.forms.data import ENV_ROLE_NO_ACCESS as NO_ACCESS from atst.forms.member import NewForm as MemberForm from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.models.environment_role import EnvironmentRole from atst.models.permissions import Permissions from atst.domain.permission_sets import PermissionSets from atst.utils.flash import formatted_flash as flash @@ -31,7 +32,14 @@ def get_environments_obj_for_app(application): "edit_form": EditEnvironmentForm(obj=env), "member_count": len(env.roles), "members": sorted( - [env_role.application_role.user_name for env_role in env.roles] + [ + { + "user_name": env_role.application_role.user_name, + "status": env_role.status.value, + } + for env_role in env.roles + ], + key=lambda env_role: env_role["user_name"], ), } for env in application.environments @@ -77,11 +85,14 @@ def filter_env_roles_form_data(member, environments): "environment_id": str(env.id), "environment_name": env.name, "role": NO_ACCESS, + "disabled": False, } env_roles_set = set(env.roles).intersection(set(member.environment_roles)) + if len(env_roles_set) == 1: (env_role,) = env_roles_set env_data["role"] = env_role.role + env_data["disabled"] = env_role.status == EnvironmentRole.Status.DISABLED env_roles_form_data.append(env_data) @@ -373,14 +384,21 @@ def remove_member(application_id, application_role_id): @user_can(Permissions.EDIT_APPLICATION_MEMBER, message="update application member") def update_member(application_id, application_role_id): app_role = ApplicationRoles.get_by_id(application_role_id) - form = UpdateMemberForm(http_request.form) + application = Applications.get(application_id) + existing_env_roles_data = filter_env_roles_form_data( + app_role, application.environments + ) + form = UpdateMemberForm( + formdata=http_request.form, environment_roles=existing_env_roles_data + ) if form.validate(): ApplicationRoles.update_permission_sets(app_role, form.data["permission_sets"]) for env_role in form.environment_roles: environment = Environments.get(env_role.environment_id.data) - Environments.update_env_role(environment, app_role, env_role.data["role"]) + new_role = None if env_role.disabled.data else env_role.data["role"] + Environments.update_env_role(environment, app_role, new_role) flash("application_member_updated", user_name=app_role.user_name) else: diff --git a/static/icons/avatar.svg b/static/icons/avatar.svg index a3b68837..23737bfd 100644 --- a/static/icons/avatar.svg +++ b/static/icons/avatar.svg @@ -1 +1 @@ - + \ No newline at end of file diff --git a/styles/sections/_application_edit.scss b/styles/sections/_application_edit.scss index 1c1c395d..8ff79b7b 100644 --- a/styles/sections/_application_edit.scss +++ b/styles/sections/_application_edit.scss @@ -91,16 +91,52 @@ margin-bottom: 8 * $gap; text-align: left; - .usa-input { - margin: $gap 0 $gap 0; + .form-row { + margin: $gap * 2; + } - .usa-input__title-inline { - margin-top: $gap; - margin-left: $gap; + .usa-alert { + margin: -0.5em 0; + padding: $gap; + background-image: unset; + + &-body { + padding: 0; + width: 100%; + display: unset; + } + } + + .usa-input { + &__title-inline { + font-weight: $font-bold; } - .form-row { - margin: 0; + &__help { + margin-bottom: 0; + } + + &.env-role__no-access { + .usa-input__title-inline, + .usa-input__help { + color: $color-gray; + } + } + + &__choices { + &.revoke-button label { + padding: 1rem 2rem; + + &::before { + display: none; + } + + &.link { + color: $color-blue; + text-decoration: underline; + text-align: right; + } + } } } @@ -112,15 +148,69 @@ margin: 0.5em 0; } } - - .form-row { - margin: 0; - } } -.environment-name--gray { - font-weight: $font-normal; - color: $color-gray-medium; +.environment-role { + padding: ($gap * 2) 0; + + h4 { + margin-bottom: $gap / 4; + } + + &__users { + background-color: $color-gray-lightest; + padding: ($gap * 1.2) ($gap * 0.6); + font-size: $small-font-size; + display: flex; + flex-wrap: wrap; + + .environment-role__user { + background-color: $color-white; + border-radius: 0.5rem; + padding: ($gap / 2) $gap; + border: solid 2px $color-blue; + margin: $gap; + white-space: nowrap; + width: 20rem; + position: relative; + height: 3.6rem; + + &-field { + position: absolute; + background-color: $color-white; + margin-top: $gap * 2; + padding: $gap; + left: -0.1rem; + border: solid 1px $color-gray-light; + width: 20rem; + z-index: 3; + + .usa-input { + margin: 0; + + li { + background-color: $color-white; + border: none; + } + } + } + + &.unassigned { + border: solid 1px $color-gray-light; + } + + .icon-link { + padding: 0; + } + } + + .environment-role__no-user { + margin: $gap; + padding: ($gap / 2) $gap; + font-weight: $font-normal; + height: 3.6rem; + } + } } .application-list-item { diff --git a/templates/applications/fragments/environments.html b/templates/applications/fragments/environments.html index 923d41a9..6ff63642 100644 --- a/templates/applications/fragments/environments.html +++ b/templates/applications/fragments/environments.html @@ -64,8 +64,9 @@ {% call ToggleSection(section_name="members") %} diff --git a/templates/applications/fragments/member_form_fields.html b/templates/applications/fragments/member_form_fields.html index a466a46d..97ab3832 100644 --- a/templates/applications/fragments/member_form_fields.html +++ b/templates/applications/fragments/member_form_fields.html @@ -1,7 +1,88 @@ +{% from "components/alert.html" import Alert %} {% from "components/checkbox_input.html" import CheckboxInput %} {% from "components/text_input.html" import TextInput %} {% from "components/phone_input.html" import PhoneInput %} +{% macro EnvRoleInput(sub_form, member_role_id=None) %} + {% set role = sub_form.role.data if not sub_form.disabled.data else "Access Suspended" %} + {% if sub_form.role.data != "No Access" and not sub_form.disabled.data -%} + +
+ {% set id = "{}-{}".format(sub_form.disabled.name, member_role_id) %} +
+
+
+ {{ sub_form.environment_name.data }} +
+
+ {{ role }} +
+
+
+ {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }} + {{ sub_form.disabled.label(for=id, class="usa-button button-danger-outline") | safe }} +
+
+
+ {% call Alert(level='warning') %} +
+
+
+ {{ sub_form.environment_name.data }} +
+

+ {{ "portfolios.applications.members.form.env_access.revoke_warning" | translate | safe }} +

+
+
+ {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }} + +
+
+ {% endcall %} +
+
+ + {% else %} +
+
+
+
+ {{ sub_form.environment_name.data }} +
+

+ {{ role }} +

+
+
+
+ {% if sub_form.role.data == "No Access" and not sub_form.disabled.data -%} + +
+ {{ sub_form.role(**{"v-model": "value", "id": "{}-{}".format(sub_form.role.name, member_role_id)}) }} +
+ + {% elif sub_form.disabled.data -%} +

+ {{ "portfolios.applications.members.form.env_access.suspended" | translate }} +

+ {%- endif %} + {{ sub_form.environment_id() }} +
+
+ {% endif %} +{% endmacro %} + {% macro PermsFields(form, new=False, member_role_id=None) %}

{{ "portfolios.applications.members.form.app_perms.title" | translate }}

{{ "portfolios.applications.members.form.app_perms.description" | translate | safe}}

@@ -23,32 +104,15 @@

{{ "portfolios.applications.members.form.env_access.title" | translate }}

-

{{ "portfolios.applications.members.form.env_access.description" | translate | safe }}

+

+ {% if not new -%} + {{ "portfolios.applications.members.form.env_access.edit_description" | translate }} + {%- endif %} + {{ "portfolios.applications.members.form.env_access.description" | translate | safe }} +

{% for environment_data in form.environment_roles %} - -
-
-
-
- -
- {{ environment_data.environment_name.data }} -
- -
-
- {{ environment_data.role(**{"v-model": "value"}) }} -
-
-
-
- - {{ environment_data.environment_id() }} + {{ EnvRoleInput(environment_data, member_role_id) }}
{% endfor %}
diff --git a/templates/applications/fragments/members.html b/templates/applications/fragments/members.html index ac4aaffb..c252f452 100644 --- a/templates/applications/fragments/members.html +++ b/templates/applications/fragments/members.html @@ -36,7 +36,7 @@ {% set modal_name = "edit_member-{}".format(loop.index) %} {% call Modal(modal_name, classes="form-content--app-mem") %}
-

{{ Icon('avatar') }} {{ member.user_name }}

+

{{ Icon('avatar') }} {{ "portfolios.applications.members.form.edit_access_header" | translate({ "user": member.user_name }) }}

diff --git a/templates/applications/fragments/new_member_modal_content.html b/templates/applications/fragments/new_member_modal_content.html index d59585f0..bee6f303 100644 --- a/templates/applications/fragments/new_member_modal_content.html +++ b/templates/applications/fragments/new_member_modal_content.html @@ -45,6 +45,6 @@ {% endset %} {% call MemberFormTemplate(next_button=next_button) %} - {{ member_fields.PermsFields(form=member_form) }} + {{ member_fields.PermsFields(form=member_form, new=True) }} {% endcall %} {% endmacro %} diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 30f7a32b..6ac96488 100644 --- a/tests/domain/test_environment_roles.py +++ b/tests/domain/test_environment_roles.py @@ -1,6 +1,7 @@ import pytest from atst.domain.environment_roles import EnvironmentRoles +from atst.models.environment_role import EnvironmentRole from tests.factories import * @@ -49,7 +50,7 @@ def test_get_by_user_and_environment(application_role, environment): def test_delete(application_role, environment, monkeypatch): - EnvironmentRoleFactory.create( + env_role = EnvironmentRoleFactory.create( application_role=application_role, environment=environment ) assert EnvironmentRoles.delete(application_role.id, environment.id) @@ -88,3 +89,14 @@ def test_disable_completed(application_role, environment): environment_role = EnvironmentRoles.disable(environment_role.id) assert environment_role.status == EnvironmentRole.Status.DISABLED + + +def test_get_for_update(application_role, environment): + EnvironmentRoleFactory.create( + application_role=application_role, environment=environment, deleted=True + ) + role = EnvironmentRoles.get_for_update(application_role.id, environment.id) + assert role + assert role.application_role == application_role + assert role.environment == environment + assert role.deleted diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 4264154c..55d54f44 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -4,7 +4,7 @@ from uuid import uuid4 from atst.domain.environments import Environments from atst.domain.environment_roles import EnvironmentRoles -from atst.domain.exceptions import NotFoundError +from atst.domain.exceptions import NotFoundError, DisabledError from atst.models.environment_role import CSPRole, EnvironmentRole from tests.factories import ( @@ -28,8 +28,7 @@ def test_create_environments(): def test_update_env_role(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) new_role = CSPRole.TECHNICAL_READ.value - - assert Environments.update_env_role( + Environments.update_env_role( env_role.environment, env_role.application_role, new_role ) assert env_role.role == new_role @@ -37,23 +36,27 @@ def test_update_env_role(): def test_update_env_role_no_access(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) - - assert Environments.update_env_role( - env_role.environment, env_role.application_role, None - ) + 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 ) + assert env_role.role is None + assert env_role.status == EnvironmentRole.Status.DISABLED -def test_update_env_role_no_change(): +def test_update_env_role_disabled_role(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) - new_role = CSPRole.BASIC_ACCESS.value + Environments.update_env_role(env_role.environment, env_role.application_role, None) - assert not Environments.update_env_role( - env_role.environment, env_role.application_role, new_role - ) + with pytest.raises(DisabledError): + Environments.update_env_role( + env_role.environment, + env_role.application_role, + CSPRole.TECHNICAL_READ.value, + ) + assert env_role.role is None + assert env_role.status == EnvironmentRole.Status.DISABLED def test_get_excludes_deleted(): diff --git a/tests/forms/test_application_member.py b/tests/forms/test_application_member.py index 61a9e373..fc056223 100644 --- a/tests/forms/test_application_member.py +++ b/tests/forms/test_application_member.py @@ -10,6 +10,7 @@ def test_environment_form(): "environment_id": 123, "environment_name": "testing", "role": ENV_ROLES[0][0], + "disabled": True, } form = EnvironmentForm(data=form_data) assert form.validate() @@ -24,6 +25,7 @@ def test_environment_form_default_no_access(): "environment_id": 123, "environment_name": "testing", "role": None, + "disabled": False, } diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index a9336b72..45522edb 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -129,10 +129,14 @@ def test_edit_application_environments_obj(app, client, user_session): assert env_obj["name"] == env.name assert env_obj["id"] == env.id assert isinstance(env_obj["edit_form"], EditEnvironmentForm) - assert ( - env_obj["members"].sort() - == [app_role1.user_name, app_role2.user_name].sort() - ) + assert { + "user_name": app_role1.user_name, + "status": env_role1.status.value, + } in env_obj["members"] + assert { + "user_name": app_role2.user_name, + "status": env_role2.status.value, + } in env_obj["members"] assert isinstance(context["audit_events"], Paginator) @@ -503,10 +507,10 @@ def test_update_member(client, user_session, session): env_1 = EnvironmentFactory.create(application=application) env_2 = EnvironmentFactory.create(application=application) # add user to two of the environments: env and env_1 - EnvironmentRoleFactory.create( + updated_role = EnvironmentRoleFactory.create( environment=env, application_role=app_role, role=CSPRole.BASIC_ACCESS.value ) - EnvironmentRoleFactory.create( + suspended_role = EnvironmentRoleFactory.create( environment=env_1, application_role=app_role, role=CSPRole.BASIC_ACCESS.value ) @@ -524,8 +528,8 @@ def test_update_member(client, user_session, session): "environment_roles-0-role": CSPRole.TECHNICAL_READ.value, "environment_roles-0-environment_name": env.name, "environment_roles-1-environment_id": env_1.id, - "environment_roles-1-role": NO_ACCESS, "environment_roles-1-environment_name": env_1.name, + "environment_roles-1-disabled": "True", "environment_roles-2-environment_id": env_2.id, "environment_roles-2-role": CSPRole.NETWORK_ADMIN.value, "environment_roles-2-environment_name": env_2.name, @@ -556,11 +560,10 @@ def test_update_member(client, user_session, session): ) environment_roles = application.roles[0].environment_roles - # make sure that old env role was deleted and there are only 2 env roles - assert len(environment_roles) == 2 # check that the user has roles in the correct envs - assert environment_roles[0].environment in [env, env_2] - assert environment_roles[1].environment in [env, env_2] + assert len(environment_roles) == 3 + assert updated_role.role == CSPRole.TECHNICAL_READ.value + assert suspended_role.status == EnvironmentRole.Status.DISABLED def test_revoke_invite(client, user_session): diff --git a/translations.yaml b/translations.yaml index 360d1daa..b0705132 100644 --- a/translations.yaml +++ b/translations.yaml @@ -404,11 +404,17 @@ portfolios: env_access: title: Environment Roles table_header: Environment Access + edit_description: Add or revoke Environment access. description: Additional role controls are available in the CSP console. Learn More + revoke_warning: Save changes to revoke access, this can not be undone. + suspended: Suspended access cannot be modified. next_button: "Next: Permissions" app_perms: title: Application Permissions description: Application permissions allow users to provision and modify applications and teams. Learn More + edit_access_header: "Manage {user}'s Access" + env_access_text: "Add or revoke Environment access. Additional controls are available in the CSP console." + step_2_title: Set Permissions and Roles add_member: Add Member menu: edit: Edit Roles and Permissions