Skip to content

feat: Implement API to GDPR delete users#3224

Draft
Janis4411 wants to merge 1 commit intomainfrom
jv/SODEV-2997-Implement-API-to-GDPR-delete-users
Draft

feat: Implement API to GDPR delete users#3224
Janis4411 wants to merge 1 commit intomainfrom
jv/SODEV-2997-Implement-API-to-GDPR-delete-users

Conversation

@Janis4411
Copy link
Copy Markdown
Contributor

This PR introduces a possible way to GDPR delete users via a API.

If we delete a user on openHPI we also have to delete the user on codeocean. Previously this was handled via a rake task that has to be triggered manually.

Part of SODEV-2997

@Janis4411 Janis4411 requested a review from arkirchner April 16, 2026 13:25
@Janis4411 Janis4411 force-pushed the jv/SODEV-2997-Implement-API-to-GDPR-delete-users branch from 85f5e83 to 4c3c10a Compare April 16, 2026 13:26
Comment thread app/controllers/api/internal/users/deletions_controller.rb Fixed
Comment on lines +21 to +22
verify_token!
verify_signature!
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Sequential calls to head :unauthorized in verify_token! and verify_signature! without a return can cause a DoubleRenderError if both checks fail.
Severity: MEDIUM

Suggested Fix

Add an explicit return statement after each head :unauthorized call in both the verify_token! and verify_signature! methods to ensure execution is halted after the response is rendered.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: app/controllers/api/internal/users/deletions_controller.rb#L21-L22

Potential issue: In the `deletions_controller`, the `authenticate_request!` method
sequentially calls `verify_token!` and `verify_signature!`. If a token is invalid,
`verify_token!` calls `head :unauthorized` but does not halt execution. Control then
proceeds to `verify_signature!`. If the signature is also invalid, it too calls `head
:unauthorized`. This second attempt to render a response in the same request cycle
triggers an `ActionController::DoubleRenderError`. This will occur for any API request
that has both an invalid token and an invalid signature.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread app/jobs/user_cleanup_job.rb Outdated
Comment on lines +15 to +16
user = ExternalUser.find_by(external_id: user_id, consumer_id: 1) # Consumer with ID 1 is openHPI.
user.update(name: 'Deleted User', email: nil)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The UserCleanupJob does not handle cases where a user is not found, causing a NoMethodError when calling .update on a nil object.
Severity: HIGH

Suggested Fix

Add a guard clause to check if the user object is present before attempting to call .update on it. For example, use user&.update(...) or wrap the call in an if user.present? block.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: app/jobs/user_cleanup_job.rb#L15-L16

Potential issue: The `UserCleanupJob` is queued by the `deletions_controller` without
first validating the existence of the `user_id`. Inside the job, `cleanup_user_data`
uses `ExternalUser.find_by` to locate the user. If no user is found, `find_by` returns
`nil`. The next line unconditionally calls `.update` on this result, which raises a
`NoMethodError: undefined method 'update' for nil:NilClass`. The job will fail and
retry, but ultimately the user's data will not be deleted, causing a silent failure of
the GDPR deletion process.

Did we get this right? 👍 / 👎 to inform future reviews.

@Janis4411 Janis4411 marked this pull request as draft April 16, 2026 13:34
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.23%. Comparing base (7876111) to head (5cee070).

Files with missing lines Patch % Lines
...rollers/api/internal/users/deletions_controller.rb 45.83% 13 Missing ⚠️
app/jobs/user_cleanup_job.rb 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3224      +/-   ##
==========================================
- Coverage   70.33%   70.23%   -0.11%     
==========================================
  Files         214      216       +2     
  Lines        6839     6873      +34     
==========================================
+ Hits         4810     4827      +17     
- Misses       2029     2046      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This is a possible way to GDPR delete users via a API. The background
is that if we delete a user on openhpi we also want to delete the user
on codeocean.

Part of SODEV-2997
@Janis4411 Janis4411 force-pushed the jv/SODEV-2997-Implement-API-to-GDPR-delete-users branch from 4c3c10a to 5cee070 Compare April 16, 2026 15:11
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