Re: [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option

[Thomas Skinner <thomas@atskinner.net>]

On Fri, Jan 24, 2025 at 3:17 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> On December 16, 2024 5:14 am, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  pve-rs/src/openid/mod.rs | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
> > index 1fa7572..cd573ee 100644
> > --- a/pve-rs/src/openid/mod.rs
> > +++ b/pve-rs/src/openid/mod.rs
> > @@ -50,13 +50,18 @@ mod export {
> >      }
> >
> >      #[export(raw_return)]
> > -    pub fn verify_authorization_code(
> > +    pub fn verify_authorization_code_userinfo(
>
> we could either add a new wrapper like in proxmox-openid, keeping the
> old one around (until PVE 9.0)
>
> >          #[try_from_ref] this: &OpenId,
> >          code: &str,
> >          private_auth_state: PrivateAuthState,
> > +        disable_userinfo: bool,
>
> or make this an Option<bool> and not rename the fn so existing callers
> are not broken

Is this a backwards compatible fix? This would seem more preferable to
me and provide a reasonable default. If not, I can definitely keep the
old one around to provide backwards compat.

> >      ) -> Result<Value, Error> {
> >          let open_id = this.inner.lock().unwrap();
> > -        let claims = open_id.verify_authorization_code_simple(code, &private_auth_state)?;
> > +        let claims = open_id.verify_authorization_code_simple_userinfo(
>
> if we go the Option route, we could either select which proxmox-openid
> fn to call here, or unwrap the Option to false and just call the new
> one.

Unwrapping to false and calling the new one does make sense to me.

> the reason I'd prefer a backwards-compat implementation here is that
> without it, we need to have the following relations:
>
> pve-access-control: depends on new libpve-rs-perl
> libpve-rs-perl: breaks old pve-access-control
> libpve-rs-perl: dpends on new librust-proxmox-openid-dev
>
> whereas with a backwards-compat implementation in libpve-rs-perl we just
> need:
> pve-access-control depends on new libpve-rs-perl
> libpve-rs-perl: depends on new librust-proxmox-openid-dev
>
> which is much less entangled as all the version constraints go in the
> same direction.

Completely agree with not wanting to break backwards compat. If the
Option method is sufficient for this and we can just call the new
function with the unwrapped value, that works for me.

> > +            code,
> > +            &private_auth_state,
> > +            disable_userinfo,
> > +        )?;
> >
> >          Ok(to_value(&claims)?)
> >      }
> > --
> > 2.39.5
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
> >
>

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel