Add account delegation preparation #3076

LXIF · 2025-05-15T14:06:25Z

Motivation

In order to be able to authenticate with accounts, we need to first prepare the delegation.

Changes

Add necessary functions - calculate_account_delegation seed (which resolves to a regular anchor number based seed for default accounts and former default accounts), prepare_account_delegation which closely mirrors prepare_delegation but uses the different seed and checks whether the anchor has access to the account, as well as the prepare_account_delegation endpoint in main.rs which checks auth, records activity and returns the delegation, wrapped in a Result for frontend convenience. Changing legacy get_delegation method to also use new functions. Also, related structs/methods etc.

Tests

No additional tests at this time, created Jira tickets for the integration tests. However, since legacy endpoint uses new functions, there is also some amount of test coverage.

… main, update existing code

…s as well as tests

…te-account merge get_accounts return result

…te-account Update error handling

…date-account

lmuntaner

Looking good!

lmuntaner · 2025-05-19T17:24:18Z

src/internet_identity/src/storage.rs

+                    // this should only happen if a named account was created, and then both it and the
+                    // default account references were moved/deleted.
+                    if acc_ref_vec.is_empty() {
+                        return Some(Account::new(


If they were moved or deleted, shouldn't we return None?

Normally yes, but we do have to guarantee that a user can always log in. If we don't return a synthetic default account here, hypothetically a user could have hit the maximum after deleting a renamed default account. Maybe we should talk about this more though, basically transferring a default account and also having a synthetic default account here means the original identity retains access to the transferred default account.

In any case, this should not happen for now because we cannot currently move or delete accounts.

That makes sense.

Let's add it in the comment as well. It's not clear at first sight why we should return the synthetic account.

I agree we should think this more. We should have a WARNING in the comment, to review this at some point and the edge cases we have with the current code.

Adding this.

lmuntaner · 2025-05-19T18:16:43Z

src/internet_identity/src/storage.rs

@@ -735,21 +735,21 @@ impl<M: Memory + Clone> Storage<M> {
            .map(|list| list.into())
    }

-    pub fn has_account_reference(
+    fn has_account_reference(


Should we rename this method to find_account_reference? It returns an AccountReference while the name has_... indicates a boolean.

I probably wrote this method, but I think it's confusing.

lmuntaner · 2025-05-19T18:18:44Z

src/internet_identity/src/storage/account/tests.rs

Can we add two more tests for read_account for the two new use cases?

lmuntaner

Looking good thanks!

Already approving, but please extend the comments. There are some edge cases that we should solve sooner rather than later.

lmuntaner · 2025-05-20T12:35:26Z

src/internet_identity/src/account_management.rs

-
-                if stored_accounts >= MAX_ANCHOR_ACCOUNTS as u64 {
-                    return Err(UpdateAccountError::AccountLimitReached);
+    storage_borrow_mut(|storage| {


I realized now, but waas this necessary for the prepare delegation?

No need to do it in another PR, just trying to understand the changes.

You and @sea-snake had asked to put anchor_has_account functionality into read_account, so I removed it here. Not sure why storage_borrow_mut got moved, moving it back now (no need to borrow storage if we're not using it).

lmuntaner · 2025-05-20T12:42:11Z

src/internet_identity/src/delegation.rs

@@ -150,6 +119,21 @@ fn calculate_seed(anchor_number: AnchorNumber, frontend: &FrontendHostname) -> H
    hash_bytes(blob)
 }

+pub fn calculate_account_seed(account_number: AccountNumber) -> Hash {


Can we add a comment on why we don't need the frontend nor the anchor number?

It's not obvious unless you know everything about the accounts.

lmuntaner · 2025-05-20T12:46:46Z

src/internet_identity/src/main.rs

+             timestamp,
+         }| (user_key, timestamp),
+    )
+    .unwrap_or_else(|err| trap(&format!("{:?}", err)))


Are we planning on use the AccountDelegationError returned by prepare_account_delegation here?

In the spirit of backwards compatibility, I have not updated legacy endpoints. We can do that, but would also need to update the frontend.

I guess that's fine for now. I was wondering whether you discussed it already.

lmuntaner · 2025-05-20T12:49:01Z

src/internet_identity/src/storage/account.rs

+            }
+            None => {
+                // If this is an added account, we derive from the account number.
+                delegation::calculate_account_seed(self.account_number.unwrap())


To be safer, you could match both:

match (self.get_seed_anchor(), self.account_number)

This way we don't need to rely in unwrapping and we can trap with a nicer error message. Wdyt?

If this is about the error message, we can just .expect() with a nicer message. Matching both means we need four branches, two of which are basically identical, this would feel a bit superfluous.

You don't need four branches. Three should be enough and it's maybe clearer the error.

(Some(seed_from_anchor), _) => ... (None, Some(account_number) => delegation::calculate_account_seed(account_number) (None, None) => trap("Missing both seed anchor and account number in this account")

Just a nitpick though. No need to implement it. I like pattern matching personally 😄

lmuntaner · 2025-05-20T12:54:03Z

src/internet_identity/src/storage.rs

+                    // this should only happen if a named account was created, and then both it and the
+                    // default account references were moved/deleted.
+                    if acc_ref_vec.is_empty() {
+                        return Some(Account::new(


That makes sense.

Let's add it in the comment as well. It's not clear at first sight why we should return the synthetic account.

I agree we should think this more. We should have a WARNING in the comment, to review this at some point and the edge cases we have with the current code.

lmuntaner · 2025-05-20T12:54:37Z

src/internet_identity/src/storage/account/tests.rs

@@ -498,3 +496,68 @@ fn should_count_accounts_different_anchors() {
        "Total counters after anchor 2 additional account mismatch"
    );
 }
+
+#[test]
+fn should_read_default_account_with_empty_reference_list() {


Replicate part of the comment on why we return a default account in this case here, please.

lmuntaner · 2025-05-20T12:55:26Z

src/internet_identity/src/storage.rs

    /// 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.


Is this comment accurate? Aren't we returning a default account if the list is empty?

It is correct but incomplete - if the default has been moved or deleted but there are other accounts, we return None. If there are no accounts left (all accounts moved/deleted), we return a synthetic default account. Updating comments.

(I do agree that this is not a great solution - we currently cannot actually get to this state, but we do need to think about this at some point)

lmuntaner · 2025-05-20T12:57:01Z

src/internet_identity/src/storage.rs

+                    }
+
+                    // if there is a default account in the list, we return it
+                    // else we return None, account has been moved or deleted


Shuold we mention here that the user can still log in with the other account?

* add get_accounts_for_origin to account_management and get_accounts to main, update existing code * add authorization fn * clippy * add tests for get and create * clippy * add account updating endpoint and related account management functions as well as tests * cwippy * add seed_from_anchor to Account * add seed_from_anchor to account struct * add calculate_account_delegation_seed function * make get_accounts return a result * add principal to GetAccountsError::Unauthorized * add const * add global acc limit and unit test * add max acc limit to update function * add authorization, add checking if anchor has account (for later) * clippy * fix did * add per-acc auth check in account management * add tests, restructure per-acc auth check in update_account * change variant name, update comment * cleanup function inputs * cwippy * cwippy again * minor changes * add account delegation preparation function and update endpoint * fix did * fix did * cwippy * fix did again * cwippy fix * fix did? * fix typing * clippyyy * cleanup * clippy * fix read_account method to account for seed_from_anchor * rename * move seed generation function into account impl * clippy * cleanup delegation generation, handle default account in has_account_reference, move anchor_has_account * return account from anchor_has_account * cleanupt * restructure has_account_reference and anchor_has_account * cwippy * return default acc if no application number * cwippy * cleanup * restructure storage borrowing * make nicer * update list_accounts to return accounts and use read_account * remove anchor_has_account, pack everything into read_account * clippy * clippy * add comment * clippy * change error type * fix function inputs * fix tests * 🤖 cargo-fmt auto-update * add tests, rename some things * remove comment * update comments * add expect message, add comments * clippy * update match statement in calculate_seed --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

* add get_accounts_for_origin to account_management and get_accounts to main, update existing code * add authorization fn * clippy * add tests for get and create * clippy * add account updating endpoint and related account management functions as well as tests * cwippy * add seed_from_anchor to Account * add seed_from_anchor to account struct * add calculate_account_delegation_seed function * make get_accounts return a result * add principal to GetAccountsError::Unauthorized * add const * add global acc limit and unit test * add max acc limit to update function * add authorization, add checking if anchor has account (for later) * clippy * fix did * add per-acc auth check in account management * add tests, restructure per-acc auth check in update_account * change variant name, update comment * cleanup function inputs * cwippy * cwippy again * minor changes * add account delegation preparation function and update endpoint * fix did * fix did * cwippy * fix did again * cwippy fix * fix did? * fix typing * clippyyy * cleanup * clippy * fix read_account method to account for seed_from_anchor * start updating api * rename * move seed generation function into account impl * start adding from * clippy * add get_account_delegation method in account_management * cwippy * cleanup delegation generation, handle default account in has_account_reference, move anchor_has_account * return account from anchor_has_account * merge updates * cleanupt * restructure has_account_reference and anchor_has_account * cwippy * return default acc if no application number * cwippy * cleanup * restructure storage borrowing * replaced existing get_delegation with new and more general get_account_delegation * cwippy * cwippy again * update did * update did * update did * make nicer * update list_accounts to return accounts and use read_account * remove anchor_has_account, pack everything into read_account * clippy * clippy * add comment * clippy * change error type * fix function inputs * fix tests * 🤖 cargo-fmt auto-update * clippy * clippy * add tests, rename some things * minor adjustments * Add account delegation preparation (#3076) * add get_accounts_for_origin to account_management and get_accounts to main, update existing code * add authorization fn * clippy * add tests for get and create * clippy * add account updating endpoint and related account management functions as well as tests * cwippy * add seed_from_anchor to Account * add seed_from_anchor to account struct * add calculate_account_delegation_seed function * make get_accounts return a result * add principal to GetAccountsError::Unauthorized * add const * add global acc limit and unit test * add max acc limit to update function * add authorization, add checking if anchor has account (for later) * clippy * fix did * add per-acc auth check in account management * add tests, restructure per-acc auth check in update_account * change variant name, update comment * cleanup function inputs * cwippy * cwippy again * minor changes * add account delegation preparation function and update endpoint * fix did * fix did * cwippy * fix did again * cwippy fix * fix did? * fix typing * clippyyy * cleanup * clippy * fix read_account method to account for seed_from_anchor * rename * move seed generation function into account impl * clippy * cleanup delegation generation, handle default account in has_account_reference, move anchor_has_account * return account from anchor_has_account * cleanupt * restructure has_account_reference and anchor_has_account * cwippy * return default acc if no application number * cwippy * cleanup * restructure storage borrowing * make nicer * update list_accounts to return accounts and use read_account * remove anchor_has_account, pack everything into read_account * clippy * clippy * add comment * clippy * change error type * fix function inputs * fix tests * 🤖 cargo-fmt auto-update * add tests, rename some things * remove comment * update comments * add expect message, add comments * clippy * update match statement in calculate_seed --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Improve copy in buttons (#3082) * Improve copy in buttons * Revert changes in use another account * small fixes * merge main and small fixes * run generate * cleanup --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Llorenç Muntaner <[email protected]>

LXIF added 30 commits May 13, 2025 11:15

add get_accounts_for_origin to account_management and get_accounts to…

80c98c2

… main, update existing code

add authorization fn

daf7c2f

clippy

5fb5bc1

add tests for get and create

c25a7bb

clippy

e5f72b3

add account updating endpoint and related account management function…

07d0f73

…s as well as tests

cwippy

b98bc18

add seed_from_anchor to Account

55f85d9

add seed_from_anchor to account struct

5a4c704

add calculate_account_delegation_seed function

af693d0

make get_accounts return a result

f10ac25

Merge branch 'andri/implement-get-accounts' into andri/implement-crea…

a281016

…te-account merge get_accounts return result

add principal to GetAccountsError::Unauthorized

d75a785

Merge branch 'andri/implement-get-accounts' into andri/implement-crea…

9110107

…te-account Update error handling

add const

4355c14

add global acc limit and unit test

fe51d28

Merge branch 'andri/implement-create-account' into andri/implement-up…

c80561c

…date-account

add max acc limit to update function

dc5863d

add authorization, add checking if anchor has account (for later)

a3c9501

merge main

fa38325

clippy

674bbb0

fix did

72a3294

Merge branch 'andri/implement-create-account' into andri/implement-up…

893d1f5

…date-account

add per-acc auth check in account management

6cefda2

add tests, restructure per-acc auth check in update_account

a74c252

change variant name, update comment

81c6521

cleanup function inputs

da58a36

merge main

206b259

cwippy

037c9e8

cwippy again

5e5687d

LXIF and others added 11 commits May 19, 2025 13:29

update list_accounts to return accounts and use read_account

cad122e

remove anchor_has_account, pack everything into read_account

068f900

clippy

ffbfa93

clippy

f23c061

add comment

ed07dc1

clippy

c27bbc3

change error type

fa0773d

Merge branch 'main' into andri/implement-create-account-delegation

db7d1ed

fix function inputs

591c28b

fix tests

19e0299

🤖 cargo-fmt auto-update

52e5349

LXIF requested review from sea-snake and lmuntaner May 19, 2025 13:39

lmuntaner reviewed May 19, 2025

View reviewed changes

LXIF added 2 commits May 20, 2025 14:21

add tests, rename some things

3788ba7

Merge branch 'main' into andri/implement-create-account-delegation

a64e846

LXIF requested a review from lmuntaner May 20, 2025 12:23

lmuntaner approved these changes May 20, 2025

View reviewed changes

LXIF added 6 commits May 20, 2025 15:40

remove comment

c7fc663

update comments

ec0af93

add expect message, add comments

f1a27d3

clippy

5b7e04a

update match statement in calculate_seed

acd0dea

Merge branch 'main' into andri/implement-create-account-delegation

fe6baa1

LXIF added this pull request to the merge queue May 20, 2025

Merged via the queue into main with commit effb443 May 20, 2025
70 checks passed

LXIF deleted the andri/implement-create-account-delegation branch May 20, 2025 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add account delegation preparation #3076

Add account delegation preparation #3076

LXIF commented May 15, 2025 •

edited by github-actions bot

Loading

lmuntaner left a comment

lmuntaner May 19, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 19, 2025

LXIF May 20, 2025

LXIF May 20, 2025

lmuntaner May 19, 2025

LXIF May 20, 2025

lmuntaner left a comment

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025 •

edited

Loading

LXIF May 20, 2025

lmuntaner May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

LXIF May 20, 2025

lmuntaner May 20, 2025

LXIF May 20, 2025

Add account delegation preparation #3076

Add account delegation preparation #3076

Conversation

LXIF commented May 15, 2025 • edited by github-actions bot Loading

Motivation

Changes

Tests

lmuntaner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmuntaner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmuntaner May 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LXIF commented May 15, 2025 •

edited by github-actions bot

Loading

lmuntaner May 20, 2025 •

edited

Loading