[ABW-3969] Shield Setup Select Factors - utility functions #297

danvleju-rdx · 2024-12-09T13:28:49Z

Changelog

Added sorted_factor_sources_for_selection function, which handles the logic for sorting a list of factor sources on the Select Factors screen.
Added selected_factor_sources_status function, which returns a newly added enum, SelectedFactorSourcesStatus, representing the status of selected factor sources in the Security Shield building process.
[Swift] Fixes the allCases for SecurityProblemKind

Sajjon · 2024-12-09T13:43:20Z

crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs

+        factor_sources: Vec<FactorSource>,
+    ) -> SelectedFactorSourcesStatus {
+        if factor_sources.is_empty() {
+            SelectedFactorSourcesStatus::Insufficient


Hmm 1 is never enough for a whole shield. So this must be PER ROLE and which role is it? Primary ("Access")?

If so the enum and this method should make it clear that it is for Primary.

(Or if it is dynamic it should be for_role: RoleKind as input, and then we should not return just an enum but rather a struct with the status and the role)

Post-our sync:
Delegate this to MatrixBuilder

In MatrixBuilder:
We should probably NOT let this be a STATIC, even if we wont use the internal state of the MatrixBuilder initially we still might want to in the future. So I would do these changes:

change it to be on &self (i.e. not "static")

change it to take a for_role: RoleKind

Sajjon · 2024-12-09T13:44:45Z

crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs

+        factor_sources: Vec<FactorSource>,
+    ) -> Vec<FactorSource> {
+        let mut sorted = factor_sources;
+        sorted.sort_by_key(|fs| fs.factor_source_kind().display_order());


the perpetual naming challenge for these kind of things... :) I think let mut sorted = factor_sources is misleading, since it is in fact not sorted yet!

Luckily In Rust we have ownership!

so:

let mut unsorted = factor_sources; unsorted.sort.... let sorted = unsorted; // no `unsorted` can never be referenced again! it has been consumed! Rust owns! sorted

Sajjon · 2024-12-09T13:46:09Z

crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs

+        InternalSecurityShieldBuilder::sorted_factor_sources_for_selection(
+            factor_sources.into_internal(),
+        );
+    factors.into_iter().map(FactorSource::from).collect()


I think:

factors.into_iter().map(FactorSource::from).collect()

can be changed to just:

factors.into_type()

Sajjon · 2024-12-09T13:46:31Z

apple/Sources/Sargon/Extensions/Methods/Profile/MFA/SecurityShieldBuilder+Wrap+Functions.swift

+import Foundation
+import SargonUniFFI
+
+extension SecurityShieldBuilder {


probably not needed since should be member method, not global free standing func.

Kept sortedFactorSourcesForSelection as a static function since it’s just a utility function and doesn’t need to involve the shield builder or imply that it can be modified.

codecov · 2024-12-09T13:47:07Z

Codecov Report

Attention: Patch coverage is 97.61905% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.3%. Comparing base (217f10a) to head (636caf9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ityCenter/SecurityProblemKind+Wrap+Functions.swift	0.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #297   +/-   ##
=====================================
  Coverage   93.3%   93.3%           
=====================================
  Files       1095    1095           
  Lines      22922   22962   +40     
  Branches      77      77           
=====================================
+ Hits       21387   21427   +40     
  Misses      1521    1521           
  Partials      14      14

Flag	Coverage Δ
kotlin	`97.9% <ø> (ø)`
rust	`92.7% <100.0%> (+<0.1%)`	⬆️
swift	`94.9% <0.0%> (ø)`

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

Files with missing lines	Coverage Δ
...rity_structures/matrices/builder/matrix_builder.rs	`100.0% <100.0%> (ø)`
...mfa/security_structures/security_shield_builder.rs	`94.5% <100.0%> (+0.7%)`	⬆️
...gon/src/profile/v100/factors/factor_source_kind.rs	`100.0% <100.0%> (ø)`
...ityCenter/SecurityProblemKind+Wrap+Functions.swift	`0.0% <0.0%> (ø)`

CyonAlexRDX · 2024-12-09T14:33:03Z

crates/sargon-uniffi/src/profile/mfa/security_structures/selected_factor_sources_status.rs

+#[derive(
+    Clone, Copy, Debug, PartialEq, Eq, InternalConversion, uniffi::Enum,
+)]
+pub enum SelectedFactorSourcesStatus {


SelectedFactorSourcesForRoleStatus ?

CyonAlexRDX · 2024-12-09T14:35:59Z

crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs

+        factor_sources: Vec<FactorSource>,
+    ) -> SelectedFactorSourcesStatus {
+        if factor_sources.is_empty() {
+            SelectedFactorSourcesStatus::Insufficient


Post-our sync:
Delegate this to MatrixBuilder

In MatrixBuilder:
We should probably NOT let this be a STATIC, even if we wont use the internal state of the MatrixBuilder initially we still might want to in the future. So I would do these changes:

change it to be on &self (i.e. not "static")

change it to take a for_role: RoleKind

GhenadieVP · 2024-12-11T11:31:06Z

crates/sargon-uniffi/src/profile/mfa/security_structures/selected_factor_sources_status.rs

+    /// The selected factor sources form an invalid combination for the specified role
+    /// in the Security Shield building process.
+    ///
+    /// Example: A Password factor source cannot be used alone for the Primary role.


This example seems to fall more under the Insufficient category

From the host’s perspective, this should fall under the Invalid category and display a generic error message. If the Primary role has threshold set to 1, then adding a Password factor source satisfies the threshold condition, but it’s still invalid. It could fall under the Insufficient category if we have a dynamic threshold that updates based on the available factor sources. Do we?

Why is threshold important in the context of selecting the factors before going in the Access setup build screen?

Also, if there are no roles selected the status is Insufficient, but if you have password it is considered Invalid, when IMO it is a case of insufficient - you need one more factor.

This is a very UI specific type and we have in the UI 3 error/warning states:

User need to select at least 1 factor -> Insufficient

It is recommended to select at least 2 factors -> Suboptimal

Any other invalid combinations for a specific role (Access) -> Invalid

Based on that, this case falls under Invalid, even if it doesn't sound correct

I am confused here - what is any other combinations? Shouldn't we display only the factors that are allowed to be selected in treshold Access role?
So then the only "Invalid" scenario is that the password cannot be alone, some other might be added in the future sure.

User need to select at least 1 factor -> Insufficient

It is recommended to select at least 2 factors -> Suboptimal

Any other invalid combinations for a specific role (Access) -> Invalid

So the Wallet will display red for 1 and 3?

So then the only "Invalid" scenario is that the password cannot be alone, some other might be added in the future sure.

Exactly

So the Wallet will display red for 1 and 3?

Yes. 3 will have a "read more" where the user can read more about the rules

GhenadieVP · 2024-12-11T11:35:02Z

crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs

+        })
+    }
+
+    pub fn selected_factor_sources_for_role_status(


So this is meant to be used in Select Factors for Transactions screen? If so, do we need an instance of a builder to validate the selected combination of the factors?

Additionally, the fact that it returns SelectedFactorSourcesForRoleStatus, implies that the builder might contain invalid factors, but AFAIK we are actively trying to prevent this.

do we need an instance of a builder

Not necessarily, @CyonAlexRDX suggested this. Also, it seems easier since all the current validation functions are on a builder instance.

implies that the builder might contain invalid factors

Not the factors themselves, but in the context of a specific role.

GhenadieVP · 2024-12-11T11:36:57Z

crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs

+pub fn security_shield_builder_sorted_factor_sources_for_selection(
+    factor_sources: Vec<FactorSource>,
+) -> Vec<FactorSource> {


Can't Sargon load the list of factor sources itself, sort and filter the appropriate factor sources?

GhenadieVP · 2024-12-11T11:39:16Z

crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs

+// ==== STATIC ====
+// ================
+impl SecurityShieldBuilder {
+    pub fn sorted_factor_sources_for_selection(


Isn't this specific for Select Factors for Transactions, aka for Access role? If so, then it should be specific. Also not all factor sources can be used for Access role, thus some filtering need to happen as well AFAIK.

Security questions and trusted contact (not supported in MVP) cannot be used for Access role, we can filter them out

correct, these should be filtered out accordingly.

GhenadieVP · 2024-12-11T11:40:15Z

crates/sargon/src/profile/v100/factors/factor_source_kind.rs

@@ -118,6 +118,20 @@ impl FactorSourceKind {
    }
 }

+impl FactorSourceKind {
+    pub fn display_order(&self) -> u8 {


IMO in the context of this PR, it should be specific display order for Select Factor for Access Role, not generic order.

CyonAlexRDX

LGTM .

We can always iterate if we need adjust it

GhenadieVP

Needs to include the filtering also

danvleju-rdx added 4 commits December 9, 2024 14:17

wip

5b8f4b5

wip

126874b

wip

74a7ccd

bump version

e85b284

Sajjon reviewed Dec 9, 2024

View reviewed changes

CyonAlexRDX requested changes Dec 9, 2024

View reviewed changes

danvleju-rdx added 2 commits December 11, 2024 13:04

wip

8a0cf18

Merge branch 'main' into ABW-3969-shield-setup-select-factors

a37ad77

GhenadieVP reviewed Dec 11, 2024

View reviewed changes

CyonAlexRDX approved these changes Dec 11, 2024

View reviewed changes

GhenadieVP approved these changes Dec 11, 2024

View reviewed changes

address feedback

636caf9

danvleju-rdx merged commit a2b1692 into main Dec 11, 2024
13 checks passed

danvleju-rdx deleted the ABW-3969-shield-setup-select-factors branch December 11, 2024 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABW-3969] Shield Setup Select Factors - utility functions #297

[ABW-3969] Shield Setup Select Factors - utility functions #297

danvleju-rdx commented Dec 9, 2024

Sajjon Dec 9, 2024

CyonAlexRDX Dec 9, 2024

Sajjon Dec 9, 2024

Sajjon Dec 9, 2024

Sajjon Dec 9, 2024 •

edited

Loading

danvleju-rdx Dec 11, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

CyonAlexRDX Dec 9, 2024

CyonAlexRDX Dec 9, 2024

GhenadieVP Dec 11, 2024

danvleju-rdx Dec 11, 2024

GhenadieVP Dec 11, 2024

danvleju-rdx Dec 11, 2024

GhenadieVP Dec 11, 2024

danvleju-rdx Dec 11, 2024

GhenadieVP Dec 11, 2024 •

edited

Loading

danvleju-rdx Dec 11, 2024

GhenadieVP Dec 11, 2024

GhenadieVP Dec 11, 2024

danvleju-rdx Dec 11, 2024

GhenadieVP Dec 11, 2024

GhenadieVP Dec 11, 2024

CyonAlexRDX left a comment

GhenadieVP left a comment

[ABW-3969] Shield Setup Select Factors - utility functions #297

[ABW-3969] Shield Setup Select Factors - utility functions #297

Conversation

danvleju-rdx commented Dec 9, 2024

Changelog

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

Sajjon Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

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

GhenadieVP Dec 11, 2024 • 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

CyonAlexRDX left a comment

Choose a reason for hiding this comment

GhenadieVP left a comment

Choose a reason for hiding this comment

Sajjon Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

GhenadieVP Dec 11, 2024 •

edited

Loading