From 5a31d1b93fb0aa324ec77b56999b59db45a77ff5 Mon Sep 17 00:00:00 2001 From: Jonathan Griffin Date: Thu, 17 Apr 2025 17:18:38 +0200 Subject: [PATCH] update DELETE /Users endpoint to use tenancy and to soft delete --- ee/api/chalicelib/core/users.py | 205 ++++++------------------------- ee/api/routers/scim.py | 50 +++----- ee/api/routers/scim_constants.py | 1 - 3 files changed, 59 insertions(+), 197 deletions(-) diff --git a/ee/api/chalicelib/core/users.py b/ee/api/chalicelib/core/users.py index de73bafd9..f9dba0619 100644 --- a/ee/api/chalicelib/core/users.py +++ b/ee/api/chalicelib/core/users.py @@ -294,6 +294,7 @@ def get_scim_user_by_id(user_id, tenant_id): WHERE users.user_id = %(user_id)s AND users.tenant_id = %(tenant_id)s + AND users.deleted_at IS NULL LIMIT 1; """, { @@ -304,42 +305,6 @@ def get_scim_user_by_id(user_id, tenant_id): ) return helper.dict_to_camel_case(cur.fetchone()) -def get_deleted_by_uuid(user_uuid, tenant_id): - with pg_client.PostgresClient() as cur: - cur.execute( - cur.mogrify( - f"""SELECT - users.user_id, - users.tenant_id, - email, - role, - users.name, - users.data, - users.internal_id, - (CASE WHEN role = 'owner' THEN TRUE ELSE FALSE END) AS super_admin, - (CASE WHEN role = 'admin' THEN TRUE ELSE FALSE END) AS admin, - (CASE WHEN role = 'member' THEN TRUE ELSE FALSE END) AS member, - origin, - role_id, - roles.name AS role_name, - roles.permissions, - roles.all_projects, - basic_authentication.password IS NOT NULL AS has_password, - users.service_account - FROM public.users LEFT JOIN public.basic_authentication ON users.user_id=basic_authentication.user_id - LEFT JOIN public.roles USING (role_id) - WHERE - users.data->>'user_id' = %(user_uuid)s - AND users.tenant_id = %(tenant_id)s - AND users.deleted_at IS NOT NULL - AND (roles.role_id IS NULL OR roles.deleted_at IS NULL AND roles.tenant_id = %(tenant_id)s) - LIMIT 1;""", - {"user_uuid": user_uuid, "tenant_id": tenant_id}) - ) - r = cur.fetchone() - return helper.dict_to_camel_case(r) - - def generate_new_api_key(user_id): with pg_client.PostgresClient() as cur: @@ -438,7 +403,7 @@ def edit_member(user_id_to_update, tenant_id, changes: schemas.EditMemberSchema, return {"data": user} -def get_scim_user_by_unique_values(email): +def get_existing_scim_user_by_unique_values(email): with pg_client.PostgresClient() as cur: cur.execute( cur.mogrify( @@ -488,7 +453,9 @@ def get_users_paginated(start_index, tenant_id, count=None): """ SELECT * FROM public.users - WHERE users.tenant_id = %(tenant_id)s + WHERE + users.tenant_id = %(tenant_id)s + AND users.deleted_at IS NULL LIMIT %(limit)s OFFSET %(offset)s; """, @@ -616,70 +583,6 @@ def delete_member(user_id, tenant_id, id_to_delete): return {"data": get_members(tenant_id=tenant_id)} -def delete_member_as_admin(tenant_id, id_to_delete): - - with pg_client.PostgresClient() as cur: - cur.execute( - cur.mogrify( - f"""SELECT - users.user_id AS user_id, - users.tenant_id, - email, - role, - users.name, - origin, - role_id, - roles.name AS role_name, - (CASE WHEN role = 'member' THEN TRUE ELSE FALSE END) AS member, - roles.permissions, - roles.all_projects, - basic_authentication.password IS NOT NULL AS has_password, - users.service_account - FROM public.users LEFT JOIN public.basic_authentication ON users.user_id=basic_authentication.user_id - LEFT JOIN public.roles USING (role_id) - WHERE - role = 'owner' - AND users.tenant_id = %(tenant_id)s - AND users.deleted_at IS NULL - AND (roles.role_id IS NULL OR roles.deleted_at IS NULL AND roles.tenant_id = %(tenant_id)s) - LIMIT 1;""", - {"tenant_id": tenant_id, "user_uuid": id_to_delete}) - ) - r = cur.fetchone() - - if r["user_id"] == id_to_delete: - return {"errors": ["unauthorized, cannot delete self"]} - - if r["member"]: - return {"errors": ["unauthorized"]} - - to_delete = get(user_id=id_to_delete, tenant_id=tenant_id) - if to_delete is None: - return {"errors": ["not found"]} - - if to_delete["superAdmin"]: - return {"errors": ["cannot delete super admin"]} - - with pg_client.PostgresClient() as cur: - cur.execute( - cur.mogrify(f"""UPDATE public.users - SET deleted_at = timezone('utc'::text, now()), - jwt_iat= NULL, jwt_refresh_jti= NULL, - jwt_refresh_iat= NULL, - role_id=NULL - WHERE user_id=%(user_id)s AND tenant_id=%(tenant_id)s;""", - {"user_id": id_to_delete, "tenant_id": tenant_id})) - cur.execute( - cur.mogrify(f"""UPDATE public.basic_authentication - SET password= NULL, invitation_token= NULL, - invited_at= NULL, changed_at= NULL, - change_pwd_expire_at= NULL, change_pwd_token= NULL - WHERE user_id=%(user_id)s;""", - {"user_id": id_to_delete, "tenant_id": tenant_id})) - return {"data": get_members(tenant_id=tenant_id)} - - - def change_password(tenant_id, user_id, email, old_password, new_password): item = get(tenant_id=tenant_id, user_id=user_id) if item is None: @@ -1056,6 +959,21 @@ def create_scim_user( return helper.dict_to_camel_case(cur.fetchone()) +def soft_delete_scim_user_by_id(user_id, tenant_id): + with pg_client.PostgresClient() as cur: + cur.execute( + cur.mogrify( + """ + UPDATE public.users + SET deleted_at = NULL + WHERE + users.user_id = %(user_id)s + AND users.tenant_id = %(tenant_id)s + """, + {"user_id": user_id, "tenant_id": tenant_id} + ) + ) + def __hard_delete_user(user_id): with pg_client.PostgresClient() as cur: @@ -1065,13 +983,6 @@ def __hard_delete_user(user_id): {"user_id": user_id}) cur.execute(query) -def __hard_delete_user_uuid(user_uuid): - with pg_client.PostgresClient() as cur: - query = cur.mogrify( - f"""DELETE FROM public.users - WHERE users.data->>'user_id' = %(user_uuid)s;""", # removed this: AND users.deleted_at IS NOT NULL - {"user_uuid": user_uuid}) - cur.execute(query) def logout(user_id: int): @@ -1199,64 +1110,28 @@ def restore_sso_user(user_id, tenant_id, email, admin, name, origin, role_id, in def restore_scim_user( user_id, tenant_id, - user_uuid, - email, - admin, - display_name, - full_name: dict, - emails, - origin, - locale, - role_id, - internal_id=None): +): with pg_client.PostgresClient() as cur: - query = cur.mogrify(f"""\ - WITH u AS ( - UPDATE public.users - SET tenant_id= %(tenant_id)s, - role= %(role)s, - name= %(name)s, - data= %(data)s, - origin= %(origin)s, - internal_id= %(internal_id)s, - role_id= (SELECT COALESCE((SELECT role_id FROM roles WHERE tenant_id = %(tenant_id)s AND role_id = %(role_id)s), - (SELECT role_id FROM roles WHERE tenant_id = %(tenant_id)s AND name = 'Member' LIMIT 1), - (SELECT role_id FROM roles WHERE tenant_id = %(tenant_id)s AND name != 'Owner' LIMIT 1))), - deleted_at= NULL, - created_at= default, - api_key= default, - jwt_iat= NULL, - weekly_report= default - WHERE user_id = %(user_id)s - RETURNING * - ), - au AS ( - UPDATE public.basic_authentication - SET password= default, - invitation_token= default, - invited_at= default, - change_pwd_token= default, - change_pwd_expire_at= default, - changed_at= NULL - WHERE user_id = %(user_id)s - RETURNING user_id - ) - SELECT u.user_id AS id, - u.email, - u.role, - u.name, - u.data, - (CASE WHEN u.role = 'owner' THEN TRUE ELSE FALSE END) AS super_admin, - (CASE WHEN u.role = 'admin' THEN TRUE ELSE FALSE END) AS admin, - (CASE WHEN u.role = 'member' THEN TRUE ELSE FALSE END) AS member, - origin - FROM u;""", - {"tenant_id": tenant_id, "email": email, "internal_id": internal_id, - "role": "admin" if admin else "member", "name": display_name, "origin": origin, - "role_id": role_id, "data": json.dumps({"lastAnnouncementView": TimeUTC.now(), "user_id": user_uuid, "locale": locale, "name": full_name, "emails": emails}), - "user_id": user_id}) cur.execute( - query + cur.mogrify( + """ + WITH u AS ( + UPDATE public.users + SET + tenant_id = %(tenant_id)s, + deleted_at = NULL, + created_at = default, + api_key = default, + jwt_iat = NULL, + weekly_report = default + WHERE user_id = %(user_id)s + RETURNING * + ) + SELECT * + FROM u; + """, + {"tenant_id": tenant_id, "user_id": user_id} + ) ) return helper.dict_to_camel_case(cur.fetchone()) diff --git a/ee/api/routers/scim.py b/ee/api/routers/scim.py index b1c73a568..83138dba6 100644 --- a/ee/api/routers/scim.py +++ b/ee/api/routers/scim.py @@ -306,30 +306,20 @@ def get_user( @public_app.post("/Users") async def create_user(r: UserRequest, tenant_id = Depends(auth_required)): - existing_db_user = users.get_scim_user_by_unique_values(r.userName) - # todo(jon): we have a conflict in our db schema and the docs for SCIM. - # here is a quote from section 3.6 of RFC 7644: - # For example, if a User resource is deleted, a CREATE - # request for a User resource with the same userName as the previously - # deleted resource SHOULD NOT fail with a 409 error due to userName - # conflict. - # this doesn't work with our db schema as `public.users.email` is UNIQUE - # but not conditionaly unique based on deletion. this would be fine if - # we did a hard delete, but it seems like we do soft deletes. - # so, we need to figure out how to handle this: - # 1. we do hard deletes for scim users. - # 2. we change how we handle the unique constraint for users on the email field. - # i think the easiest thing to do here would be 1, since it wouldn't require - # updating any other parts of the codebase (potentially) - if existing_db_user: + # note(jon): this method will return soft deleted users as well + existing_db_user = users.get_existing_scim_user_by_unique_values(r.userName) + if existing_db_user and existing_db_user["deletedAt"] is None: return _uniqueness_error_response() - db_user = users.create_scim_user( - email=r.userName, - # note(jon): scim schema does not require the `name.formatted` attribute, but we require `name`. - # so, we have to define the value ourselves here - name="", - tenant_id=tenant_id, - ) + if existing_db_user and existing_db_user["deletedAt"] is not None: + db_user = users.restore_scim_user(existing_db_user["userId"], tenant_id) + else: + db_user = users.create_scim_user( + email=r.userName, + # note(jon): scim schema does not require the `name.formatted` attribute, but we require `name`. + # so, we have to define the value ourselves here + name="", + tenant_id=tenant_id, + ) scim_user = _convert_db_user_to_scim_user(db_user) response = JSONResponse( status_code=201, @@ -391,15 +381,13 @@ def deactivate_user(user_id: str, r: PatchUserRequest): return Response(status_code=204, content="") -@public_app.delete("/Users/{user_uuid}", dependencies=[Depends(auth_required)]) -def delete_user(user_uuid: str): - """Delete user from database, hard-delete""" - tenant_id = 1 - user = users.get_by_uuid(user_uuid, tenant_id) - if not user: - raise HTTPException(status_code=404, detail="User not found") - users.__hard_delete_user_uuid(user_uuid) +@public_app.delete("/Users/{user_id}") +def delete_user(user_id: str, tenant_id = Depends(auth_required)): + user = users.get_scim_user_by_id(user_id, tenant_id) + if not user: + return _not_found_error_response(user_id) + users.soft_delete_scim_user_by_id(user_id, tenant_id) return Response(status_code=204, content="") diff --git a/ee/api/routers/scim_constants.py b/ee/api/routers/scim_constants.py index 255bfba3e..5a6256ee3 100644 --- a/ee/api/routers/scim_constants.py +++ b/ee/api/routers/scim_constants.py @@ -696,7 +696,6 @@ USER_SCHEMA = { } - SCHEMAS = sorted( [ SERVICE_PROVIDER_CONFIG_SCHEMA,