RUST-1670: Add ALLOWED_HOSTS checking for human flow OIDC #1042
Conversation
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I tried to make this return an Iterator to avoid allocating Vecs, but I'm not sure how to handle errors internally without collecting to <Result<Vec<_>>.
There was a problem hiding this comment.
Ok, I found a solution, and a deeper understanding of impl return. I should have remembered it's existential not universal (for good reason).
There was a problem hiding this comment.
Now that you've solved it, I think Result<Vec<&'a str>> would actually be better :) That'll eagerly validate the list of hosts rather than only erroring if the iteration happens to hit an invalid value, and the overhead of Vec might not even be more than the overhead of Box<dyn ..> depending on list length and Iterator dynamic dispatch size.
There was a problem hiding this comment.
That is true for short lists. This is sort of optimizing for large lists of allowed hosts specified by users, which is probably pretty unlikely.
src/client/auth/oidc.rs
Outdated
| } | ||
|
|
||
| fn get_allowed_hosts<'a>( | ||
| mechanism_properties: &'a Option<Document>, |
There was a problem hiding this comment.
Rust convention is to always accept options as concrete types, i.e. Option<&'a Document>.
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Now that you've solved it, I think Result<Vec<&'a str>> would actually be better :) That'll eagerly validate the list of hosts rather than only erroring if the iteration happens to hit an invalid value, and the overhead of Vec might not even be more than the overhead of Box<dyn ..> depending on list length and Iterator dynamic dispatch size.
isabelatkinson
left a comment
isabelatkinson
left a comment
There was a problem hiding this comment.
impl looks good! I just have a few minor style suggestions, no need for re-review
src/client/auth.rs
Outdated
| // "127.0.0.1", | ||
| // "::1", | ||
| // ] | ||
| if has_mechanism_properties { |
There was a problem hiding this comment.
suggest using and_then (flat map) here instead of using has_mechanism_properties and unwrapping:
if let Some(allowed_hosts) = credential
.mechanism_properties
.as_ref()
.and_then(|p| p.get("ALLOWED_HOSTS"))
{
// check for array
}
src/client/auth.rs
Outdated
| .mechanism_properties | ||
| .as_ref() | ||
| .map_or(false, |p| p.contains_key("PROVIDER_NAME")); | ||
| let has_mechanism_properties = credential.mechanism_properties.is_some(); |
There was a problem hiding this comment.
I think this change can be reverted with the suggestion below -- it's nice to avoid unwraps (even if they're safe) when map works instead
src/client/auth/oidc.rs
Outdated
| if mechanism_properties.is_none() { | ||
| return Ok(Vec::from(DEFAULT_ALLOWED_HOSTS)); | ||
| } | ||
| if let Some(allowed_hosts) = mechanism_properties.as_ref().unwrap().get("ALLOWED_HOSTS") { |
There was a problem hiding this comment.
nit: suggest flat mapping here as well, i.e.
if let Some(allowed_hosts) =
mechanism_properties.and_then(|p| p.get_array("ALLOWED_HOSTS").ok()) {
// iterate
} else {
// default
}
There was a problem hiding this comment.
wow, I totally missed get_array, that is so nice
3 participants
No description provided.