Revert user deletion job

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
+        ),
+    )
+

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 = (
+        existing_env_role = EnvironmentRoles.get(application_role_id, environment_id)
-            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()
-        )
 
         if existing_env_role:
-            existing_env_role.status = EnvironmentRole.Status.PENDING_DELETE
+            # TODO: Implement suspension
-            existing_env_role.role = "deleted"
+            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]

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)

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)
 

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):

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

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 not EnvironmentRoles.get(
-    assert env_role.status == EnvironmentRole.Status.PENDING_DELETE
+        env_role.application_role.id, env_role.environment.id
+    )
 
 
 def test_update_env_role_no_change():

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)

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()

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)