-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rules for Roles #286
Merged
CyonAlexRDX
merged 72 commits into
main
from
ac/security_structures_matrices_roles_with_rules
Dec 6, 2024
Merged
Rules for Roles #286
CyonAlexRDX
merged 72 commits into
main
from
ac/security_structures_matrices_roles_with_rules
Dec 6, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GhenadieVP
reviewed
Dec 2, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments, need to review further.
crates/sargon-uniffi/src/profile/mfa/security_structures/builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon-uniffi/src/profile/mfa/security_structures/builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon-uniffi/src/profile/mfa/security_structures/builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon-uniffi/src/profile/mfa/security_structures/builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon-uniffi/src/profile/mfa/security_structures/builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/builder/matrix_builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/builder/matrix_builder.rs
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/matrix_of_factor_instances.rs
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/matrix_of_factor_sources.rs
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/roles/builder/roles_builder.rs
Outdated
Show resolved
Hide resolved
GhenadieVP
reviewed
Dec 2, 2024
crates/sargon/src/profile/mfa/security_structures/roles/builder/roles_builder.rs
Outdated
Show resolved
Hide resolved
GhenadieVP
reviewed
Dec 2, 2024
crates/sargon/src/profile/mfa/security_structures/roles/builder/roles_builder.rs
Outdated
Show resolved
Hide resolved
…MatrixBuilderOrBuilt. Allowing us to drop PhantomData
… to threshold and threshold was 0. add doc support for macros. doc types.
…set_recovery_and_confirmation_role_state`
…einterpret` crate instead of `paste` crate.
matiasbzurovski
approved these changes
Dec 5, 2024
crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs
Outdated
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/builder/matrix_template.rs
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/builder/matrix_template.rs
Show resolved
Hide resolved
crates/sargon/src/profile/mfa/security_structures/matrices/matrix_of_factor_source_ids.rs
Outdated
Show resolved
Hide resolved
* Add FactorSourceCategory & checking of SecurityShieldBuildingPrerequisites
CyonAlexRDX
deleted the
ac/security_structures_matrices_roles_with_rules
branch
December 6, 2024 13:38
CyonAlexRDX
restored the
ac/security_structures_matrices_roles_with_rules
branch
December 6, 2024 13:41
CyonAlexRDX
deleted the
ac/security_structures_matrices_roles_with_rules
branch
December 18, 2024 07:06
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
The UniFFI exported
SecurityShieldBuilder
is a reference type which is stateful and hostsSHOULD not keep any state relating to factors in its ViewMode/Reducer.State, hosts should instead
hold a
builder: SecurityShieldBuilder
and use that and only that for state!For Swift we might need wrap that reference type in a value type to play well with
Reducer.State
?Maybe using @shared state!
Note
I've changed from mozilla/UniFFI to Sajjon/UniFFI (my fork) awaiting merge of JNA bug fix
into Mozilla: mozilla/uniffi-rs#2344
Description
Impl of rules for roles
This PR implements a
MatrixOfFactorSourceIdsBuilder
- which uses a newRoleWithFactorSourceIdsBuilder
- one for each role:Each of these roles builders validates any mutation done to it and can answer two basic queries:
Furthermore, we can call a
validate
method onMatrixOfFactorSourceIdsBuilder
, which aparts from validating each role builder in isolation; also validates the roles in combination with each other (and also validates thenumber_of_days_until_auto_confirm
).Finally the
MatrixOfFactorSourceIdsBuilder
of course has abuild
method, which callsvalidate
and if valid returns a builtMatrixOfFactorSourceIds
.Important
Building happens on
FactorSourceID
level - since it is what we are going to store into Profile anyway, and it is trivial to map fromFactorSource
->FactorSourceID
(just call.factor_source_id()
on the factor source). We note UI in hosts works on FactorSource level, but the builder works with FactorSourceIDs. We have throwing conversion from a builtMatrixOfFactorSourceIds
->MatrixOfFactorSources
which we can use for screen where we wanna display a Security Shield already saved in Profile.API
See unit test in Sargon-Uniffi to get a feeling of the API, but will paste parts of it here:
API EXAMPLE HERE (Click me‼️ )
Implementation
All past implementations of
SecurityStructureOf*
,MatrixOf
andRoleWith
have been re-implemented/replaced (they were declared by overly-complex macros in Sargon which was copy pasted over to sargon-uniffi crate. In fact I started this work by trying to implement all rules inside the macro, but it quickly became much too complex on something which already was too complex.).The current implementation uses some macros inside the sargon-uniffi crate for conversion between Sargon and Sargon-UniFFI types - but these very much more well written than the old ones, and in fact quite "easy" to understand. They are much more well structured, much shorter and also documented. Here is the macro for
role_conversion
and here is the macro formatrix_conversion
. Why are those macros needed? Well I did not want to let UniFFI crate dictate how Sargon implements its types - which theInternalConversion
proc-macro does! It dictates that the fields in Sargon and Sargon-UniFFI crates map 1:1 and also requires all fields in Sargon to bepub
- that is not how I've implemented the types in Sargon!The implementation of the types in Sargon uses one single type for ALL role types (except
GeneralRoleWithHierarchicalDeterministicFactorInstances
):The type is named "BuilderOrBuilt" because it is used by both the (role)builders and the "built" roles!
All role builder types are then declared by using the:
like so:
Note the
const R: u8
is 🪄 Rust magic ✨🔮 it is a "Value Generics"! Not a generic type, but a generic value! This allows me to create distinct RoleBuilders, as if they were different types, using a typealias. But I dont have to declare distinct "TagTypes" (like SwiftTagged
), instead I can just use three constpub const ROLE_PRIMARY: u8 = 0;
which is neat! Furthermore, since I declareROLE_PRIMARY = 0
I can do this:Assert<{ R > ROLE_PRIMARY }>: IsTrue,
This is soo cool! We can put shared behaviour between only theRecoveryRoleBuilder
andConfirmationRoleBuilder
builder! Very convenient!(
which does require us to put:
but feels OK, not a huge pain to remove it if the feature
generic_const_exprs
wont be stabilized....)
Rules
I've implemented the rules without checking the const value, rather relying on runtime checks instead of compile time checks, like so
match self.role()
and doing validation. I think it is an easier and more flexible implementation, and I have 100% code coverage for all rules. See e.g.validation_add
Templates
I've implemented all example Security Shield configs listed in table (except the last two which required "Custodian" FS). This is cool! I've implemented the vision I had 1,5 years ago! Like so:
This might in fact be a valid implementation of
Automatic Shield Construction
(see below underFuture Work
). This allows us to create aMatrixOfFactorSourceIds
without using a builder - instead usingMatrixTemplate::materialize(template, factors_in_profile)
and it will always produce a valid Matrix! We can in fact in the future build UI where user can select any of these predefined templates! But I did them mostly to be able to "reuse matrices" between tests - and not have to build them all the time. These templates are useful for tests and for can be really useful for end user!Never construct
MatrixOfFactor
ourselves!We never construct any
MatrixOfFactor***
ourselves (apart from usingsample()
/sample_other()
ofc...)MatrixOfFactorSourceIds
These are two ways by which we can construct
MatrixOfFactorSourceIds*
:MatrixBuilder
(FactorSourceID
level)MatrixTemplate
+ FactorSourceIds collection, e.g. those in Profile or for tests usingALL_FACTOR_SOURCE_ID_SAMPLES_INC_NON_HD
)MatrixOFFactorSources
Use
MatrixOfFactorSourceIds
andtry_from
with a iter ofFactorSources
, there is anew
ctor which is throwing (of the referencedFactorSourceID
was not found among the provided FactorSources).MatrixOfFactorInstances
This is the process of using the
FactorInstancesProvider
(FIP) or actually one of its "specialized adopters" theSecurifyEntityFactorInstancesProvider
, see gated behindcfg(test)
method__OFFLINE_ONLY_securify_accounts
onSargonOS
. Which derives FactorInstances from aSecurityStructureOfFactorSources
.Note
We should probably update
FactorInstancesProvider
/SecurifyEntityFactorInstancesProvider
and alsoKeysCollector
to work onFactorSourceId
level instead ofFactorSource
!But this depends a bit on @micbakos-rdx work with
Interactors
. We can useFactorSourceID
s everywhere and then we can let the interactor on the Host side be responsible for doing the lookupFactorSourceID -> FactorSource
if it needs to show more info about the factor source (such as "last used" / "label" etc).Future Work
TransactionManifest
building supportI did not carry over the
Into<ScryptoAccessRule>
which for a*RoleWithFactorInstances
had onmain
branch. I will do that in a coming PR - also with support forMatrixOfFactorInstances
!Automatic Shield Construction
In
Automatic Security Shield Construction
section of the Confluence page, Matt lays out a heuristics for trying to build a shield solely based on the FactorSources user has in Profile. That has not yet been done.Tip
However, since I did implement the Configs one could possible very easily implement automatic shield construction by
MatrixTemplate::all().iter(|template| template.materialize(profile.factors).first()
!