Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add soft delete for users and permissions #952

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

harishv7
Copy link
Contributor

@harishv7 harishv7 commented Dec 20, 2024

Notes

  1. Do not merge till security review
  2. Requires expanding migration

Problem

Closes ISOM-1701

Solution

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • Added soft delete functionality for Users and ResourcePermissions via deletedAt column
  • Added user deletion check during email authentication flow

Improvements:

  • Enhanced permission queries to exclude soft-deleted records
  • Added filtering for deleted users in ongoing sessions

Bug Fixes:

  • Fixed potential security issue where deleted users could still access the system with active sessions

Tests

  • Verify soft delete functionality for Users, by setting deletedAt to any valid timestamp
  • Users with deletedAt in the User table should not be able to login
  • Users who are already logged in with an active session should be logged out when the deletedAt is set in User table
  • Users can have Resource Permissions to several sites. If a Resource Permission is set, then the User should not be able to see that specific site in the All Sites view. Visitng the deleted resource Site's URL directly should show that they are unauthorised to view the page

New scripts:

  • 20241220130729_add_deletedat_user_perms : Adds deletedAt columns to User and ResourcePermission tables

@harishv7 harishv7 changed the title soft delete for user and perms add soft delete for users and permissions Dec 20, 2024
Copy link
Contributor Author

harishv7 commented Dec 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@harishv7 harishv7 marked this pull request as ready for review December 20, 2024 15:24
@harishv7 harishv7 requested a review from a team as a code owner December 20, 2024 15:24
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 20, 2024

Datadog Report

Branch report: 12-20-soft_delete_for_user_and_perms
Commit report: 4d4bdc9
Test service: isomer-studio

✅ 0 Failed, 257 Passed, 36 Skipped, 45.46s Total Time
➡️ Test Sessions change in coverage: 1 no change

Copy link

linear bot commented Dec 20, 2024

Copy link
Contributor

@adriangohjw adriangohjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, it seems like this solution isn't "audit complete".

For example, how do we handle situation where agency wants to restore a user's permission for a site's resource? we cannot add another record due to ResourcePermission's @@unique([userId, siteId, resourceId, role]). We can remove the deletedAt but it will mean remove the audit trail aspect of it

It's alright if this is a stopgap measure, but just want to clarify that this is intentional

Not urgent but we might also want to add a backlog ticket to write a script to automate this.

apps/studio/src/server/modules/user/user.service.ts Outdated Show resolved Hide resolved
apps/studio/src/server/modules/user/user.service.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

harishv7 commented Jan 9, 2025

@adriangohjw this measure is to support soft deletes for now. In terms of audit completeness, we will have to re-look into that in ISOM-1072

To re-store a user's permissions, we will need to remove the deletedAt in User table. For ResourcePermission we will add a new entry. To find out the audit trail, we can check based on the userId when were the resource permissions removed fully and re-added.

Couple the above with Ops email/Slack request, there is still some kind of full audit picture. Lmk if that makes sense!

@harishv7 harishv7 requested a review from spaceraccoon January 15, 2025 03:30
@harishv7 harishv7 force-pushed the 12-20-soft_delete_for_user_and_perms branch from 6f97b13 to 11d220c Compare January 15, 2025 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants