Skip to content

Commit

Permalink
[identity] add user ID to account ownership message
Browse files Browse the repository at this point in the history
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.

Reviewers: bartek, michal, jon

Reviewed By: bartek

Subscribers: ashoat, tomek, wyilio

Differential Revision: https://phab.comm.dev/D9482
  • Loading branch information
vdhanan committed Oct 19, 2023
1 parent e69d485 commit 7b1ed66
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 19 deletions.
7 changes: 5 additions & 2 deletions keyserver/src/responders/user-responders.js
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,11 @@ async function claimUsernameResponder(

const issuedAt = new Date().toISOString();
const reservedUsernameMessage: ReservedUsernameMessage = {
statement: 'This user is the owner of the following username',
payload: username,
statement: 'This user is the owner of the following username and user ID',
payload: {
username,
userID: viewer.userID,
},
issuedAt,
};
const message = JSON.stringify(reservedUsernameMessage);
Expand Down
7 changes: 5 additions & 2 deletions lib/types/crypto-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,11 @@ export type ReservedUsernameMessage =
+issuedAt: string,
}
| {
+statement: 'This user is the owner of the following username',
+payload: string,
+statement: 'This user is the owner of the following username and user ID',
+payload: {
+username: string,
+userID: string,
},
+issuedAt: string,
};

Expand Down
19 changes: 11 additions & 8 deletions services/identity/src/client_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ use crate::grpc_utils::DeviceInfoWithAuth;
use crate::id::generate_uuid;
use crate::nonce::generate_nonce_data;
use crate::reserved_users::{
validate_account_ownership_message_and_get_user_id,
validate_add_reserved_usernames_message,
validate_remove_reserved_username_message,
validate_signed_account_ownership_message,
};
use crate::siwe::{is_valid_ethereum_address, parse_and_verify_siwe_message};
use crate::token::{AccessTokenData, AuthType};
Expand All @@ -61,6 +61,7 @@ pub enum WorkflowInProgress {
pub struct UserRegistrationInfo {
pub username: String,
pub flattened_device_key_upload: FlattenedDeviceKeyUpload,
pub user_id: Option<String>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -173,6 +174,7 @@ impl IdentityClientService for ClientService {
device_type: DeviceType::try_from(DBDeviceTypeInt(device_type))
.map_err(handle_db_error)?,
},
user_id: None,
};
let session_id = generate_uuid();
self
Expand Down Expand Up @@ -217,16 +219,16 @@ impl IdentityClientService for ClientService {
.username_in_reserved_usernames_table(&message.username)
.await
.map_err(handle_db_error)?;
if username_in_reserved_usernames_table {
validate_signed_account_ownership_message(
&message.username,
&message.keyserver_message,
&message.keyserver_signature,
)?;
} else {
if !username_in_reserved_usernames_table {
return Err(tonic::Status::permission_denied("username not reserved"));
}

let user_id = validate_account_ownership_message_and_get_user_id(
&message.username,
&message.keyserver_message,
&message.keyserver_signature,
)?;

if let client_proto::ReservedRegistrationStartRequest {
opaque_registration_request: register_message,
username,
Expand Down Expand Up @@ -276,6 +278,7 @@ impl IdentityClientService for ClientService {
device_type: DeviceType::try_from(DBDeviceTypeInt(device_type))
.map_err(handle_db_error)?,
},
user_id: Some(user_id),
};

let session_id = generate_uuid();
Expand Down
8 changes: 7 additions & 1 deletion services/identity/src/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ impl DatabaseClient {
Some((registration_state.username, Blob::new(password_file))),
None,
None,
registration_state.user_id,
)
.await
}
Expand All @@ -160,6 +161,7 @@ impl DatabaseClient {
None,
Some(wallet_address),
Some(social_proof),
None,
)
.await
}
Expand All @@ -170,8 +172,9 @@ impl DatabaseClient {
username_and_password_file: Option<(String, Blob)>,
wallet_address: Option<String>,
social_proof: Option<String>,
user_id: Option<String>,
) -> Result<String, Error> {
let user_id = generate_uuid();
let user_id = user_id.unwrap_or_else(generate_uuid);
let device_info =
create_device_info(flattened_device_key_upload.clone(), social_proof);
let devices = HashMap::from([(
Expand Down Expand Up @@ -212,6 +215,9 @@ impl DatabaseClient {
.put_item()
.table_name(USERS_TABLE)
.set_item(Some(user))
// make sure we don't accidentaly overwrite existing row
.condition_expression("attribute_not_exists(#pk)")
.expression_attribute_names("#pk", USERS_TABLE_PARTITION_KEY)
.send()
.await
.map_err(|e| Error::AwsSdk(e.into()))?;
Expand Down
25 changes: 19 additions & 6 deletions services/identity/src/reserved_users.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ struct Message<T> {
issued_at: String,
}

// This type should not be changed without making equivalent changes to
// `ReservedUsernameMessage` in lib/types/crypto-types.js
#[derive(Deserialize)]
#[serde(rename_all = "camelCase")]
struct UsernameAndID {
username: String,
#[serde(rename = "userID")]
user_id: String,
}

fn validate_and_decode_message<T: serde::de::DeserializeOwned>(
keyserver_message: &str,
keyserver_signature: &str,
Expand Down Expand Up @@ -69,22 +79,25 @@ fn validate_and_decode_message<T: serde::de::DeserializeOwned>(
Ok(deserialized_message)
}

pub fn validate_signed_account_ownership_message(
pub fn validate_account_ownership_message_and_get_user_id(
username: &str,
keyserver_message: &str,
keyserver_signature: &str,
) -> Result<(), Status> {
let deserialized_message = validate_and_decode_message::<String>(
) -> Result<String, Status> {
const EXPECTED_STATEMENT: &[u8; 60] =
b"This user is the owner of the following username and user ID";

let deserialized_message = validate_and_decode_message::<UsernameAndID>(
keyserver_message,
keyserver_signature,
b"This user is the owner of the following username",
EXPECTED_STATEMENT,
)?;

if deserialized_message.payload != username {
if deserialized_message.payload.username != username {
return Err(Status::invalid_argument("message invalid"));
}

Ok(())
Ok(deserialized_message.payload.user_id)
}

pub fn validate_add_reserved_usernames_message(
Expand Down

0 comments on commit 7b1ed66

Please sign in to comment.