Page MenuHomePhabricator

[keyserver] Update keyserver delete account endpoint to not require password
ClosedPublic

Authored by varun on Sep 19 2023, 9:04 PM.
Tags
None
Referenced Files
F3158232: D9236.id31727.diff
Tue, Nov 5, 9:39 PM
F3158073: D9236.id31291.diff
Tue, Nov 5, 9:28 PM
F3157078: D9236.diff
Tue, Nov 5, 7:14 PM
Unknown Object (File)
Fri, Nov 1, 8:58 AM
Unknown Object (File)
Fri, Nov 1, 8:58 AM
Unknown Object (File)
Fri, Nov 1, 8:58 AM
Unknown Object (File)
Fri, Nov 1, 8:56 AM
Unknown Object (File)
Fri, Nov 1, 4:09 AM
Subscribers

Details

Summary

in the future we won't be sending passwords to keyservers at all. will update client side to not send password hash to keyserver when account deletion flow goes through identity service.

Test Plan
 varun  ~  Code  comm  keyserver  git diff                                                            ST 228   ab660e6 
diff --git a/keyserver/src/responders/handlers.js b/keyserver/src/responders/handlers.js
index 2ecc83081..b9c0908bc 100644
--- a/keyserver/src/responders/handlers.js
+++ b/keyserver/src/responders/handlers.js
@@ -46,6 +46,7 @@ function createJSONResponder<I, O>(
 ): JSONResponder {
   return {
     responder: async (viewer, input) => {
+      console.log('Got request with password ' + JSON.stringify(input));
       const request = await validateInput(viewer, inputValidator, input);
       const result = await responder(viewer, request);
       return validateOutput(viewer.platformDetails, outputValidator, result);
diff --git a/lib/actions/user-actions.js b/lib/actions/user-actions.js
index 01542ce9f..8eeacb145 100644
--- a/lib/actions/user-actions.js
+++ b/lib/actions/user-actions.js
@@ -95,7 +95,7 @@ const deleteAccount =
     preRequestUserState: PreRequestUserState,
   ) => Promise<LogOutResult>) =>
   async (password, preRequestUserState) => {
-    const response = await callServerEndpoint('delete_account', { password });
+    const response = await callServerEndpoint('delete_account', {});
     return { currentUserInfo: response.currentUserInfo, preRequestUserState };
   };

modified user-actions.js to not send a password -- checked on the keyserver that no password was sent and account deletion was still successful.

undid the modification to user-actions.js and confirmed that older clients that send a password can still delete accounts successfully.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Sep 19 2023, 9:23 PM
ashoat requested changes to this revision.Sep 20 2023, 5:52 AM
  1. Please extend the test plan to include testing a request where the password field is not supplied. deleteAccountRequestInputValidator takes a password: t.maybe(tPassword), and it's not clear to me if that will crash when password is not supplied (as opposed to password: undefined or something)
  2. After this change, we no longer need a request passed to deleteAccount at all. While accountDeletionResponder will need to support a request for a while (for old clients), deleteAccount is downstream and should be abstracted from that
    • First, this will require some changes in this file so that request is no longer a param. See inline comments
    • Second, you'll want to update accountDeletionResponder to no longer pass request in
keyserver/src/deleters/account-deleters.js
30 ↗(On Diff #31291)

Remove this param

32 ↗(On Diff #31291)

All script viewers are logged in, so this can be significantly simplified

83 ↗(On Diff #31291)

There is only one callsite that doesn't pass a request in: keyserver/src/scripts/merge-users.js. We can replace this with a viewer.isScriptViewer check

118–122 ↗(On Diff #31291)
124–132 ↗(On Diff #31291)
This revision now requires changes to proceed.Sep 20 2023, 5:52 AM
ashoat requested changes to this revision.Oct 6 2023, 10:52 AM

Copy-pasting from my last review:

While accountDeletionResponder will need to support a request for a while (for old clients), deleteAccount is downstream and should be abstracted from that

keyserver/src/endpoints.js
286 ↗(On Diff #31727)

This change will break all existing clients!!

This revision now requires changes to proceed.Oct 6 2023, 10:52 AM
varun requested review of this revision.Oct 6 2023, 11:04 AM

From my updated test plan:

undid the modification to user-actions.js and confirmed that older clients that send a password can still delete accounts successfully.

Am I missing something? It seems like account deletion still works fine on existing clients after these changes

Ah, my bad... I expected to see the old validator still there, and was surprised to see ignoredArgumentValidator (which I wasn't familiar with). After examining it more closely, it looks like it does what I probably should've concluded it does from the name – ignores all arguments

This revision is now accepted and ready to land.Oct 6 2023, 12:46 PM