Set default security shield API #362

giannis-rdx · 2025-01-29T13:07:17Z

This PR is part of the ABW-4146.
It adds an API to change the default security shield

codecov · 2025-01-29T13:18:40Z

Codecov Report

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

Project coverage is 92.64%. Comparing base (3634378) to head (da8a962).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ogic/app_preferences/security_structures_update.rs	96.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   92.59%   92.64%   +0.04%     
==========================================
  Files        1172     1180       +8     
  Lines       26255    26618     +363     
  Branches       81       77       -4     
==========================================
+ Hits        24312    24660     +348     
- Misses       1932     1947      +15     
  Partials       11       11

Flag	Coverage Δ
kotlin	`97.63% <ø> (-0.10%)`	⬇️
rust	`92.30% <98.14%> (+0.10%)`	⬆️
swift	`92.86% <ø> (-0.18%)`	⬇️

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.

CyonAlexRDX · 2025-01-29T13:52:11Z

crates/system/os/factors/src/sargon_os_security_structures.rs

+                    });
+            }
+
+            Ok(p.app_preferences


Prefer creating a method for this inside Profile logic crate.

Better to delegate as much logic relating to profile changes as possible to Profile logic crate. And let SargonOS be a as dumb as possible "coordinator"

hmm you mean in the profile_update.rs, right?

…ic crate

Sajjon · 2025-01-30T08:31:12Z

crates/profile/logic/logic_SPLIT_ME/src/logic/profile_update.rs

+        &mut self,
+        shield_id: &SecurityStructureID,
+    ) -> Result<bool> {
+        Ok(self


why return Result?

If ID not found. Right.

So do you map to correct Error if not found?

as I wrote in #362 (review) there should be an update method on AppPreferences , which should actually should delegate to Security which should have this impl, but you need to change the impl to actually return an Error if not found - which you are not doing now. So Result<()> is correct, but now you are incorrectly returning Bool which you unconditionally wrap in a Result.

impl Security { /// # Throws /// Throws `CommonError:InvalidSecurityStructureID` if the structure identified by `shield_id` does not exist. fn update_security_structure_remove_flag_default( &mut self, shield_id: &SecurityStructureID, ) -> Result<()> { self .security_structures_of_factor_source_ids .try_update_with(shield_id, |s| { s.metadata.remove_flag(SecurityStructureFlag::Default) })) .map(|_| CommonError::InvalidSecurityStructureID { bad_value: shield_id.to_string() }) } }

and then ofc unit test all the way :)

I understand why you want reusable functions and not having profile logic in SargonOS, but I don't understand at all where am I supposed to place the new functions. The way the profile-logic is structured doesn't really help me.

Therefore we need the following functions:

update_security_structure_remove_flag_default: where this is supposed to go? in (profile crate) models/app_preferences, I see there is a query fun there has_gateway_with_url. But it doesn't seem the right place to me. Should it be in the profile_update? Should I create a security_structures_logic file under profile/logic?

update_security_structure_set_flag_default: same as above

get_default_scurity_structure: a new file query_app_preferences under profile/logic? or in the models/app_preferences?

set_default_security_structure: it looks like it has to to profile_update or something else?

Sajjon

I mean you should do this - favour composition of many small resusable functions, instead of a bigger function which is not reausable.

The idea of SargonOS is to not have Profile model logic. So it should delegate that logic to profile-logic crate.

// In os-factors crate
async fn set_default_security_structure(
    &self,
    shield_id: SecurityStructureID,
) -> Result<()> {
    let updated_ids = self.update_profile_with(|p| Ok(p.set_default_security_structure(shield_id)))?;

    // Emit event
    self.event_bus
        .emit(EventNotification::profile_modified(
            EventProfileModified::SecurityStructuresUpdated {
                ids: updated_ids,
            },
        ))
        .await;

    Ok(())
}

// In Profile Logic crate
impl AppPreferences {
    fn get_default_scurity_structure(&self) -> Option<SecurityStructureOfFactorSourceIds> {
        self
            .security
            .security_structures_of_factor_source_ids
            .iter()
            .find(|s| s.metadata.is_default());
    }
}
impl Profile {
    fn get_default_Security_structure(&self) -> Option<SecurityStructureOfFactorSourceIds> {
        self
            .app_preferences.get_default_security_structure()
    }

    /// Returns the list of IDs of updated SecurityShields - either one or two elements,
    /// depending on if shield identified by `shield_id` already was "default" shield or not.
    fn set_default_security_structure(
        &self,
        shield_id: SecurityStructureID,
    ) -> Result<<Vec<SecurityStructureID>> {

        let current_default_shield = self.get_default_Security_structure();

        let updated_ids = match current_default_shield {
            Some(ref current_default_shield) => {
                vec![current_default_shield.metadata.id, shield_id]
            }
            None => vec![shield_id],
        };

        if let Some(current_default_shield) = &current_default_shield {
            self.update_security_structure_remove_flag_default(
                &current_default_shield.metadata.id,
            )
        }
        p.update_security_structure_add_flag_default(&shield_id)

        updated_ids
    }
}

matiasbzurovski · 2025-01-30T08:49:37Z

...profile/models/security-structures/src/roles_matrices_structures/security_structure_flags.rs

+    /// Used to mark a Security Shield as "default". We can only have one.
+    Default,


should we call it Main as we do on FactorSourceFlag (despite in UI it will be treated as "default")?

If so, I would update the name of every related function as well

Well, it’s not exactly the same. “Main” indicates the primary or most important option, while “default” refers to something preselected. That’s why in UI, it is (correctly) defined as “default.”

Based on that, do you really think we should treat it as “main” in the code? 🤔

I think they are both (Device Factor Sources & Security Shields) kind of the same. Nothing prevents using a non-main factor source/security shield, but host apps will use the main one when either creating/securifying an entity.

Yeah main in code makes sense to call it

We have democracy, so if the majority agrees, I will change it 😄

matiasbzurovski · 2025-01-30T08:51:35Z

...file/models/security-structures/src/roles_matrices_structures/security_structure_metadata.rs

        )
    }
+
+    pub fn set_flag(&mut self, flag: SecurityStructureFlag) {


insert_flag ?

matiasbzurovski · 2025-01-30T08:54:43Z

crates/system/os/factors/src/sargon_os_security_structures.rs

+        assert!(event_bus_driver.recorded().iter().any(|e| e.event
+            == Event::ProfileModified {
+                change: EventProfileModified::SecurityStructuresUpdated {
+                    ids: vec![structure_ids_sample_other.metadata.id()]
+                }
+            }));


need to check ProfileSaved was emitted as well

Also, we should check that SecurityStructuresUpdated is emitted with two ids when there was an existent main shield, and with one id when there isn't

…tructures_update for AppPreferences logic

giannis-rdx added 4 commits January 29, 2025 14:45

added SecurityStructureFlag

7e31c2f

updated SecurityStructureMetadata with SecurityStructureFlag

7249c0b

added SecurityStructuresUpdated SecurityStructuresUpdated

fcbafd4

added and exposed set_default_security_structure

766e421

giannis-rdx requested review from CyonAlexRDX, GhenadieVP, micbakos-rdx, danvleju-rdx, sergiupuhalschi-rdx and matiasbzurovski January 29, 2025 13:07

giannis-rdx self-assigned this Jan 29, 2025

CyonAlexRDX reviewed Jan 29, 2025

View reviewed changes

delegate remove/add default flag of security structure to profile log…

5c9af2b

…ic crate

Sajjon reviewed Jan 30, 2025

View reviewed changes

matiasbzurovski requested changes Jan 30, 2025

View reviewed changes

giannis-rdx added 5 commits January 30, 2025 11:56

fixed result/mapping and other minor

d699d2e

added app_preferences directory and query_app_preferences, security_s…

01030de

…tructures_update for AppPreferences logic

update sargon_os_security_structures with delegating logic to profile

8698f0e

added throws

e5a3b4a

renamed default flag to main for security structures

da8a962

giannis-rdx requested review from matiasbzurovski, Sajjon and CyonAlexRDX January 30, 2025 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default security shield API #362

Set default security shield API #362

giannis-rdx commented Jan 29, 2025 •

edited by jira bot

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

CyonAlexRDX Jan 29, 2025

giannis-rdx Jan 29, 2025

giannis-rdx Jan 30, 2025

Sajjon Jan 30, 2025

Sajjon Jan 30, 2025

Sajjon Jan 30, 2025

Sajjon Jan 30, 2025

giannis-rdx Jan 30, 2025

Sajjon left a comment •

edited

Loading

matiasbzurovski Jan 30, 2025

giannis-rdx Jan 30, 2025

matiasbzurovski Jan 30, 2025

CyonAlexRDX Jan 30, 2025

giannis-rdx Jan 30, 2025

matiasbzurovski Jan 30, 2025

giannis-rdx Jan 30, 2025

matiasbzurovski Jan 30, 2025

		/// Used to mark a Security Shield as "default". We can only have one.
		Default,

Set default security shield API #362

Are you sure you want to change the base?

Set default security shield API #362

Conversation

giannis-rdx commented Jan 29, 2025 • edited by jira bot Loading

codecov bot commented Jan 29, 2025 • 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

Sajjon left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx commented Jan 29, 2025 •

edited by jira bot

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

Sajjon left a comment •

edited

Loading