-
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
FIP: Support mix of Accounts & Personas #306
Changes from all commits
c91f8f6
2d3259e
502ba59
7eb9e2d
4f39117
bf57732
ed7f78d
9f7790d
332c6a9
9cded45
8faaf3e
5efcfff
20fccd0
f2ba3fa
642c9cc
d68c597
9c8e7bc
6e56fa2
a0f3e6a
15200f5
67bc386
e54a5d3
3ba71d3
5940db0
a2f77c8
48a568f
ce3e9fd
01288fa
48874f9
f5319b1
ed84dba
8da5a2d
9a367a4
59d597f
e3de138
c0e0b42
2dc1484
64c17f8
4b477c6
8ae342f
1da8628
22c5855
41b1a11
4b8c041
ccf46b6
8e4c576
f269392
e8ff22f
632f907
9d89a31
93bc314
18009fa
d67aea5
1ff2f54
b2ca5b9
cd456d2
e8d16d6
6715385
5e29d97
3a21681
d69166b
a8146bb
92fe297
8d0d109
4d0b9ab
ee84209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,6 +109,11 @@ impl SecurityShieldBuilder { | |
self.get(|builder| builder.get_name()) | ||
} | ||
|
||
pub fn get_authentication_signing_factor(&self) -> Option<FactorSourceID> { | ||
self.get(|builder| builder.get_authentication_signing_factor()) | ||
.map(|x| x.into()) | ||
} | ||
|
||
pub fn get_primary_threshold_factors(&self) -> Vec<FactorSourceID> { | ||
self.get_factors(|builder| builder.get_primary_threshold_factors()) | ||
} | ||
|
@@ -135,6 +140,17 @@ impl SecurityShieldBuilder { | |
self.set(|builder| builder.set_name(&name)); | ||
} | ||
|
||
pub fn set_authentication_signing_factor( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For other factors we seem to follow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add / remove makes sense when we support multiple factors. This is always one single |
||
&self, | ||
new: Option<FactorSourceID>, | ||
) { | ||
self.set(|builder| { | ||
builder.set_authentication_signing_factor( | ||
new.clone().map(|x| x.into_internal()), | ||
) | ||
}); | ||
} | ||
|
||
pub fn remove_factor_from_all_roles( | ||
&self, | ||
factor_source_id: FactorSourceID, | ||
|
@@ -756,6 +772,18 @@ mod tests { | |
sut.remove_factor_from_confirmation(f.clone()); | ||
assert_eq!(xs, sut.get_confirmation_factors()); | ||
|
||
assert_eq!( | ||
sut.validate().unwrap(), | ||
SecurityShieldBuilderInvalidReason::MissingAuthSigningFactor | ||
); | ||
sut.set_authentication_signing_factor(Some( | ||
FactorSourceID::sample_device_other(), | ||
)); | ||
assert_eq!( | ||
sut.get_authentication_signing_factor(), | ||
Some(FactorSourceID::sample_device_other()) | ||
); | ||
|
||
let v0 = sut.validate(); | ||
let v1 = sut.validate(); // can call validate many times! | ||
assert_eq!(v0, v1); | ||
|
@@ -764,6 +792,10 @@ mod tests { | |
let shield = sut.build().unwrap(); // can call build many times! | ||
assert_eq!(shield0, shield); | ||
|
||
assert_eq!( | ||
shield.authentication_signing_factor, | ||
FactorSourceID::sample_device_other() | ||
); | ||
assert_eq!(shield.metadata.display_name.value, "S.H.I.E.L.D."); | ||
assert_eq!( | ||
shield.matrix_of_factors.primary_role.override_factors, | ||
|
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.
Do we need the other two purposes above? Does
SecurifyingAccountsAndPersonas
mean necessarilyand
, or it can happen to have only accounts or personas?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.
Yes if you only security account or only security personas those other are used.
But host will most likely map all variants to same Crowdin key anyway...
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.
In the host API, it will send a list of AccountOrPersona right? and then in Sargon you map to specific purpose based on the input, I think I've seen this logic in this PR.