Re: [pbs-devel] [PATCH proxmox-backup v2 07/15] api: access: add routes for managing AD realms

[Christoph Heiss <c.heiss@proxmox.com>]

On Tue, Nov 28, 2023 at 09:23:51AM +0100, Fabian Grünbichler wrote:
> [..]
> On August 16, 2023 4:47 pm, Christoph Heiss wrote:
> > +#[api(
> > +    input: {
> > +        properties: {},
> > +    },
> > +    returns: {
> > +        description: "List of configured AD realms.",
> > +        type: Array,
> > +        items: { type: AdRealmConfig },
> > +    },
> > +    access: {
> > +        permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false),
>
> this one here
>
> [..]
> > +
> > +#[api(
> > +    input: {
> > +        properties: {
> > +            realm: {
> > +                schema: REALM_ID_SCHEMA,
> > +            },
> > +        },
> > +    },
> > +    returns: { type: AdRealmConfig },
> > +    access: {
> > +        permission: &Permission::Privilege(&["access", "domains"], PRIV_SYS_AUDIT, false),
>
> and this one here don't really agree - copied over from LDAP ;)

Well, the OpenID realm also uses PRIV_SYS_AUDIT for reading. So for the
sake of consistency, AD should use the same.

>
> also, maybe this one here should check on /access/domains/{realm}
> (although that might be postponed to do it in sync with the other
> endpoint(s), but it would be more in line with how we handle entity ACLs
> in general).

Good catch, seems sensible. For the existing LDAP/OpenID endpoints
changing it would probably constitute a breaking change?

Changing both things above for the existing LDAP/OpenID endpoints would
probably constitute a breaking change?


> [..]
> > +const ITEM_ROUTER: Router = Router::new()
> > +    .get(&API_METHOD_READ_AD_REALM)
> > +    .put(&API_METHOD_UPDATE_AD_REALM)
> > +    .delete(&super::ldap::API_METHOD_DELETE_LDAP_REALM);
>
> this seems a bit weird - as in - why doesn't that endpoint check that
> it's actually being passed an LDAP realm?

The LDAP endpoint for this is actually generic enough that it works for
AD too.
IOW, it just deletes the realm with that particular name from the config
(since names are unique across all realm types anyway) and the LDAP bind
password, if one exists. The infrastructure for the latter is also
reused by the AD realm.

So in favor of not unnecessarily duplicating code, I chose to simply
reuse it as-is.

At the end of the day, AD is just LDAP with some Microsoft
idiosyncrasies, thus I tried to reuse as much code/infrastructure as
possible.