Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security structures setup changes #336

Merged
merged 17 commits into from
Jan 14, 2025

Conversation

sergiupuhalschi-rdx
Copy link
Contributor

This PR slightly changes the existing API around security structures to conform to the UI requirements:

  • Exposed function to save SecurityStructureOfFactorSourceIDs into the profile to make it possible to save a security structure right after it's been built;
  • Changed from selected_factor_sources_for_role_status into selected_primary_threshold_factors_status to validate only the threshold factors of the primary role in isolation. This solves an issue in the hosts when the user selects some threshold factors, proceeds to the next screen, makes the primary role invalid by adding a override factor that breaks the combination rules, and returns to the threshold factors selection at which point the invalid combination error shouldn't be presented if the threshold factors selection is still valid.
  • Augmented SelectedPrimaryThresholdFactorsStatus::Invalid with some error details required in the UI. This makes it possible to display a warning specific to a FactorListKind like a "Cannot use by itself" warning for the Password section.
  • Added the FactorListKind parameter to the remove_factor_from_primary to make it possible to remove either from threshold or override list only. Also, it's possible to have the security structure in an invalid state, specifically now a factor can be both in the threshold and override lists, that's why the removal could no longer be based on the fact that it cannot.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 93.54839% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.59%. Comparing base (f98db7f) to head (3139c66).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...matrices_structures/roles/builder/roles_builder.rs 90.38% 5 Missing ⚠️
...actor_instance_level/role_with_factor_instances.rs 25.00% 3 Missing ⚠️
...ces_structures/matrices/builder/matrix_template.rs 0.00% 1 Missing ⚠️
...s/factor_source_level/roles_with_factor_sources.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   92.54%   92.59%   +0.04%     
==========================================
  Files        1152     1155       +3     
  Lines       25623    25688      +65     
  Branches       85       85              
==========================================
+ Hits        23713    23785      +72     
+ Misses       1899     1892       -7     
  Partials       11       11              
Flag Coverage Δ
kotlin 97.95% <ø> (ø)
rust 92.05% <93.54%> (+0.05%) ⬆️
swift 93.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergiupuhalschi-rdx sergiupuhalschi-rdx self-assigned this Jan 9, 2025
Comment on lines 220 to 223
self.primary_role
.validate_threshold_factors()
.into_matrix_err(RoleKind::Primary)?;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just:

 self.primary_role
            .validate_threshold_factors()
            .into_matrix_err(RoleKind::Primary)

@@ -0,0 +1,55 @@
use crate::prelude::*;

/// Represents the status of selected threshold factor sources in the Security Shield building process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Represents the status of selected threshold factor source

Specifically for Primary Treshold

sergiupuhalschi-rdx and others added 6 commits January 13, 2025 17:55
* Update security structure threshold

* Update api and tests

* Update default threshold. Add tests

* Add test

* Fix swift tests

* Fix fs removing from primary threshold. Add test

* Update naming. Add Threshold ctor
@sergiupuhalschi-rdx sergiupuhalschi-rdx added Rust 🦀 Changes in Rust Sargon UniFFI 🦄 Changes in UniFFI exported APIs labels Jan 14, 2025
@sergiupuhalschi-rdx sergiupuhalschi-rdx marked this pull request as ready for review January 14, 2025 12:53
self.threshold.value(self.threshold_factors.len())
}

/// How many threshold factors that must be used to perform some function with
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs a different doc compared to above

return Some(
NotYetValidReason::ThresholdHigherThanThresholdFactorsLen,
);
}
if self.get_threshold() == 0 && !self.get_threshold_factors().is_empty()
{
if threshold_value == 0 && !self.get_threshold_factors().is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

threshold_value cannot be zero, but I guess can be a paranoia check.

Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Contributor

@danvleju-rdx danvleju-rdx left a comment

Choose a reason for hiding this comment

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

Looks good!

@sergiupuhalschi-rdx sergiupuhalschi-rdx merged commit 51d850f into main Jan 14, 2025
13 checks passed
@sergiupuhalschi-rdx sergiupuhalschi-rdx deleted the sp/security-structures-setup-changes branch January 14, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust 🦀 Changes in Rust Sargon UniFFI 🦄 Changes in UniFFI exported APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants