feat(station): validate AllowListed and AllowListedByMetadata rules #570
Conversation
…e applied to Transfers
| for referencing_named_rule in referencing_named_rules.iter() { | ||
| if policy.rule.has_named_rule_id(referencing_named_rule) { | ||
| validate_rule_for_specifier(rule, &policy.specifier).map_err(|e| { | ||
| NamedRuleError::IncompatibleWithLinkedPolicy { | ||
| policy_id: Uuid::from_bytes(policy.id).hyphenated().to_string(), | ||
| error: e.to_string(), | ||
| } | ||
| })?; | ||
| } | ||
| } |
There was a problem hiding this comment.
| for referencing_named_rule in referencing_named_rules.iter() { | |
| if policy.rule.has_named_rule_id(referencing_named_rule) { | |
| validate_rule_for_specifier(rule, &policy.specifier).map_err(|e| { | |
| NamedRuleError::IncompatibleWithLinkedPolicy { | |
| policy_id: Uuid::from_bytes(policy.id).hyphenated().to_string(), | |
| error: e.to_string(), | |
| } | |
| })?; | |
| } | |
| } | |
| validate_rule_for_specifier_with_substitution(policy.rule, &policy.specifier, id, rule).map_err(|e| { | |
| NamedRuleError::IncompatibleWithLinkedPolicy { | |
| policy_id: Uuid::from_bytes(policy.id).hyphenated().to_string(), | |
| error: e.to_string(), | |
| } | |
| })?; |
where the function validate_rule_for_specifier_with_substitution performs a traversal of RequestPolicyRule just like validate_rule_for_specifier, but whenever it encounters the named rule with id, then it continues the traversal on rule instead of the current child. This terminates assuming there are no (indirect) circular references (which we should ensure separately).
I pushed a counterexample why the current algorithm is not correct.
There was a problem hiding this comment.
Addressed in f3e14ab
There was a problem hiding this comment.
I'd still simplify:
/// Validates that the named rule is compatible with the policies that reference it.
/// It traverses all linked policy rules recursively and assumes no circular references.
fn validate_policy_compatibility(
id: &NamedRuleId,
rule: &RequestPolicyRule,
) -> ModelValidatorResult<NamedRuleError> {
let policies = REQUEST_POLICY_REPOSITORY.list();
for policy in policies {
validate_rule_for_specifier(&policy.rule, &policy.specifier, &[(id, rule)])
.map_err(|e| NamedRuleError::IncompatibleWithLinkedPolicy {
policy_id: Uuid::from_bytes(policy.id).hyphenated().to_string(),
error: e.to_string(),
})?;
}
Ok(())
}
This way, we only traverse all policy rules once (better worst-case runtime) and the algorithm is simpler (more robust, less changes for logical bugs).
There was a problem hiding this comment.
Changed as discussed.
There was a problem hiding this comment.
Should we also trigger the validation in post-upgrade?
There was a problem hiding this comment.
I dont think that would be good UX. The user would need to understand the nuances of the policy/named_rule/rule system in order to get past it. Let's discuss with Kepler next week?
There was a problem hiding this comment.
I dont think that would be good UX. The user would need to understand the nuances of the policy/named_rule/rule system in order to get past it.
I agree, but now we risk having invalid stuff in the upgraded station and potentially getting "stuck" if the validation is on a critical path.
Let's discuss with Kepler next week?
Indeed.
There was a problem hiding this comment.
To get stuck what would need to happen? You can always edit a policy's rule directly, which should not involve validation of anything else. That way you should be able to fix all invalid policies. Then you can fix the named rules if any of them are invalid, since no policies will use them. Can you think of an unrecoverable scenario?
There was a problem hiding this comment.
E.g., what if there's no valid policy according to the new rules: will the user still be able to execute a request? I guess there are two options:
- fail in post-upgrade if there's any violation of the new rules;
- add end-to-end tests that a violation of the new rules can be recovered after an upgrade.
This PR adds validations to make sure rules containing AllowListed and AllowListedByMetadata can only be applied to Transfer requests by: