Skip to content

Conversation

@daniel-mader
Copy link
Collaborator

@daniel-mader daniel-mader commented Feb 14, 2025

Description of change

The user can opt-in to use biometrics to unlock the app.

Links to any relevant issues

How the change has been tested

Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.

Definition of Done checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

daniel-mader and others added 30 commits August 28, 2024 22:59
@daniel-mader daniel-mader marked this pull request as ready for review April 15, 2025 14:41
@daniel-mader daniel-mader requested a review from Copilot April 15, 2025 21:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

identity-wallet/src/state/common/reducers/unlock_storage.rs:26

  • [nitpick] Consider extracting the block of code handling the check_password_only flag into a separate helper function to improve readability and maintainability.
if check_password_only.unwrap_or_default() {

@nanderstabel
Copy link
Contributor

I've looked into the failing tests and there are 2.5 causes:
1.
test_qr_code_scanned_handle_siopv2_authorization_request and test_qr_code_scanned_handle_oid4vp_authorization_request fail because during both tests, the Authorization Response is send to http://example.com, however this domain doesn't exist anymore and therefore the tests both time out. So these failing tests are not introduced in this PR. I've validated that this was the issue by replacing the domain to http://example.org (http://example.test also doesn't exist anymore), you can check out the changes in the fix/example-org branch. This would be a quick fix, but relying on external domain is not a good idea like we've discussed before. Perhaps it is better to change the test domains to https://www.impierce.com/ (because this domain will live forever 😉) since the only requirement for the tests is that it can send an HTTP request (they don't care about the responses, only whether the domain exists). Of course ultimately the best solution would be to test against a test UniCore instance.

Edit: it can be simply solved by skipping sending the Authorization Response during tests:

    #[cfg(not(feature = "test_utils"))]
    if provider_manager.send_response(&response).await.is_err() {
        info!("failed to send response");
        return Err(SendAuthorizationResponseError);
    }

in both handle_siopv2_authorization_request.rs and handle_oid4vp_authorization_request.rs

test_get_state_create_new and test_locale_stays_unchanged_on_profile_creation are failing because their corresponding fixture JSON files do not contain the newly introduced biometrics_enabled field in the CreateNew Action. So you can either add those fields to the corresponding fixtures, or probably better: add #[serde(default)] so that the missing field will default to false.

pub struct CreateNew {
    pub name: String,
    pub picture: String,
    pub theme: AppTheme,
    pub password: String,
    #[serde(default)]
    pub biometrics_enabled: bool,
}

2.5
verifier_successfully_verifies_eddsa_signed_data is still flaky because test_credential_is_removed_from_appstate_and_from_stronghold_and_image_is_deleted is not serial. This can easily be fixed by adding the serial attribute:

    #[tokio::test]
    #[serial_test::serial]
    async fn test_credential_is_removed_from_appstate_and_from_stronghold_and_image_is_deleted()

Copy link
Contributor

@nanderstabel nanderstabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust code looks pretty solid, nicely done! Just left some questions and suggestions regarding the translations.

maiertech
maiertech previously approved these changes Apr 22, 2025
@daniel-mader
Copy link
Collaborator Author

daniel-mader commented May 13, 2025

@nanderstabel, regarding the failing tests:

  1. Let's fix this in a separate PR since it seems like a larger fix. Related issues:
  1. fixed ✅
    2.5 fixed ✅

@nanderstabel nanderstabel self-requested a review May 14, 2025 14:53
@daniel-mader daniel-mader merged commit c39754a into dev May 19, 2025
3 checks passed
@daniel-mader daniel-mader deleted the feat/biometric-keystore branch May 19, 2025 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Added A new feature that requires a minor release. Blocked An issue that is blocked by another issue or another blocker. V1 MVP Necessary for the minimal MVP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Biometric unlock

4 participants