Superseded by: https://lore.proxmox.com/all/20250603091256.40923-1-a.abra...@proxmox.com/ > Dominik Csapak <d.csa...@proxmox.com> hat am 17.02.2025 09:54 CET geschrieben: > > > On 2/6/25 13:01, Alexander Abraham wrote: > > Two things were added to the proxmox-openid crate to fix > > bug #5076: i) the function to require strict audience checking > > was called and ii) an extra verifier function was added to check > > if the configured audiences match the receieved audiences. > > Hi, > > first, it would be nice if the three relevant patches > (proxmox/access-control/manager) would get a > combined cover-letter. that way it's easier to see that the patches > belong together. > > aside from that, it would also be good if the commit message contain > a 'why'. The 'what' and 'how' should (most often) be self-evident from > the diff, but the why isn't most of the time. > > E.g. a short sentence like: We want to verify additional audiences because ... > makes it much easier to reason about the intentions later on. > > a few smaller comments inline > > > > > Signed-off-by: Alexander Abraham <a.abra...@proxmox.com> > > --- > > proxmox-openid/src/lib.rs | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs > > index fe65fded..396f55cd 100644 > > --- a/proxmox-openid/src/lib.rs > > +++ b/proxmox-openid/src/lib.rs > > @@ -1,10 +1,9 @@ > > #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] > > > > -use std::path::Path; > > - > > use anyhow::{format_err, Error}; > > use serde::{Deserialize, Serialize}; > > use serde_json::Value; > > +use std::path::Path; > > these two hunks seem unrelated (and wrong), please leave the > 'std' imports seperate > > > > > mod http_client; > > pub use http_client::http_client; > > @@ -53,6 +52,8 @@ pub struct OpenIdConfig { > > pub prompt: Option<String>, > > #[serde(skip_serializing_if = "Option::is_none")] > > pub acr_values: Option<Vec<String>>, > > + #[serde(skip_serializing_if = "Option::is_none")] > > + pub aud: Option<Vec<String>>, > > } > > > > pub struct OpenIdAuthenticator { > > @@ -204,21 +205,32 @@ impl OpenIdAuthenticator { > > .set_pkce_verifier(private_auth_state.pkce_verifier()) > > .request(http_client) > > .map_err(|err| format_err!("Failed to contact token endpoint: > > {}", err))?; > > - > > any special reason why you remove the whitespace here? > > > - let id_token_verifier: CoreIdTokenVerifier = > > self.client.id_token_verifier(); > > let id_token_claims: &CoreIdTokenClaims = token_response > > .extra_fields() > > .id_token() > > .expect("Server did not return an ID token") > > - .claims(&id_token_verifier, &private_auth_state.nonce) > > + .claims( > > + &((self.client.id_token_verifier() as CoreIdTokenVerifier) > > is this cast here really necessary? AFAICS it shouldn't ? > > > + .require_audience_match(true) > > + .set_other_audience_verifier_fn(|aud| { > > + let curr_aud: &String = &**aud; > > clippy warns here: > > deref which would be done by auto-deref > > > so you can just write: > > let curr_aud: &String = aud; > > > + if &self.config.client_id == curr_aud { > > + true > > + } else { > > + match self.config.aud.as_ref() { > > + Some(confd_auds) => > > confd_auds.contains(curr_aud), > > + None => false, > > + } > > + } > > + })), > > + &private_auth_state.nonce, > > + ) > > .map_err(|err| format_err!("Failed to verify ID token: {}", > > err))?; > > - > > why the white space removal here too? > > > let userinfo_claims: GenericUserInfoClaims = self > > .client > > .user_info(token_response.access_token().to_owned(), None)? > > .request(http_client) > > .map_err(|err| format_err!("Failed to contact userinfo > > endpoint: {}", err))?; > > - > > and here > > > Ok((id_token_claims.clone(), userinfo_claims)) > > } > > > > @@ -230,9 +242,7 @@ impl OpenIdAuthenticator { > > ) -> Result<Value, Error> { > > let (id_token_claims, userinfo_claims) = > > self.verify_authorization_code(code, private_auth_state)?; > > - > > let mut data = serde_json::to_value(id_token_claims)?; > > - > > and here > > > let data2 = serde_json::to_value(userinfo_claims)?; > > > > if let Some(map) = data2.as_object() { > > @@ -243,7 +253,6 @@ impl OpenIdAuthenticator { > > data[key] = value.clone(); > > } > > } > > - > > and here > > IMO, white space cleanup can be fine, but please as a separate (upfront) > patch, > so it does not pollute the actual patch > > that said, in this case, I'd just leave the empty lines in place > > > Ok(data) > > } > > }
_______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
