diff --git a/alembic/versions/e05d1f2682af_remove_deleted_environment_role_status.py b/alembic/versions/e05d1f2682af_remove_deleted_environment_role_status.py new file mode 100644 index 00000000..45c58930 --- /dev/null +++ b/alembic/versions/e05d1f2682af_remove_deleted_environment_role_status.py @@ -0,0 +1,59 @@ +"""remove deleted environment_role status + +Revision ID: e05d1f2682af +Revises: 1497926ddec1 +Create Date: 2019-10-14 16:03:33.816215 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'e05d1f2682af' # pragma: allowlist secret +down_revision = '1497926ddec1' # pragma: allowlist secret +branch_labels = None +depends_on = None + + +def upgrade(): + conn = op.get_bind() + conn.execute( + """ + UPDATE environment_roles + SET status = (CASE WHEN status = 'DELETED' THEN 'PENDING_DELETE' ELSE status END) + """ + ) + op.alter_column( + "environment_roles", + "status", + type_=sa.Enum( + "PENDING", "COMPLETED", "PENDING_DELETE", name="status", native_enum=False + ), + existing_type=sa.Enum( + "PENDING", + "COMPLETED", + "PENDING_DELETE", + "DELETED", + name="status", + native_enum=False, + ), + ) + +def downgrade(): + op.alter_column( + "environment_roles", + "status", + type_=sa.Enum( + "PENDING", + "COMPLETED", + "PENDING_DELETE", + "DELETED", + name="status", + native_enum=False, + ), + existing_type=sa.Enum( + "PENDING", "COMPLETED", "PENDING_DELETE", name="status", native_enum=False + ), + ) + diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index bdf4ae09..016110a2 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -27,6 +27,7 @@ class EnvironmentRoles(object): .filter( EnvironmentRole.application_role_id == application_role_id, EnvironmentRole.environment_id == environment_id, + EnvironmentRole.deleted == False, ) .one_or_none() ) @@ -49,6 +50,7 @@ class EnvironmentRoles(object): .filter( ApplicationRole.user_id == user_id, EnvironmentRole.environment_id == environment_id, + EnvironmentRole.deleted == False, ) .one_or_none() ) @@ -56,20 +58,12 @@ class EnvironmentRoles(object): @classmethod def delete(cls, application_role_id, environment_id): - existing_env_role = ( - db.session.query(EnvironmentRole) - .join(ApplicationRole) - .filter( - ApplicationRole.id == application_role_id, - EnvironmentRole.environment_id == environment_id, - EnvironmentRole.status != EnvironmentRole.Status.PENDING_DELETE, - ) - .one_or_none() - ) + existing_env_role = EnvironmentRoles.get(application_role_id, environment_id) if existing_env_role: - existing_env_role.status = EnvironmentRole.Status.PENDING_DELETE - existing_env_role.role = "deleted" + # TODO: Implement suspension + existing_env_role.deleted = True + db.session.add(existing_env_role) db.session.commit() return True else: @@ -97,17 +91,3 @@ class EnvironmentRoles(object): .all() ) return [id_ for id_, in results] - - @classmethod - def get_environment_roles_pending_deletion(cls) -> List[UUID]: - results = ( - db.session.query(EnvironmentRole.id) - .join(Environment) - .join(ApplicationRole) - .filter(Environment.deleted == False) - .filter(Environment.baseline_info != None) - .filter(EnvironmentRole.status == EnvironmentRole.Status.PENDING_DELETE) - .filter(ApplicationRole.status == ApplicationRoleStatus.ACTIVE) - .all() - ) - return [id_ for id_, in results] diff --git a/atst/jobs.py b/atst/jobs.py index 320cbbfe..379e6b14 100644 --- a/atst/jobs.py +++ b/atst/jobs.py @@ -132,19 +132,6 @@ def do_provision_user(csp: CloudProviderInterface, environment_role_id=None): db.session.commit() -def do_delete_user(csp: CloudProviderInterface, environment_role_id=None): - environment_role = EnvironmentRoles.get_by_id(environment_role_id) - - with claim_for_update(environment_role) as environment_role: - credentials = environment_role.environment.csp_credentials - - csp.delete_user(credentials, environment_role.csp_user_id) - environment_role.status = EnvironmentRole.Status.DELETED - environment_role.deleted = True - db.session.add(environment_role) - db.session.commit() - - def do_work(fn, task, csp, **kwargs): try: fn(csp, **kwargs) @@ -181,13 +168,6 @@ def provision_user(self, environment_role_id=None): ) -@celery.task(bind=True) -def delete_user(self, environment_role_id=None): - do_work( - do_delete_user, self, app.csp.cloud, environment_role_id=environment_role_id - ) - - @celery.task(bind=True) def dispatch_create_environment(self): for environment_id in Environments.get_environments_pending_creation( @@ -218,11 +198,3 @@ def dispatch_provision_user(self): environment_role_id ) in EnvironmentRoles.get_environment_roles_pending_creation(): provision_user.delay(environment_role_id=environment_role_id) - - -@celery.task(bind=True) -def dispatch_delete_user(self): - for ( - environment_role_id - ) in EnvironmentRoles.get_environment_roles_pending_deletion(): - delete_user.delay(environment_role_id=environment_role_id) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index f3e8004f..988d19ab 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -40,7 +40,6 @@ class EnvironmentRole( PENDING = "pending" COMPLETED = "completed" PENDING_DELETE = "pending_delete" - DELETED = "deleted" status = Column(SQLAEnum(Status, native_enum=False), default=Status.PENDING) diff --git a/atst/queue.py b/atst/queue.py index 3a68be54..c7d117c6 100644 --- a/atst/queue.py +++ b/atst/queue.py @@ -22,10 +22,6 @@ def update_celery(celery, app): "task": "atst.jobs.dispatch_provision_user", "schedule": 60, }, - "beat-dispatch_delete_user": { - "task": "atst.jobs.dispatch_delete_user", - "schedule": 60, - }, } class ContextTask(celery.Task): diff --git a/tests/domain/test_application_roles.py b/tests/domain/test_application_roles.py index 32c5cd2e..ca5c6fc1 100644 --- a/tests/domain/test_application_roles.py +++ b/tests/domain/test_application_roles.py @@ -85,4 +85,4 @@ def test_disable(session): session.refresh(member_role) session.refresh(environment_role) assert member_role.status == ApplicationRoleStatus.DISABLED - assert environment_role.status == EnvironmentRole.Status.PENDING_DELETE + assert environment_role.deleted diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index dc92d9e0..2a2e3bb1 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -35,15 +35,16 @@ def test_update_env_role(): assert env_role.role == new_role -def test_update_env_role_no_access(session): +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 ) - session.refresh(env_role) - assert env_role.status == EnvironmentRole.Status.PENDING_DELETE + assert not EnvironmentRoles.get( + env_role.application_role.id, env_role.environment.id + ) def test_update_env_role_no_change(): diff --git a/tests/models/test_application_role.py b/tests/models/test_application_role.py index 618d983e..f4526f59 100644 --- a/tests/models/test_application_role.py +++ b/tests/models/test_application_role.py @@ -1,4 +1,5 @@ from atst.domain.permission_sets import PermissionSets +from atst.domain.environment_roles import EnvironmentRoles from atst.models.audit_event import AuditEvent from tests.factories import * @@ -53,4 +54,4 @@ def test_environment_roles(): environment=environment2, application_role=application_role, deleted=True ) - assert application_role.environment_roles == [environment_role1] + assert not EnvironmentRoles.get_by_user_and_environment(user.id, environment2.id) diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 62bfd34a..1b873ec2 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -537,6 +537,13 @@ def test_update_member(client, user_session, session): app_role.has_permission_set(PermissionSets.DELETE_APPLICATION_ENVIRONMENTS) ) + 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] + def test_revoke_invite(client, user_session): invite = ApplicationInvitationFactory.create() diff --git a/tests/test_jobs.py b/tests/test_jobs.py index ce710d69..c21728af 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -17,8 +17,6 @@ from atst.jobs import ( create_environment, dispatch_provision_user, do_provision_user, - do_delete_user, - dispatch_delete_user, ) from atst.models.utils import claim_for_update from atst.domain.exceptions import ClaimFailedException @@ -372,68 +370,3 @@ def test_do_provision_user(csp, session): ) # I expect that the EnvironmentRole now has a csp_user_id assert environment_role.csp_user_id - - -def test_do_delete_user(csp, session): - credentials = MockCloudProvider(())._auth_credentials - provisioned_environment = EnvironmentFactory.create( - cloud_id="cloud_id", - root_user_info={"credentials": credentials}, - baseline_info={}, - ) - - environment_role = EnvironmentRoleFactory.create( - environment=provisioned_environment, - status=EnvironmentRole.Status.PENDING_DELETE, - role="my_role", - ) - - do_delete_user(csp=csp, environment_role_id=environment_role.id) - - session.refresh(environment_role) - - assert environment_role.status == EnvironmentRole.Status.DELETED - assert environment_role.deleted == True - - -def test_dispatch_delete_user(csp, session, monkeypatch): - # Given that I have five environment roles: - # (A) one of which has a completed status - # (B) one of which has an environment that has not been provisioned - # (C) one of which is pending, has a provisioned environment but an inactive application role - # (D) one of which is pending, has a provisioned environment and has an active application role - # (E) one of which is pending delete, has a provisioned environment and has an active application role - provisioned_environment = EnvironmentFactory.create( - cloud_id="cloud_id", root_user_info={}, baseline_info={} - ) - unprovisioned_environment = EnvironmentFactory.create() - _er_a = EnvironmentRoleFactory.create( - environment=provisioned_environment, status=EnvironmentRole.Status.COMPLETED - ) - _er_b = EnvironmentRoleFactory.create( - environment=unprovisioned_environment, status=EnvironmentRole.Status.PENDING - ) - _er_c = EnvironmentRoleFactory.create( - environment=unprovisioned_environment, - status=EnvironmentRole.Status.PENDING, - application_role=ApplicationRoleFactory(status=ApplicationRoleStatus.PENDING), - ) - _er_d = EnvironmentRoleFactory.create( - environment=unprovisioned_environment, - status=EnvironmentRole.Status.PENDING_DELETE, - application_role=ApplicationRoleFactory(status=ApplicationRoleStatus.PENDING), - ) - er_e = EnvironmentRoleFactory.create( - environment=provisioned_environment, - status=EnvironmentRole.Status.PENDING_DELETE, - application_role=ApplicationRoleFactory(status=ApplicationRoleStatus.ACTIVE), - ) - - mock = Mock() - monkeypatch.setattr("atst.jobs.delete_user", mock) - - # When I dispatch the user deletion task - dispatch_delete_user.run() - - # I expect it to dispatch only one call, to EnvironmentRole E - mock.delay.assert_called_once_with(environment_role_id=er_e.id)