Skip to content

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jul 3, 2025

Fixes #4742

This allows using the endpoint to deactivate a user without GDPR-erasing it

This allows using the endpoint to deactivate a user without deleting it.

TODO: make the request body optional.
Copy link

cloudflare-workers-and-pages bot commented Jul 3, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 49f2dae
Status: ✅  Deploy successful!
Preview URL: https://b31f2644.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://af-rest-deactivate-erase-opt.matrix-authentication-service-docs.pages.dev

View logs

If body is absent, treat "erase" as true.
If body is present, require "erase" to be present in the body.
@AndrewFerr AndrewFerr marked this pull request as ready for review July 4, 2025 18:53
@AndrewFerr
Copy link
Member Author

AndrewFerr commented Jul 4, 2025

The failure is due to the schema updater marking the request body of /deactivate as required, but I want it to be optional. The unwanted change is:

diff --git a/docs/api/spec.json b/docs/api/spec.json
index cb597e14..c0e2ee51 100644
--- a/docs/api/spec.json
+++ b/docs/api/spec.json
@@ -1367,7 +1367,7 @@
               }
             }
           },
-          "required": false
+          "required": true
         },
         "responses": {
           "200": {

In deactivate.rs, I've already allowed the handler to accept an optional body (with a parameter of body: Option<Json<Request>>), so in practice the body really is optional (and a unit test confirms that).

Is there a way to make schema generation aware of the body being optional?

The reason I want it to be optional is to keep the endpoint backwards-compatible with its current form, which expects no body.

EDIT: this is now handled by 1101dd9.

Comment on lines 176 to 186
// It should have scheduled a deactivation job for the user
// XXX: we don't have a good way to look for the deactivation job
let job: Json<serde_json::Value> = sqlx::query_scalar(
"SELECT payload FROM queue_jobs WHERE queue_name = 'deactivate-user'",
)
.fetch_one(&pool)
.await
.expect("Deactivation job to be scheduled");
assert_eq!(job["user_id"], serde_json::json!(user.id));
assert_eq!(job["hs_erase"], serde_json::json!(erase.unwrap_or(true)));

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept this in to check for the erase parameter on the job, as the response to GET /users doesn't indicate whether the user was erased or not.

@AndrewFerr AndrewFerr requested a review from sandhose July 11, 2025 14:10
This makes it more intuitive for an empty request body to be equivalent
to the option being set to false.
@sandhose sandhose changed the title Add "erase" option to REST deactivate request body Allow skipping GDPR-erasure when deactivating a user through the admin API Jul 17, 2025
Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Not 100% convinced by the negation (skip_erase vs. erase) but I guess it makes it backwards-compatible

@sandhose sandhose added the A-Admin-API Related to the admin API label Jul 17, 2025
@sandhose sandhose merged commit b83c747 into main Jul 17, 2025
28 checks passed
@sandhose sandhose deleted the af/rest-deactivate-erase-option branch July 17, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Admin-API Related to the admin API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REST Admin API to deactivate user without erasing it

2 participants