Add and remove OpenID credentials from anchor #2810

sea-snake · 2025-01-22T12:50:06Z

Changes

OpenID credentials in stable structures.
Lookup anchor number by OpenID credential in stable structures.
Anchor management methods to add/remove OpenID credentials.
Anchor management method to lookup anchor with OpenID credential.
Canister methods to add/remove OpenID credentials.
Replaced frontend mock OpenID implementation with above canister methods.
Use canister config endpoint to get Google client id for FedCM interaction.

Tests

Added additional anchor tests:

should_add_openid_credential
should_remove_openid_credential
should_update_openid_credential

Added additional storage test:

should_write_and_update_openid_credential_lookup

🟡 Some screens were changed

…ent methods to add/remove OpenID credential.

- Lookup anchor number by OpenID credential in stable structures. - Anchor management methods to add/remove OpenID credentials

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

lmuntaner

A few comments. Looking good. I'd like to leave until tomorrow to think about the implications of adding the StableAnchor to the memory.

My first impression, is that it makes sense. Let's discuss again tomorrow.

src/internet_identity/src/openid.rs

src/internet_identity/src/storage.rs

src/internet_identity/src/storage/anchor.rs

src/internet_identity/src/storage/stable_anchor.rs

…rds compatible.

src/internet_identity/src/storage/anchor.rs

src/internet_identity/src/storage.rs

src/internet_identity/src/storage/stable_anchor.rs

src/internet_identity/src/storage.rs

src/internet_identity/src/openid.rs

lmuntaner · 2025-01-23T09:16:01Z

@sea-snake after sleeping over this new memory, I can't find a reason not to move forward. Looks good to me!

Let me know when the PR is ready for the final review.

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

…rt unbounded tuples

src/internet_identity/src/anchor_management.rs

jasonz-dfinity · 2025-01-23T15:49:39Z

src/internet_identity/src/anchor_management.rs

+pub fn update_openid_credential(
+    anchor: &mut Anchor,
+    openid_credential: OpenIdCredential,
+) -> Result<(), AnchorError> {


There is probably a good reason and might be related to how it's going to be used later, but I just wanted to point out that the update here is seemingly inconsistent with add/remove in terms of return type, and also different from add/remove/update for devices.

Yeah that's intentional, compared to updating a device, updating a credential doesn't result in any operation that needs to be stored in the archive canister. This is because a device its metadata can change after user action e.g. alias change while an OpenID credential will be updated on every JWT verification (to keep latest claims from OpenID provider stored).

Additionally, I intentionally only keep track of the issuer in operations for the archive canister, anything beyond that would likely impact a user's privacy. This data is enough to keep track of adoption of a given OpenId without knowing details beyond that.

src/internet_identity/src/storage/anchor/tests.rs

src/internet_identity/src/storage/stable_anchor.rs

src/internet_identity/src/storage.rs

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

lmuntaner

Thanks!

lmuntaner

Thanks! I left a few comments.

This PR is too big for my taste, it's hard to keep track of all the implications. Please test it carefully before merging.

I'm requesting changes to prevent merging before the comments are addressed.

lmuntaner · 2025-01-28T10:21:30Z

dfx.json

@@ -5,7 +5,7 @@
      "candid": "src/internet_identity/internet_identity.did",
      "wasm": "internet_identity.wasm.gz",
      "build": "bash -c 'II_DEV_CSP=1 II_FETCH_ROOT_KEY=1 II_DUMMY_CAPTCHA=${II_DUMMY_CAPTCHA:-1} scripts/build'",
-      "init_arg": "(opt record { captcha_config = opt record { max_unsolved_captchas= 50:nat64; captcha_trigger = variant {Static = variant {CaptchaDisabled}}}})",
+      "init_arg": "(opt record { captcha_config = opt record { max_unsolved_captchas= 50:nat64; captcha_trigger = variant {Static = variant {CaptchaDisabled}}}; openid_google = opt opt record { client_id = \"45431994619-cbbfgtn7o0pp0dpfcg2l66bc4rcg7qbu.apps.googleusercontent.com\" }})",


Is this our dev environment? Did you check with IT about the prod one?

It's a dev environment I created, only works internally within DFINITY. This should be removed at a later point and replaced with instructions on creating a client id and configuring it with II.

Haven't gotten around to ask IT about the prod one yet, we'll need to also ask them to create others for our other environments.

src/frontend/src/flows/manage/index.ts

src/frontend/src/utils/iiConnection.ts

src/internet_identity/src/anchor_management.rs

src/internet_identity/src/storage/anchor.rs

src/internet_identity/src/storage/anchor/tests.rs

src/frontend/src/flows/manage/linkedAccountsSection.json

src/frontend/src/utils/iiConnection.ts

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

sea-snake and others added 10 commits January 22, 2025 11:54

Add OpenID credential storage in stable structures and anchor managem…

464c0d2

…ent methods to add/remove OpenID credential.

- OpenID credentials in stable structures.

7db91b5

- Lookup anchor number by OpenID credential in stable structures. - Anchor management methods to add/remove OpenID credentials

🤖 cargo-fmt auto-update

7e749e7

🤖 npm run generate auto-update

1a92adb

Fix error in archive operation compat

cca4497

Fix error in anchor tests

eaf5646

Add OpenID credentials to did.

db90889

🤖 npm run generate auto-update

b3534cb

Add OpenID credentials operations to archive did.

98e3455

Merge remote-tracking branch 'origin/sea-snake/openid-google-add-remo…

279a73a

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

lmuntaner reviewed Jan 22, 2025

View reviewed changes

sea-snake and others added 7 commits January 22, 2025 14:54

Make OpenID credential in StableAnchor not optional.

28e4640

Make OpenID credentials in IdentityAnchorInfo optional so it's backwa…

04117db

…rds compatible.

🤖 cargo-fmt auto-update

072252c

Make OpenID credentials in IdentityAnchorInfo optional so it's backwa…

80d94ac

…rds compatible.

🤖 npm run generate auto-update

3042122

Fix principal from seed in tests

a555694

🤖 cargo-fmt auto-update

bed886e

jasonz-dfinity approved these changes Jan 22, 2025

View reviewed changes

Changes based on feedback and added tests for new Anchor methods.

114d254

sea-snake marked this pull request as ready for review January 23, 2025 14:38

github-actions bot and others added 3 commits January 23, 2025 14:39

🤖 cargo-fmt auto-update

2a6827b

Fix serde name for archive operation.

4639e4e

Merge remote-tracking branch 'origin/sea-snake/openid-google-add-remo…

5a7e8ef

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

sea-snake requested a review from lmuntaner January 23, 2025 14:47

sea-snake and others added 2 commits January 23, 2025 19:37

Wrap tuple with struct since stable-structures doesn't directly suppo…

aa1f722

…rt unbounded tuples

🤖 cargo-fmt auto-update

0049fbc

jasonz-dfinity reviewed Jan 23, 2025

View reviewed changes

Update based on feedback.

5c47f82

sea-snake and others added 10 commits January 27, 2025 23:59

Undo changes

0631fe2

Undo changes

7f90e07

🤖 cargo-fmt auto-update

770168a

Fix import

f045a05

Merge remote-tracking branch 'origin/sea-snake/openid-google-add-remo…

bfa4f0c

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

Add InternalCanisterError to error enums similar to v2 api.

f7614a0

Fix did file.

e6aec3e

🤖 npm run generate auto-update

8bfe986

Remove frontend env variable and use backend config instead.

37340fb

Merge remote-tracking branch 'origin/sea-snake/openid-google-add-remo…

08e0bb5

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

sea-snake requested a review from lmuntaner January 28, 2025 00:04

Merge branch 'main' into sea-snake/openid-google-add-remove-from-anchor

6c27565

lmuntaner approved these changes Jan 28, 2025

View reviewed changes

lmuntaner requested changes Jan 28, 2025

View reviewed changes

Remove #[allow(unused)] from methods that are now used.

2268e5d

lmuntaner reviewed Jan 28, 2025

View reviewed changes

sea-snake and others added 4 commits January 28, 2025 12:14

Update translation labels based on feedback.

223447f

Add TODO comment.

abd4a04

Add should_register_openid_credential_only_for_a_single_anchor test.

0a1a7d7

🤖 cargo-fmt auto-update

69a7ac7

sea-snake requested a review from lmuntaner January 28, 2025 12:24

sea-snake added 4 commits January 28, 2025 13:25

Add should_register_openid_credential_only_for_a_single_anchor test.

1b47942

Merge remote-tracking branch 'origin/sea-snake/openid-google-add-remo…

eb20b55

…ve-from-anchor' into sea-snake/openid-google-add-remove-from-anchor

Move imports into test itself.

fcbef18

Fix test

0da1a61

lmuntaner approved these changes Jan 28, 2025

View reviewed changes

sea-snake added this pull request to the merge queue Jan 28, 2025

Merged via the queue into main with commit 67f9c63 Jan 28, 2025
67 checks passed

sea-snake deleted the sea-snake/openid-google-add-remove-from-anchor branch January 28, 2025 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and remove OpenID credentials from anchor #2810

Add and remove OpenID credentials from anchor #2810

sea-snake commented Jan 22, 2025 •

edited by github-actions bot

Loading

lmuntaner left a comment

lmuntaner commented Jan 23, 2025

jasonz-dfinity Jan 23, 2025

sea-snake Jan 23, 2025

lmuntaner left a comment

lmuntaner left a comment

lmuntaner Jan 28, 2025

sea-snake Jan 28, 2025

Add and remove OpenID credentials from anchor #2810

Add and remove OpenID credentials from anchor #2810

Conversation

sea-snake commented Jan 22, 2025 • edited by github-actions bot Loading

Changes

Tests

lmuntaner left a comment

Choose a reason for hiding this comment

lmuntaner commented Jan 23, 2025

jasonz-dfinity Jan 23, 2025

Choose a reason for hiding this comment

sea-snake Jan 23, 2025

Choose a reason for hiding this comment

lmuntaner left a comment

Choose a reason for hiding this comment

lmuntaner left a comment

Choose a reason for hiding this comment

lmuntaner Jan 28, 2025

Choose a reason for hiding this comment

sea-snake Jan 28, 2025

Choose a reason for hiding this comment

sea-snake commented Jan 22, 2025 •

edited by github-actions bot

Loading