tl;dr we neeed an optional force param so that we can determine when a user is intentionally replacing their existing keyserver with a new one
Details
Details
logged in to local identity from local keyserver. then tried logging in from docker keyserver with the same credentials -- this failed. then tried logging in again from the docker keyserver with the same credentials AND force set to true -- this succeeded. The old keyserver device id was removed from DDB and the new one was added
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/commtest/tests/identity_device_list_tests.rs | ||
|---|---|---|
| 219 ↗ | (On Diff #37943) | should we avoid generating this extra update? it occurs because we remove keyserver 1 and add keyserver 2 in separate transactions in log_in_password_user_finish |
| services/identity/src/client_service.rs | ||
| 360–378 ↗ | (On Diff #37943) | related to the question above -- can/should these be a single transaction? |
| services/commtest/tests/identity_device_list_tests.rs | ||
|---|---|---|
| 219 ↗ | (On Diff #37943) | Good question - it depends on how we understand Whitepaper section 5.2.3:
If we can do one addition OR one deletion, there it should be two transactions. But if the above means that a device can be replaced, then I think it should be done in a single transaction. However, if this RPC is going to be deprecated, and replaced by a primary-device-managed device list, then we don't have to care much about this now. Identity service is managing the device list only temporarily |
| services/identity/src/client_service.rs | ||
| 360–378 ↗ | (On Diff #37943) | Not in this way. |
| services/commtest/tests/identity_device_list_tests.rs | ||
|---|---|---|
| 219 ↗ | (On Diff #37943) |
i think this is correct. probably not worth spending time making keyserver replacement a single device list update, then. what do you think, @ashoat ? |
| services/commtest/tests/identity_device_list_tests.rs | ||
|---|---|---|
| 219 ↗ | (On Diff #37943) | In the eventual system, it's actually important that this is one transaction. It can't be validated as two transactions, since in the first transaction the old primary would be removed, and there should be a new primary signing the device list, but the new primary isn't there yet. In the current system, we don't have signing so it doesn't seem that important. |
| services/commtest/src/identity/device.rs | ||
|---|---|---|
| 122 ↗ | (On Diff #37943) | It's pointless to create an option each time we call this |
| 155 ↗ | (On Diff #37943) | |
| services/commtest/tests/identity_device_list_tests.rs | ||
| 219 ↗ | (On Diff #37943) | As identity-managed device list updates aren't going to be signed, I think we can leave two transactions now |
| services/identity/src/client_service.rs | ||
| 307 ↗ | (On Diff #37943) | Instead of passing reference to option, can we simply unwrap_or(false)? |
| services/commtest/src/identity/device.rs | ||
|---|---|---|
| 122 ↗ | (On Diff #37943) | i actually prefer Option<bool> here because this only applies to keyservers, so it makes more sense to me to provide None when calling this function to log in, say, an iOS device. i don't feel too strongly, though, so i've made the change you requested |
| services/identity/src/client_service.rs | ||
| 360–378 ↗ | (On Diff #37943) | going to leave it as two transactions |