diff --git a/src/frontend/src/lib/generated/internet_identity_idl.js b/src/frontend/src/lib/generated/internet_identity_idl.js index 45ac298b82..32109def41 100644 --- a/src/frontend/src/lib/generated/internet_identity_idl.js +++ b/src/frontend/src/lib/generated/internet_identity_idl.js @@ -387,6 +387,14 @@ export const idlFactory = ({ IDL }) => { 'expiration' : Timestamp, 'anchor_number' : UserNumber, }); + const PrepareAccountDelegation = IDL.Record({ + 'user_key' : UserKey, + 'timestamp' : Timestamp, + }); + const AccountDelegationError = IDL.Variant({ + 'InternalCanisterError' : IDL.Text, + 'Unauthorized' : IDL.Principal, + }); const PrepareIdAliasRequest = IDL.Record({ 'issuer' : FrontendHostname, 'relying_party' : FrontendHostname, @@ -653,7 +661,12 @@ export const idlFactory = ({ IDL }) => { SessionKey, IDL.Opt(IDL.Nat64), ], - [UserKey, Timestamp], + [ + IDL.Variant({ + 'Ok' : PrepareAccountDelegation, + 'Err' : AccountDelegationError, + }), + ], [], ), 'prepare_delegation' : IDL.Func( diff --git a/src/frontend/src/lib/generated/internet_identity_types.d.ts b/src/frontend/src/lib/generated/internet_identity_types.d.ts index 27eda75223..b0c0a1a5e7 100644 --- a/src/frontend/src/lib/generated/internet_identity_types.d.ts +++ b/src/frontend/src/lib/generated/internet_identity_types.d.ts @@ -2,6 +2,8 @@ import type { Principal } from '@dfinity/principal'; import type { ActorMethod } from '@dfinity/agent'; import type { IDL } from '@dfinity/candid'; +export type AccountDelegationError = { 'InternalCanisterError' : string } | + { 'Unauthorized' : Principal }; export interface AccountInfo { 'name' : [] | [string], 'origin' : string, @@ -302,6 +304,10 @@ export interface OpenIdPrepareDelegationResponse { 'expiration' : Timestamp, 'anchor_number' : UserNumber, } +export interface PrepareAccountDelegation { + 'user_key' : UserKey, + 'timestamp' : Timestamp, +} export type PrepareIdAliasError = { 'InternalCanisterError' : string } | { 'Unauthorized' : Principal }; export interface PrepareIdAliasRequest { @@ -524,7 +530,8 @@ export interface _SERVICE { SessionKey, [] | [bigint], ], - [UserKey, Timestamp] + { 'Ok' : PrepareAccountDelegation } | + { 'Err' : AccountDelegationError } >, 'prepare_delegation' : ActorMethod< [UserNumber, FrontendHostname, SessionKey, [] | [bigint]], diff --git a/src/internet_identity/internet_identity.did b/src/internet_identity/internet_identity.did index c80a1b9dfc..b725261f8d 100644 --- a/src/internet_identity/internet_identity.did +++ b/src/internet_identity/internet_identity.did @@ -571,6 +571,16 @@ type UpdateAccountError = variant { Unauthorized : principal; }; +type AccountDelegationError = variant { + InternalCanisterError : text; + Unauthorized : principal; +}; + +type PrepareAccountDelegation = record { + user_key : UserKey; + timestamp : Timestamp; +}; + type GetAccountsError = variant { InternalCanisterError : text; Unauthorized : principal; @@ -873,7 +883,7 @@ service : (opt InternetIdentityInit) -> { account_number : opt AccountNumber, // Null is unreserved default account session_key : SessionKey, max_ttl : opt nat64 - ) -> (UserKey, Timestamp); + ) -> (variant { Ok : PrepareAccountDelegation; Err : AccountDelegationError }); get_account_delegation : ( anchor_number : UserNumber, diff --git a/src/internet_identity/src/account_management.rs b/src/internet_identity/src/account_management.rs index 922b75365c..6281457066 100644 --- a/src/internet_identity/src/account_management.rs +++ b/src/internet_identity/src/account_management.rs @@ -1,53 +1,33 @@ use crate::{ - state::{storage_borrow, storage_borrow_mut}, + delegation::{ + add_delegation_signature, check_frontend_length, delegation_bookkeeping, + der_encode_canister_sig_key, + }, + ii_domain::IIDomain, + state::{self, storage_borrow, storage_borrow_mut}, storage::{ account::{ - Account, AccountReference, AccountsCounter, CreateAccountParams, ReadAccountParams, - UpdateAccountParams, + Account, AccountDelegationError, AccountsCounter, CreateAccountParams, + PrepareAccountDelegation, ReadAccountParams, UpdateAccountParams, }, StorageError, }, + update_root_hash, }; -use ic_cdk::caller; +use ic_cdk::{api::time, caller}; use internet_identity_interface::internet_identity::types::{ - AccountNumber, AccountUpdate, AnchorNumber, CreateAccountError, FrontendHostname, + AccountNumber, AccountUpdate, AnchorNumber, CreateAccountError, FrontendHostname, SessionKey, UpdateAccountError, }; +use serde_bytes::ByteBuf; const MAX_ANCHOR_ACCOUNTS: usize = 500; -pub fn anchor_has_account( - anchor_number: AnchorNumber, - origin: &FrontendHostname, - account_number: Option, -) -> Option { - // check if anchor has acc - storage_borrow(|storage| { - storage - .lookup_application_number_with_origin(origin) - .and_then(|application_number| { - storage.has_account_reference(anchor_number, application_number, account_number) - }) - }) -} - pub fn get_accounts_for_origin( anchor_number: AnchorNumber, origin: &FrontendHostname, ) -> Vec { - storage_borrow(|storage| { - storage - .list_accounts(anchor_number, origin) - .iter() - .filter_map(|acc_ref| { - storage.read_account(ReadAccountParams { - account_number: acc_ref.account_number, - anchor_number, - origin, - }) - }) - .collect() - }) + storage_borrow(|storage| storage.list_accounts(anchor_number, origin)) } pub fn create_account_for_origin( @@ -81,10 +61,6 @@ pub fn update_account_for_origin( origin: FrontendHostname, update: AccountUpdate, ) -> Result { - // If the anchor doesn't own this account, we return unauthorized. - if anchor_has_account(anchor_number, &origin, account_number).is_none() { - return Err(UpdateAccountError::Unauthorized(caller())); - } match update.name { Some(name) => storage_borrow_mut(|storage| { // If the account to be updated is a default account @@ -96,6 +72,7 @@ pub fn update_account_for_origin( stored_account_references: _, } = storage.get_account_counter(anchor_number); + // TODO: also check the actual number and reset if inaccurate if stored_accounts >= MAX_ANCHOR_ACCOUNTS as u64 { return Err(UpdateAccountError::AccountLimitReached); } @@ -127,6 +104,47 @@ pub fn update_account_for_origin( } } +pub async fn prepare_account_delegation( + anchor_number: AnchorNumber, + origin: FrontendHostname, + account_number: Option, + session_key: SessionKey, + max_ttl: Option, + ii_domain: &Option, +) -> Result { + state::ensure_salt_set().await; + check_frontend_length(&origin); + + let account = storage_borrow(|storage| { + storage + .read_account(ReadAccountParams { + account_number, + anchor_number, + origin: &origin, + }) + .ok_or(AccountDelegationError::Unauthorized(caller())) + })?; + + let session_duration_ns = u64::min( + max_ttl.unwrap_or(crate::delegation::DEFAULT_EXPIRATION_PERIOD_NS), + crate::delegation::MAX_EXPIRATION_PERIOD_NS, + ); + let expiration = time().saturating_add(session_duration_ns); + let seed = account.calculate_seed(); + + state::signature_map_mut(|sigs| { + add_delegation_signature(sigs, session_key, seed.as_ref(), expiration); + }); + update_root_hash(); + + delegation_bookkeeping(origin, ii_domain.clone(), session_duration_ns); + + Ok(PrepareAccountDelegation { + user_key: ByteBuf::from(der_encode_canister_sig_key(seed.to_vec())), + timestamp: expiration, + }) +} + #[test] fn should_create_account_for_origin() { use crate::state::{storage_borrow_mut, storage_replace}; @@ -140,13 +158,14 @@ fn should_create_account_for_origin() { assert_eq!( create_account_for_origin(anchor.anchor_number(), origin.clone(), name.clone()), - Ok(Account { - account_number: Some(1), - anchor_number: anchor.anchor_number(), + Ok(Account::new_full( + anchor.anchor_number(), origin, - last_used: None, - name: Some(name) - }) + Some(name), + Some(1), + None, + None + )) ); } @@ -212,27 +231,23 @@ fn should_get_accounts_for_origin() { assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created + Account::new(anchor_number, origin.clone(), None, None), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(1), + origin.clone(), + Some("Alice".to_string()), + Some(1), + None, + None + ), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Alice".to_string()) - }, - Account { - account_number: Some(2), - anchor_number, - origin, - last_used: None, - name: Some("Bob".to_string()) - } + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ) } @@ -258,40 +273,30 @@ fn should_only_get_own_accounts_for_origin() { assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created + Account::new(anchor_number, origin.clone(), None, None), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(1), // because of how allocate_account_number is implemented, this starts at 1 - anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Alice".to_string()) - }, + origin.clone(), + Some("Alice".to_string()), + Some(1), + None, + None + ), ] ); assert_eq!( get_accounts_for_origin(anchor_number_two, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created - anchor_number: anchor_number_two, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(2), - anchor_number: anchor_number_two, - origin, - last_used: None, - name: Some("Bob".to_string()) - } + Account::new(anchor_number_two, origin.clone(), None, None), + Account::new_full( + anchor_number_two, + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ) } @@ -315,27 +320,23 @@ fn should_update_account_for_origin() { assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created + Account::new(anchor_number, origin.clone(), None, None), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(1), + origin.clone(), + Some("Alice".to_string()), + Some(1), + None, + None + ), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Alice".to_string()) - }, - Account { - account_number: Some(2), - anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Bob".to_string()) - } + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ); @@ -348,39 +349,36 @@ fn should_update_account_for_origin() { name: Some("Becky".to_string()) } ), - Ok(Account { - account_number: Some(1), + Ok(Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Becky".to_string()) - }) + origin.clone(), + Some("Becky".to_string()), + Some(1), + None, + None + )) ); assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created + Account::new(anchor_number, origin.clone(), None, None), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(1), - anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Becky".to_string()) - }, - Account { - account_number: Some(2), + origin.clone(), + Some("Becky".to_string()), + Some(1), + None, + None + ), + Account::new_full( anchor_number, - origin, - last_used: None, - name: Some("Bob".to_string()) - } + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ); } @@ -404,27 +402,23 @@ fn should_update_default_account_for_origin() { assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: None, // default account gets created when additional account gets created - anchor_number, - origin: origin.clone(), - last_used: None, - name: None - }, - Account { - account_number: Some(1), + Account::new(anchor_number, origin.clone(), None, None), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Alice".to_string()) - }, - Account { - account_number: Some(2), + origin.clone(), + Some("Alice".to_string()), + Some(1), + None, + None + ), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Bob".to_string()) - } + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ); @@ -437,39 +431,43 @@ fn should_update_default_account_for_origin() { name: Some("Becky".to_string()) } ), - Ok(Account { - account_number: Some(3), + Ok(Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Becky".to_string()) - }) + origin.clone(), + Some("Becky".to_string()), + Some(3), + None, + Some(anchor_number) + )) ); assert_eq!( get_accounts_for_origin(anchor_number, &origin), vec![ - Account { - account_number: Some(3), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Becky".to_string()) - }, - Account { - account_number: Some(1), + origin.clone(), + Some("Becky".to_string()), + Some(3), + None, + Some(anchor_number) + ), + Account::new_full( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some("Alice".to_string()) - }, - Account { - account_number: Some(2), + origin.clone(), + Some("Alice".to_string()), + Some(1), + None, + None + ), + Account::new_full( anchor_number, - origin, - last_used: None, - name: Some("Bob".to_string()) - } + origin.clone(), + Some("Bob".to_string()), + Some(2), + None, + None + ), ] ); } diff --git a/src/internet_identity/src/delegation.rs b/src/internet_identity/src/delegation.rs index b0f835bb9d..a452f62bf5 100644 --- a/src/internet_identity/src/delegation.rs +++ b/src/internet_identity/src/delegation.rs @@ -2,13 +2,12 @@ use crate::ii_domain::IIDomain; use crate::stats::event_stats::{ update_event_based_stats, Event, EventData, PrepareDelegationEvent, }; -use crate::{state, update_root_hash, DAY_NS, MINUTE_NS}; +use crate::{state, DAY_NS, MINUTE_NS}; use candid::Principal; use ic_canister_sig_creation::signature_map::{CanisterSigInputs, SignatureMap}; use ic_canister_sig_creation::{ delegation_signature_msg, CanisterSigPublicKey, DELEGATION_SIG_DOMAIN, }; -use ic_cdk::api::time; use ic_cdk::{id, trap}; use ic_certification::Hash; use internet_identity_interface::internet_identity::types::*; @@ -18,44 +17,14 @@ use std::net::IpAddr; // The expiration used for delegations if none is specified // (calculated as now() + this) -const DEFAULT_EXPIRATION_PERIOD_NS: u64 = 30 * MINUTE_NS; +pub const DEFAULT_EXPIRATION_PERIOD_NS: u64 = 30 * MINUTE_NS; // The maximum expiration time for delegation // (calculated as now() + this) -const MAX_EXPIRATION_PERIOD_NS: u64 = 30 * DAY_NS; - -pub async fn prepare_delegation( - anchor_number: AnchorNumber, - frontend: FrontendHostname, - session_key: SessionKey, - max_time_to_live: Option, - ii_domain: &Option, -) -> (UserKey, Timestamp) { - state::ensure_salt_set().await; - check_frontend_length(&frontend); - - let session_duration_ns = u64::min( - max_time_to_live.unwrap_or(DEFAULT_EXPIRATION_PERIOD_NS), - MAX_EXPIRATION_PERIOD_NS, - ); - let expiration = time().saturating_add(session_duration_ns); - let seed = calculate_seed(anchor_number, &frontend); - - state::signature_map_mut(|sigs| { - add_delegation_signature(sigs, session_key, seed.as_ref(), expiration); - }); - update_root_hash(); - - delegation_bookkeeping(frontend, ii_domain.clone(), session_duration_ns); - - ( - ByteBuf::from(der_encode_canister_sig_key(seed.to_vec())), - expiration, - ) -} +pub const MAX_EXPIRATION_PERIOD_NS: u64 = 30 * DAY_NS; /// Update metrics and the list of latest front-end origins. -fn delegation_bookkeeping( +pub fn delegation_bookkeeping( frontend: FrontendHostname, ii_domain: Option, session_duration_ns: u64, @@ -107,7 +76,7 @@ pub fn get_delegation( state::assets_and_signatures(|certified_assets, sigs| { let inputs = CanisterSigInputs { domain: DELEGATION_SIG_DOMAIN, - seed: &calculate_seed(anchor_number, &frontend), + seed: &calculate_anchor_seed(anchor_number, &frontend), message: &delegation_signature_msg(&session_key, expiration, None), }; match sigs.get_signature_as_cbor(&inputs, Some(certified_assets.root_hash())) { @@ -127,12 +96,12 @@ pub fn get_delegation( pub fn get_principal(anchor_number: AnchorNumber, frontend: FrontendHostname) -> Principal { check_frontend_length(&frontend); - let seed = calculate_seed(anchor_number, &frontend); + let seed = calculate_anchor_seed(anchor_number, &frontend); let public_key = der_encode_canister_sig_key(seed.to_vec()); Principal::self_authenticating(public_key) } -fn calculate_seed(anchor_number: AnchorNumber, frontend: &FrontendHostname) -> Hash { +pub fn calculate_anchor_seed(anchor_number: AnchorNumber, frontend: &FrontendHostname) -> Hash { let salt = state::salt(); let mut blob: Vec = vec![]; @@ -150,6 +119,25 @@ fn calculate_seed(anchor_number: AnchorNumber, frontend: &FrontendHostname) -> H hash_bytes(blob) } +/// Calculate a seed only from an `AccountNumber`. +/// This is only called when we're not dealing with a default account. +/// Frontend origin and anchor number are not included because accounts are already stored per-origin and per-user. +/// Leaving them out allows us to potentially allow account transfer or sharing in the future. +pub fn calculate_account_seed(account_number: AccountNumber) -> Hash { + let salt = state::salt(); + + let mut blob: Vec = vec![]; + blob.push(salt.len() as u8); + blob.extend_from_slice(&salt); + + let account_number_str = account_number.to_string(); + let account_number_blob = account_number_str.bytes(); + blob.push(account_number_blob.len() as u8); + blob.extend(account_number_blob); + + hash_bytes(blob) +} + fn hash_bytes(value: impl AsRef<[u8]>) -> Hash { let mut hasher = Sha256::new(); hasher.update(value.as_ref()); diff --git a/src/internet_identity/src/main.rs b/src/internet_identity/src/main.rs index f45695c596..25cd8bc274 100644 --- a/src/internet_identity/src/main.rs +++ b/src/internet_identity/src/main.rs @@ -28,6 +28,7 @@ use internet_identity_interface::internet_identity::types::vc_mvp::{ use internet_identity_interface::internet_identity::types::*; use serde_bytes::ByteBuf; use std::collections::HashMap; +use storage::account::{AccountDelegationError, PrepareAccountDelegation}; use storage::{Salt, Storage}; mod account_management; @@ -275,14 +276,23 @@ async fn prepare_delegation( ) -> (UserKey, Timestamp) { let ii_domain = check_authz_and_record_activity(anchor_number) .unwrap_or_else(|err| trap(&format!("{err}"))); - delegation::prepare_delegation( + + account_management::prepare_account_delegation( anchor_number, frontend, + None, session_key, max_time_to_live, &ii_domain, ) .await + .map( + |PrepareAccountDelegation { + user_key, + timestamp, + }| (user_key, timestamp), + ) + .unwrap_or_else(|err| trap(&format!("{:?}", err))) } #[query] @@ -350,14 +360,27 @@ fn update_account( } #[update] -fn prepare_account_delegation( - _anchor_number: AnchorNumber, - _origin: FrontendHostname, - _account_number: Option, - _session_key: SessionKey, - _max_ttl: Option, -) -> (UserKey, Timestamp) { - (ByteBuf::new(), 0) +async fn prepare_account_delegation( + anchor_number: AnchorNumber, + origin: FrontendHostname, + account_number: Option, + session_key: SessionKey, + max_ttl: Option, +) -> Result { + match check_authz_and_record_activity(anchor_number) { + Ok(ii_domain) => { + account_management::prepare_account_delegation( + anchor_number, + origin, + account_number, + session_key, + max_ttl, + &ii_domain, + ) + .await + } + Err(err) => Err(err.into()), + } } #[query] diff --git a/src/internet_identity/src/storage.rs b/src/internet_identity/src/storage.rs index c636cb86fd..f3e77171e6 100644 --- a/src/internet_identity/src/storage.rs +++ b/src/internet_identity/src/storage.rs @@ -735,21 +735,21 @@ impl Storage { .map(|list| list.into()) } - pub fn has_account_reference( + fn find_account_reference( &self, anchor_number: AnchorNumber, - application_number: ApplicationNumber, + application_number: Option, account_number: Option, ) -> Option { - self.stable_account_reference_list_memory - .get(&(anchor_number, application_number)) - .map(|acc_ref_list| acc_ref_list.into_acc_ref_vec()) - .and_then(|acc_ref_vec| { - acc_ref_vec - .iter() - .find(|acc_ref| acc_ref.account_number == account_number) - .cloned() - }) + application_number.and_then(|app_num| { + self.lookup_account_references(anchor_number, app_num) + .and_then(|acc_ref_vec| { + acc_ref_vec + .iter() + .find(|acc_ref| acc_ref.account_number == account_number) + .cloned() + }) + }) } /// Updates the anchor account, application and account counters. @@ -833,7 +833,6 @@ impl Storage { .collect() } - #[allow(dead_code)] pub fn create_additional_account( &mut self, params: CreateAccountParams, @@ -895,64 +894,136 @@ impl Storage { } // Return the new account - Ok(Account { - account_number: Some(account_number), + Ok(Account::new( anchor_number, - origin: origin.clone(), - last_used: None, - name: Some(params.name), - }) + origin.to_string(), + Some(params.name), + Some(account_number), + )) } #[allow(dead_code)] - /// Returns a list of account references for a given anchor and application. - /// If the application doesn't exist, returns a list with a default account reference. - /// If the account references doesn't exist, returns a list with a default account reference. + /// Returns a list of accounts for a given anchor and application. + /// If the application doesn't exist, returns a list with a synthetic default account. + /// If the account references don't exist, returns a list with a synthetic default account. pub fn list_accounts( &self, anchor_number: AnchorNumber, origin: &FrontendHostname, - ) -> Vec { + ) -> Vec { match self.lookup_application_number_with_origin(origin) { - None => vec![AccountReference { - account_number: None, - last_used: None, - }], + None => vec![Account::new(anchor_number, origin.clone(), None, None)], Some(app_num) => match self.lookup_account_references(anchor_number, app_num) { - None => vec![AccountReference { - account_number: None, - last_used: None, - }], - Some(refs) => refs, + None => vec![Account::new(anchor_number, origin.clone(), None, None)], + Some(refs) => refs + .iter() + .filter_map(|acc_ref| { + self.read_account(ReadAccountParams { + account_number: acc_ref.account_number, + anchor_number, + origin, + }) + }) + .collect(), }, } } - #[allow(dead_code)] - /// Returns the requested account. - /// If the account number doesn't esist, returns a default Account. - /// If the account number exists but the account doesn't exist, returns None. - /// If the account exists, returns it as Account. + /// Returns the requested `Account`. + /// If the anchor doesn't own this `Account`, returns None. + /// If the `Account` is default but has been moved/deleted, returns None. + /// If the `Account` is default but ALL `Account`s for this origin have been moved or deleted, returns a default `Account`. + /// If the `Account` number doesn't esist, returns a default `Account`. + /// If the `Account` number exists but the `Account` doesn't exist, returns None. + /// If the `Account` exists, returns it as `Account`. pub fn read_account(&self, params: ReadAccountParams) -> Option { + let application_number = self.lookup_application_number_with_origin(params.origin); + match params.account_number { + // If a default account is requested None => { - // Application number doesn't exist, return a default Account - Some(Account::new( - params.anchor_number, - params.origin.clone(), - // Default accounts have no name - None, - params.account_number, - )) + // if there is no stored application, return a synthetic default account + if application_number.is_none() { + return Some(Account::new( + params.anchor_number, + params.origin.clone(), + None, + None, + )); + } + // check if there is a stored account reference list + if let Some(acc_ref_vec) = + // we can safely unwrap here + self.lookup_account_references( + params.anchor_number, + application_number.unwrap(), + ) + { + // if the list exists but is empty, we should still return a synthetic default account + // this should only happen if a named account was created, and then both it and the + // default account references were moved/deleted. + // XXX WARNING: this is done for the case that a user might have moved/deleted a default account + // and then reached the maximum accounts limit. If we don't return a synthetic default account here, + // they would be locked out of their account. + // However: if we implement account transfers at some point, and default accounts can be transfered, + // this would allow a user to regain access to their transferred default account. + if acc_ref_vec.is_empty() { + return Some(Account::new( + params.anchor_number, + params.origin.clone(), + None, + None, + )); + } + + // if there is a default account in the list, we return it + // else we return None, account has been moved or deleted + // but there is another account in the list, so user can log in with that + acc_ref_vec + .iter() + .find(|acc_ref| acc_ref.account_number.is_none()) + .map(|acc_ref| { + Account::new_with_last_used( + params.anchor_number, + params.origin.clone(), + None, + acc_ref.account_number, + acc_ref.last_used, + ) + }) + } else { + //if there is no list, we return a synthetic default account + Some(Account::new( + params.anchor_number, + params.origin.clone(), + None, + None, + )) + } } + // if a named/stored account is requested Some(account_number) => match self.stable_account_memory.get(&account_number) { + // if it does not exist, return None None => None, - Some(storable_account) => Some(Account::new( - params.anchor_number, - params.origin.clone(), - Some(storable_account.name.clone()), - Some(account_number), - )), + Some(storable_account) => { + // if it does exist, check whether it is owned by the caller anchor + // and belongs to the correct origin + self.find_account_reference( + params.anchor_number, + application_number, + params.account_number, + ) + .map(|acc_ref| { + Account::new_full( + params.anchor_number, + params.origin.clone(), + Some(storable_account.name.clone()), + Some(account_number), + acc_ref.last_used, + storable_account.seed_from_anchor, + ) + }) + } }, } } @@ -960,7 +1031,6 @@ impl Storage { /// Updates an account. /// If the account number exists, then updates that account. /// If the account number doesn't exist, then gets or creates an application and creates and stores a default account. - #[allow(dead_code)] pub fn update_account( &mut self, params: UpdateAccountParams, @@ -982,7 +1052,6 @@ impl Storage { } } - #[allow(dead_code)] /// Used in `update_account` to update an existing account. fn update_existing_account( &mut self, @@ -1002,7 +1071,6 @@ impl Storage { } } - #[allow(dead_code)] /// Used in `update_account` to create a default account. /// Default account are not initially stored. They are stored when updated. /// If the default account reference does not exist, it must be created. diff --git a/src/internet_identity/src/storage/account.rs b/src/internet_identity/src/storage/account.rs index 11f843370e..b477c4110c 100644 --- a/src/internet_identity/src/storage/account.rs +++ b/src/internet_identity/src/storage/account.rs @@ -1,11 +1,16 @@ -use candid::CandidType; +use candid::{CandidType, Principal}; + +use ic_cdk::trap; +use ic_certification::Hash; use ic_stable_structures::{storable::Bound, Storable}; use internet_identity_interface::internet_identity::types::{ - AccountInfo, AccountNumber, AnchorNumber, FrontendHostname, Timestamp, + AccountInfo, AccountNumber, AnchorNumber, FrontendHostname, Timestamp, UserKey, }; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use std::borrow::Cow; +use crate::{authz_utils::IdentityUpdateError, delegation}; + #[cfg(test)] mod tests; @@ -124,6 +129,7 @@ pub struct Account { pub origin: FrontendHostname, pub last_used: Option, pub name: Option, + seed_from_anchor: Option, } impl Account { @@ -139,9 +145,49 @@ impl Account { origin, last_used: None, name, + seed_from_anchor: None, + } + } + + pub fn new_with_last_used( + anchor_number: AnchorNumber, + origin: FrontendHostname, + name: Option, + account_number: Option, + last_used: Option, + ) -> Account { + Self { + account_number, + anchor_number, + origin, + last_used, + name, + seed_from_anchor: None, } } + pub fn new_full( + anchor_number: AnchorNumber, + origin: FrontendHostname, + name: Option, + account_number: Option, + last_used: Option, + seed_from_anchor: Option, + ) -> Account { + Self { + account_number, + anchor_number, + origin, + last_used, + name, + seed_from_anchor, + } + } + + fn get_seed_anchor(&self) -> Option { + self.seed_from_anchor + } + // Used in tests (for now) #[allow(dead_code)] pub fn to_reference(&self) -> AccountReference { @@ -159,4 +205,55 @@ impl Account { name: self.name.clone(), } } + + /// Create `Hash` used for a delegation that can make calls on behalf of an `Account`. + /// If the `Account` is a non-stored default account or has a `seed_from_anchor` (and thus is a stored default account), + /// the respective anchor number will be used as a seed input. Otherwise, the `AccountNumber` is used. + /// + /// # Arguments + /// + /// * `account` is the `Account` we're using for this delegation + pub fn calculate_seed(&self) -> Hash { + // If this is a non-stored default account, we derive from frontend and anchor + if self.account_number.is_none() { + return delegation::calculate_anchor_seed(self.anchor_number, &self.origin); + } + + match (self.get_seed_anchor(), self.account_number) { + (Some(seed_from_anchor), _) => { + // If this is a stored default account, we derive from frontend and anchor + delegation::calculate_anchor_seed(seed_from_anchor, &self.origin) + } + (None, Some(account_number)) => { + // If this is an added account, we derive from the account number. + delegation::calculate_account_seed(account_number) + } + (None, None) => trap("Attempted to calculate an account seed from an account without seed anchor or anchor number - this should never happen!") + } + } +} + +#[derive(CandidType, Debug, Serialize, Deserialize)] +pub enum AccountDelegationError { + Unauthorized(Principal), + InternalCanisterError(String), +} + +impl From for AccountDelegationError { + fn from(err: IdentityUpdateError) -> Self { + match err { + IdentityUpdateError::Unauthorized(principal) => { + AccountDelegationError::Unauthorized(principal) + } + IdentityUpdateError::StorageError(_identity_number, storage_error) => { + AccountDelegationError::InternalCanisterError(storage_error.to_string()) + } + } + } +} + +#[derive(CandidType, Serialize)] +pub struct PrepareAccountDelegation { + pub user_key: UserKey, + pub timestamp: Timestamp, } diff --git a/src/internet_identity/src/storage/account/tests.rs b/src/internet_identity/src/storage/account/tests.rs index 50413b376e..e6d60a4348 100644 --- a/src/internet_identity/src/storage/account/tests.rs +++ b/src/internet_identity/src/storage/account/tests.rs @@ -1,4 +1,4 @@ -use crate::storage::account::{Account, AccountReference}; +use crate::storage::account::Account; use crate::storage::application::Application; use crate::storage::{CreateAccountParams, ReadAccountParams, UpdateAccountParams}; use crate::Storage; @@ -66,6 +66,7 @@ fn should_create_additional_account() { origin: origin.clone(), name: Some(account_name.clone()), last_used: None, + seed_from_anchor: None, }; assert_eq!(additional_account, expected_account); assert_eq!( @@ -119,14 +120,9 @@ fn should_list_accounts() { origin: origin.clone(), name: account_name.clone(), }; - let expected_additional_account_ref = AccountReference { - account_number: Some(1), - last_used: None, - }; - let expected_default_account_ref = AccountReference { - account_number: None, - last_used: None, - }; + let expected_additional_account = + Account::new(anchor_number, origin.clone(), Some(account_name), Some(1)); + let expected_default_account = Account::new(anchor_number, origin.clone(), None, None); storage.create_additional_account(new_account).unwrap(); // 5. List accounts returns default account @@ -139,11 +135,11 @@ fn should_list_accounts() { "Expected exactly two accounts to be listed" ); assert_eq!( - listed_accounts[0], expected_default_account_ref, + listed_accounts[0], expected_default_account, "Default account reference is missing from the listed accounts." ); assert_eq!( - listed_accounts[1], expected_additional_account_ref, + listed_accounts[1], expected_additional_account, "Additional account reference is missing from the listed accounts." ); assert_eq!( @@ -241,10 +237,7 @@ fn should_update_default_account() { // 2. Default account exists withuot creating it let initial_accounts = storage.list_accounts(anchor_number, &origin); - let expected_unreserved_account = AccountReference { - account_number: None, - last_used: None, - }; + let expected_unreserved_account = Account::new(anchor_number, origin.clone(), None, None); assert_eq!(initial_accounts, vec![expected_unreserved_account]); // 3. Update default account @@ -258,10 +251,14 @@ fn should_update_default_account() { // 4. Check that the default account has been created with the updated values. let updated_accounts = storage.list_accounts(anchor_number, &origin); - let expected_updated_account = AccountReference { - account_number: Some(new_account_number), - last_used: None, - }; + let expected_updated_account = Account::new_full( + anchor_number, + origin, + Some(account_name), + Some(new_account_number), + None, + Some(anchor_number), + ); assert_eq!(updated_accounts, vec![expected_updated_account]); assert_eq!( storage.get_account_counter(anchor_number), @@ -346,6 +343,7 @@ fn should_update_additional_account() { origin: origin.clone(), last_used: None, name: Some(new_account_name), + seed_from_anchor: None, }; assert_eq!(updated_account, expected_updated_account); assert_eq!( @@ -498,3 +496,73 @@ fn should_count_accounts_different_anchors() { "Total counters after anchor 2 additional account mismatch" ); } + +// XXX WARNING: this functionality exists for the case that a user might have moved/deleted a default account +// and then reached the maximum accounts limit. If we don't return a synthetic default account here, +// they would be locked out of their account. +// However: if we implement account transfers at some point, and default accounts can be transfered, +// this would allow a user to regain access to their transferred default account. +#[test] +fn should_read_default_account_with_empty_reference_list() { + // Setup storage + let memory = VectorMemory::default(); + let mut storage = Storage::new((10_000, 3_784_873), memory); + + // 1. Define parameters + let anchor_number: AnchorNumber = 10_000; + let origin: FrontendHostname = "https://some.origin".to_string(); + + // 2. Create application but with empty account reference list + let app_num = storage.lookup_or_insert_application_number_with_origin(&origin); + storage.stable_account_reference_list_memory.insert( + (anchor_number, app_num), + vec![].into(), // Empty reference list + ); + + // 3. Try to read default account + let read_params = ReadAccountParams { + account_number: None, + anchor_number, + origin: &origin, + }; + let default_account = storage.read_account(read_params).unwrap(); + + // 4. Verify we get a synthetic default account + let expected_account = Account::new(anchor_number, origin, None, None); + assert_eq!(default_account, expected_account); +} + +#[test] +fn should_not_read_account_from_wrong_anchor() { + // Setup storage + let memory = VectorMemory::default(); + let mut storage = Storage::new((10_000, 3_784_873), memory); + + // 1. Define parameters for two different anchors + let anchor_number_1: AnchorNumber = 10_000; + let anchor_number_2: AnchorNumber = 10_001; + let origin: FrontendHostname = "https://some.origin".to_string(); + let account_name = "account name".to_string(); + + // 2. Create account for first anchor + let create_params = CreateAccountParams { + anchor_number: anchor_number_1, + origin: origin.clone(), + name: account_name, + }; + storage.create_additional_account(create_params).unwrap(); + + // 3. Try to read the account with second anchor + let read_params = ReadAccountParams { + account_number: Some(1), // First account created + anchor_number: anchor_number_2, // Different anchor + origin: &origin, + }; + let account = storage.read_account(read_params); + + // 4. Verify we get None since the account doesn't belong to anchor_number_2 + assert!( + account.is_none(), + "Should not be able to read account from wrong anchor" + ); +} diff --git a/src/internet_identity/src/storage/storable_account_reference_list.rs b/src/internet_identity/src/storage/storable_account_reference_list.rs index bec6ccaef6..a3b7eac9fc 100644 --- a/src/internet_identity/src/storage/storable_account_reference_list.rs +++ b/src/internet_identity/src/storage/storable_account_reference_list.rs @@ -14,6 +14,10 @@ impl StorableAccountReferenceList { pub fn into_acc_ref_vec(self) -> Vec { self.0 } + + pub fn len(&self) -> usize { + self.0.len() + } } impl From for Vec {