[identity] add user ID to account ownership message
ClosedPublic
Actions

Authored by varun on Oct 12 2023, 10:38 PM.

Details

Reviewers

bartek
michal
• jon

Commits

rCOMM7b1ed66674c8: [identity] add user ID to account ownership message

Summary

add user ID to the account ownership message. we use this user ID instead of minting a new one for existing users if they're already registered on keyserver but haven't yet registered with the identity service.

also added a condition expression to make sure we don't overwrite any user data accidentally.

Depends on D9480

Test Plan

successfully registered a user with my local identity service who was previously registered with my local keyserver.

called the add_user_to_users_table function twice with the same user ID to test the condition expression. second call failed as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Oct 12 2023, 10:38 PM

Herald added a reviewer: • jon. · View Herald TranscriptOct 12 2023, 10:39 PM

Herald added subscribers: will, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B23232: Diff 31988.Oct 12 2023, 10:55 PM

varun requested review of this revision.Oct 12 2023, 10:55 PM

bartek accepted this revision.Oct 16 2023, 1:01 AM

bartek added inline comments.

services/identity/src/database.rs
245–247 ↗	(On Diff #31988)	👍 By the way, we should look for other places where we are missing such checks.
services/identity/src/reserved_users.rs
24 ↗	(On Diff #31988)	To match our naming conventions

This revision is now accepted and ready to land.Oct 16 2023, 1:01 AM

michal added inline comments.Oct 16 2023, 2:44 AM

services/identity/src/reserved_users.rs
87 ↗	(On Diff #31988)	nit: this could be const

Will this require some sort of synced rollout between keyserver and identity service?

In D9482#279327, @ashoat wrote:

Will this require some sort of synced rollout between keyserver and identity service?

doesn't need to be perfectly in sync since we have the cron job that runs daily. i'll still let you know before i deploy a new version of the identity service

services/identity/src/database.rs
245–247 ↗	(On Diff #31988)	the other two places we call `put_item` are adding a nonce to the nonces table and adding an access token to the access token table. i think it's fine if we don't have condition expressions there. what do you think?

Closed by commit rCOMM7b1ed66674c8: [identity] add user ID to account ownership message (authored by varun). · Explain WhyOct 18 2023, 8:00 PM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMM7b1ed66674c8: [identity] add user ID to account ownership message.

bartek added inline comments.Oct 18 2023, 10:12 PM

services/identity/src/database.rs
245–247 ↗	(On Diff #31988)	Yeah that's okay. I mean we should check places where the checks are missing but it'd be recommended to have them based on the functionality. Adding a nonce doesn't look like something that requires them