Conversation
85f5e83 to
4c3c10a
Compare
| verify_token! | ||
| verify_signature! |
There was a problem hiding this comment.
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.
| user = ExternalUser.find_by(external_id: user_id, consumer_id: 1) # Consumer with ID 1 is openHPI. | ||
| user.update(name: 'Deleted User', email: nil) |
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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
4c3c10a to
5cee070
Compare
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