Change registration on API v2 to flow with 3 steps #2629

frederikrothenberger · 2024-09-26T12:53:01Z

The registration for new identities using API v2 is now split into 3 different steps:

start a flow, providing a principal to track progress on that flow
solve captcha
submit authn_method to tie the identity to

This PR is a preparation to making the step 2 dynamically remove step 2 on low load.

The registration for new identities using API v2 is now split into 3 different steps: 1. start a flow, providing a principal to track progress on that flow 2. solve captcha 3. submit authn_method to tie the identity to This PR is a preparation to making the step 2 dynamically remove step 2 on low load.

frederikrothenberger · 2024-09-26T12:55:40Z

src/internet_identity/src/state/flow_states.rs

+const FLOW_EXPIRATION_NS: u64 = 5 * MINUTE_NS;
+
+#[derive(Default)]
+pub struct FlowStates {


@lmuntaner: Ultimately, I decided against mixing flow states with temp_keys. I feel this is cleaner in terms of separation.
But it means that the "expiration logic" here and temp_keys is duplicated. I don't think it is big enough to warrant extracting that, but up to you.

So now the temp keys won't be used to track the registration flow, right?

Do we need the enum for the TempKeyId still?

No, I will revert #2618. I'll do that after this is merged.

frederikrothenberger · 2024-09-26T12:57:33Z

src/internet_identity/src/state/flow_states.rs

+    /// * Once the flow expires
+    /// * The Internet Identity canister is upgraded
+    ///
+    /// The total number of flow states is limited by the registration rate limit.


Given the captcha is now tied to the flow_state and the number of flow_states is limited by the registration rate limit, the max_unsolved_captchas config option in the captcha config only has an effect on the legacy registration flow.

Doesn't that depend on what comes furst, the registration rate limit or the max_unsolved_captchas?

No, max_unsolved_captchas is not used at all in the context of the V2 registration flow, as we store the captcha with the flow state.

lmuntaner

I'm happy because after our conversation I was able to follow this PR pretty well. Thanks!

Minor comments, already approving.

lmuntaner · 2024-09-27T07:26:56Z

.github/workflows/canister-tests.yml

@@ -955,8 +955,23 @@ jobs:
    steps:
      - uses: actions/checkout@v4
      - uses: ./.github/actions/setup-didc
+      - name: 'Get latest release'


Does this needs to be done every time we introduce a breaking change?

Do you have a script or how do you remember how to do it?

I usually pull it out again from git history whenever I need it. My hope was that it would not be worth it improving the infrastructure to do that, as we would eventually stop making breaking changes (as APIv2 would be finished)...

lmuntaner · 2024-09-27T07:28:22Z

src/internet_identity/internet_identity.did

@@ -532,6 +521,72 @@ type SignedIdAlias = record {
    id_dapp : principal;
 };

+type IdRegNextStepResult = record {


Why not IdRegStartResult?

I reuse this result for _start and check_captcha. It is essentially a result to indicate a step has been completed correctly and what the next step is.

So if there are more / different steps in the flow in the future, they can also use this.

lmuntaner · 2024-09-27T07:35:00Z

src/internet_identity/src/anchor_management/registration/registration_flow_v2.rs

+    })
+}
+
+async fn captcha_flow_state(flow_created_timestamp_ns: u64) -> (Base64, RegistrationFlowState) {


Will you add that this returns Option<Base64> afterwards?

You mean, once the captcha might be skipped? No, I think I will identity_registration_start with some logic to decide whether this function should be called or if the next step is Finish.

src/internet_identity/src/anchor_management/registration/registration_flow_v2.rs

lmuntaner · 2024-09-27T07:38:37Z

src/internet_identity/src/anchor_management/registration/registration_flow_v2.rs

+}
+
+fn create_identity(arg: &IdRegFinishArg) -> Result<IdentityNumber, IdRegFinishError> {
+    let Some(mut identity) = state::storage_borrow_mut(|s| s.allocate_anchor()) else {


Is this a thing we will need to change to allow creating the identity number at the beginning of the flow instead of the end?

Yes, there will definitely be changes here. But there need to be many more. Maybe its best we have a separate discussion about it.

lmuntaner · 2024-09-27T07:42:59Z

src/internet_identity/src/state/flow_states.rs

+    /// * Once the flow expires
+    /// * The Internet Identity canister is upgraded
+    ///
+    /// The total number of flow states is limited by the registration rate limit.


Doesn't that depend on what comes furst, the registration rate limit or the max_unsolved_captchas?

src/internet_identity/src/state/flow_states.rs

lmuntaner · 2024-09-27T07:47:09Z

src/internet_identity/src/state/flow_states.rs

+        self.registration_flows.remove(principal);
+    }
+
+    pub fn registration_flow_state(&self, principal: &Principal) -> Option<RegistrationFlowState> {


Should we also prune here?

I don't think it is necessary, as we explicitly check expiration before returning anything.
IMHO it is more natural to prune alongside other modifications, rather than on read.

src/internet_identity/src/state/flow_states.rs

lmuntaner · 2024-09-27T07:51:44Z

src/internet_identity/tests/integration/v2_api/identity_register.rs

@@ -37,6 +38,40 @@ fn should_register_multiple_identities() {
    assert_ne!(identity_number_1, identity_number_2);
 }

+#[test]
+fn should_transition_flow_principal_to_temp_key() {


Why this name for this test?

It's to make sure that the temp_key is expired after the required time, right?

The test is there for two things:

the principal used in the flow can still be used after the registration (e.g. to add additional devices / get a delegation, etc.)

the principal used in the flow expires after the expected amount of time

frederikrothenberger requested a review from a team as a code owner September 26, 2024 12:53

🤖 npm run generate auto-update

28aebf0

frederikrothenberger commented Sep 26, 2024

View reviewed changes

frederikrothenberger requested a review from lmuntaner September 26, 2024 13:00

lmuntaner approved these changes Sep 27, 2024

View reviewed changes

Small review fixes

ca17ae5

frederikrothenberger added this pull request to the merge queue Sep 27, 2024

Merged via the queue into main with commit efc7537 Sep 27, 2024
66 checks passed

frederikrothenberger deleted the frederik/refactor-register-flow branch September 27, 2024 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change registration on API v2 to flow with 3 steps #2629

Change registration on API v2 to flow with 3 steps #2629

frederikrothenberger commented Sep 26, 2024 •

edited by github-actions bot

Loading

frederikrothenberger Sep 26, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

frederikrothenberger Sep 26, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner left a comment

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner Sep 27, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

lmuntaner Sep 27, 2024

frederikrothenberger Sep 27, 2024

Change registration on API v2 to flow with 3 steps #2629

Change registration on API v2 to flow with 3 steps #2629

Conversation

frederikrothenberger commented Sep 26, 2024 • edited by github-actions bot 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

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

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

frederikrothenberger commented Sep 26, 2024 •

edited by github-actions bot

Loading