Skip to content

Commit

Permalink
Upgrade ShieldBuilder to validate FactorSourceKind for ROLA facto…
Browse files Browse the repository at this point in the history
…rs (#314)

set_authentication_signing_factor now validates the FactorSourceKind, and we can query shield builder for which FactorSourceKinds are supported for it
  • Loading branch information
CyonAlexRDX authored Dec 20, 2024
1 parent 87a2400 commit 191f729
Show file tree
Hide file tree
Showing 6 changed files with 156 additions and 10 deletions.
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/sargon-uniffi/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "sargon-uniffi"
# Don't forget to update version in crates/sargon/Cargo.toml
version = "1.1.93"
version = "1.1.94"
edition = "2021"
build = "build.rs"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,29 @@ impl SecurityShieldBuilder {

#[uniffi::export]
impl SecurityShieldBuilder {
/// "Statically" queries which FactorSourceKinds are disallowed for authentication signing.
pub fn disallowed_factor_source_kinds_for_authentication_signing(
&self,
) -> Vec<FactorSourceKind> {
sargon::SecurityShieldBuilder::disallowed_factor_source_kinds_for_authentication_signing().into_type()
}

/// "Statically" queries which FactorSourceKinds are allowed for authentication signing.
pub fn allowed_factor_source_kinds_for_authentication_signing(
&self,
) -> Vec<FactorSourceKind> {
sargon::SecurityShieldBuilder::allowed_factor_source_kinds_for_authentication_signing().into_type()
}

/// "Statically" queries if `factor_source_kind`` is allowed for authentication signing.
pub fn is_allowed_factor_source_kind_for_authentication_signing(
&self,
factor_source_kind: FactorSourceKind,
) -> bool {
sargon::SecurityShieldBuilder::is_allowed_factor_source_kind_for_authentication_signing(
factor_source_kind.clone().into())
}

pub fn addition_of_factor_source_of_kind_to_primary_threshold_is_fully_valid(
&self,
factor_source_kind: FactorSourceKind,
Expand Down Expand Up @@ -561,6 +584,35 @@ mod tests {
#[allow(clippy::upper_case_acronyms)]
type SUT = SecurityShieldBuilder;

#[test]
fn rola() {
let sut = SUT::new();
assert_eq!(sut.disallowed_factor_source_kinds_for_authentication_signing().len(), sargon::SecurityShieldBuilder::disallowed_factor_source_kinds_for_authentication_signing().len());

assert_eq!(sut.allowed_factor_source_kinds_for_authentication_signing().len(), sargon::SecurityShieldBuilder::allowed_factor_source_kinds_for_authentication_signing().len());

assert!(
sut.is_allowed_factor_source_kind_for_authentication_signing(
FactorSourceKind::Device
)
);
assert!(
!sut.is_allowed_factor_source_kind_for_authentication_signing(
FactorSourceKind::Password
)
);
assert!(
!sut.is_allowed_factor_source_kind_for_authentication_signing(
FactorSourceKind::TrustedContact
)
);
assert!(
!sut.is_allowed_factor_source_kind_for_authentication_signing(
FactorSourceKind::SecurityQuestions
)
);
}

#[test]
fn test() {
let sut = SUT::new();
Expand Down
2 changes: 1 addition & 1 deletion crates/sargon/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "sargon"
# Don't forget to update version in crates/sargon-uniffi/Cargo.toml
version = "1.1.93"
version = "1.1.94"
edition = "2021"
build = "build.rs"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,26 @@ impl SecurityShieldBuilder {
self
}

/// Sets the ROLA (authentication signing) factor to `new` if and only if
/// `new` is not Some(invalid), where invalid is defined by `allowed_factor_source_kinds_for_authentication_signing`,
/// that is, it checks the `FactorSourceKind` of the factor, according to the
/// rules defined in [doc][doc].
///
/// [doc]: https://radixdlt.atlassian.net/wiki/spaces/AT/pages/3758063620/MFA+Rules+for+Factors+and+Security+Shield
pub fn set_authentication_signing_factor(
&self,
new: impl Into<Option<FactorSourceID>>,
) -> &Self {
*self.authentication_signing_factor.write().unwrap() = new.into();
let new = new.into();
if let Some(new) = new.as_ref() {
if !Self::is_allowed_factor_source_kind_for_authentication_signing(
new.get_factor_source_kind(),
) {
warn!("Invalid FactorSourceKind for ROLA");
return self;
}
}
*self.authentication_signing_factor.write().unwrap() = new;
self
}

Expand Down Expand Up @@ -295,6 +310,30 @@ impl SecurityShieldBuilder {
}

impl SecurityShieldBuilder {
pub fn disallowed_factor_source_kinds_for_authentication_signing(
) -> IndexSet<FactorSourceKind> {
IndexSet::from_iter([
FactorSourceKind::Password,
FactorSourceKind::SecurityQuestions,
FactorSourceKind::TrustedContact,
])
}

pub fn allowed_factor_source_kinds_for_authentication_signing(
) -> IndexSet<FactorSourceKind> {
let all = FactorSourceKind::all();
let disallowed =
Self::disallowed_factor_source_kinds_for_authentication_signing();
all.difference(&disallowed).cloned().collect()
}

pub fn is_allowed_factor_source_kind_for_authentication_signing(
factor_source_kind: FactorSourceKind,
) -> bool {
Self::allowed_factor_source_kinds_for_authentication_signing()
.contains(&factor_source_kind)
}

/// Returns `true` for `Ok` and `Err(NotYetValid)`.
pub fn addition_of_factor_source_of_kind_to_primary_threshold_is_valid_or_can_be(
&self,
Expand Down Expand Up @@ -583,6 +622,49 @@ mod tests {
assert_eq!(sut.get_threshold(), 42);
}

#[test]
fn allowed_rola() {
let allowed =
SUT::allowed_factor_source_kinds_for_authentication_signing();
assert_eq!(
allowed,
IndexSet::<FactorSourceKind>::from_iter([
FactorSourceKind::LedgerHQHardwareWallet,
FactorSourceKind::ArculusCard,
FactorSourceKind::OffDeviceMnemonic,
FactorSourceKind::Device,
])
);
}

#[test]
fn is_allowed_rola() {
let disallowed =
SUT::disallowed_factor_source_kinds_for_authentication_signing();
assert!(disallowed.iter().all(|k| {
!SUT::is_allowed_factor_source_kind_for_authentication_signing(*k)
}));
}

#[test]
fn test_invalid_rola_kind_does_not_change_rola() {
let sut = SUT::new();
assert!(sut.get_authentication_signing_factor().is_none());
let valid = FactorSourceID::sample_device();
sut.set_authentication_signing_factor(valid);
assert_eq!(sut.get_authentication_signing_factor().unwrap(), valid);

let invalid_factors = vec![
FactorSourceID::sample_password(),
FactorSourceID::sample_security_questions(),
FactorSourceID::sample_trusted_contact(),
];
for invalid in invalid_factors {
sut.set_authentication_signing_factor(invalid); // should not have changed anything
}
assert_eq!(sut.get_authentication_signing_factor().unwrap(), valid);
}

#[test]
fn test() {
let sut = SUT::default();
Expand Down
22 changes: 17 additions & 5 deletions crates/sargon/src/profile/v100/factors/factor_source_kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::prelude::*;
Eq,
Hash,
PartialOrd,
enum_iterator::Sequence,
Ord,
)]
pub enum FactorSourceKind {
Expand Down Expand Up @@ -93,6 +94,13 @@ pub enum FactorSourceKind {
TrustedContact,
}

impl FactorSourceKind {
/// All FactorSourceKind
pub fn all() -> IndexSet<Self> {
enum_iterator::all::<Self>().collect()
}
}

impl FactorSourceKind {
pub fn discriminant(&self) -> String {
// We do `to_value.as_str` instead of `to_string(_pretty)` to avoid unwanted quotation marks around the string.
Expand Down Expand Up @@ -168,6 +176,15 @@ mod tests {
#[allow(clippy::upper_case_acronyms)]
type SUT = FactorSourceKind;

#[test]
fn ord() {
assert!(SUT::Device < SUT::TrustedContact);
let unsorted = SUT::all(); // is in fact sorted
let mut sorted = unsorted.clone();
sorted.sort();
assert_eq!(unsorted, sorted);
}

#[test]
fn string_roundtrip() {
use FactorSourceKind::*;
Expand Down Expand Up @@ -214,11 +231,6 @@ mod tests {
);
}

#[test]
fn ord() {
assert!(SUT::Device < SUT::TrustedContact);
}

#[test]
fn discriminant() {
assert_eq!(SUT::Device.discriminant(), "device");
Expand Down

0 comments on commit 191f729

Please sign in to comment.