Add new one-time-keys table for both content and notif keys.
https://linear.app/comm/issue/ENG-4535
Depends on D8642
Details
Details
- Reviewers
bartek varun - Commits
- rCOMMbdabed9dccb1: [Identity] Add DDB table for one-time-keys
cd services/terraform/dev ./run.sh
The two identity-*-onetime-keys should be created.
The constants in service/identity/ are tested later
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Not [yet] an expert when it comes to DDB batch writes so leaving this part to @varun
Besides that, LGTM
| services/identity/src/database.rs | ||
|---|---|---|
| 317–318 ↗ | (On Diff #29510) | I think this is the same concern as @varun had in https://phab.comm.dev/D8452#250578 - single batch is limited to 25 items and we need to split into chunks - he did this in D8631 |
| services/identity/src/ddb_utils.rs | ||
| 28–29 ↗ | (On Diff #29510) | This comment is no longer relevant, since we have only one table now |
| 30 ↗ | (On Diff #29510) | I like this usage of local imports ;) |
| services/identity/src/database.rs | ||
|---|---|---|
| 317–318 ↗ | (On Diff #29510) | yep batchWriteItem has limits on size/number of items that we need to adhere to |
| services/identity/src/database.rs | ||
|---|---|---|
| 378 ↗ | (On Diff #29631) | we need to follow the BatchWriteItem limit here, too, right? |
Comment Actions
Chunkify other BatchWriteItem
| services/identity/src/database.rs | ||
|---|---|---|
| 378 ↗ | (On Diff #29631) | yea, my bad. New devices should only be writing 20 items, and the refreshKeysRequest requests 10. So it shouldn't cause an issue. However, better to be safe than sorry. |
Comment Actions
sorry I didn't catch this earlier but we should be consistent about onetime vs one-time/oneTime/OneTime/one_time. the latter matches the white paper, so we should remove all uses of the former. i noted a couple instances where we're using the wrong format inline, but it's not an exhaustive list. some other comments inline too.
also, we have to update the add_user_to_users_table function, right?
| services/commtest/tests/identity_onetime_key_tests.rs | ||
|---|---|---|
| 18 | identity | |
| 35 | fwiw i feel like a full test would also check that we can retrieve the one-time keys as well. | |
| services/identity/src/constants.rs | ||
| 91 | one-time | |
| services/terraform/modules/shared/dynamodb.tf | ||
| 257 | i think it should be one-time | |
| 258 | one-time | |
Comment Actions
Sigh, I was going off the naming you had in constants.rs. Do you mind if I just create a follow up task to rename all instances of onetime/ONETIME to one-time/ONE_TIME. I really don't want to deal with those kinds of rebase conflicts in the current stack....
Comment Actions
Removing this here means that add_user_... method needs to be updated with adding the otk's to the related table
sorry to keep requesting changes but i'm still not seeing this change in the diff
| services/identity/src/constants.rs | ||
|---|---|---|
| 93 | are you sure we want to remove this comment? | |
| services/identity/src/constants.rs | ||
|---|---|---|
| 93 | Another victim of the bad patch. | |
Comment Actions
can you please create a follow up task for renaming affected db methods? e.g. add_user_to_users_table -> add_user_to_ddb, add_password_user_to_users_table -> add_password_user_to_ddb, add_device_to_users_table -> add_device_to_ddb (these are just suggestions, i'm open to other names)
| services/identity/src/constants.rs | ||
|---|---|---|
| 94–97 ↗ | (On Diff #30028) | nit on comment location and PARTITION_KEY naming |