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.
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
- 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)
- 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) | |
Comment Actions
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 | This change will break all existing clients!! | |
Comment Actions
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
Comment Actions
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