Re: [pbs-devel] [PATCH many v4 00/43] integrate notification system

From: Lukas Wagner <l.wagner@proxmox.com>
To: "Proxmox Backup Server development discussion"
	<pbs-devel@lists.proxmox.com>,
	"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Subject: Re: [pbs-devel] [PATCH many v4 00/43] integrate notification system
Date: Tue, 23 Apr 2024 13:13:17 +0200	[thread overview]
Message-ID: <8a7ac9d4-fbf3-4b94-9f7c-42ddc3fd5574@proxmox.com> (raw)
In-Reply-To: <1713795522.9okq9bghe3.astroid@yuna.none>

On  2024-04-22 16:24, Fabian Grünbichler wrote:
>> Rough edges:
>>   - Datastore option view in UI could be improved. When 'notification-mode' is
>>     set to 'notification-system', we should indicate that 'mailto-user' and the
>>     other notification settings have no effect. Already did that in the edit
>>     window, but in the grid panel I did not find a quick way to do that.
> 
> in general this series looks mostly good to me, some rough edges I
> noticed while testing:
> 
> - if the user lacks Sys.Audit, the notifications part is empty, even
>   though there are built-in targets/matchers, this might cause
>   complications for one-off tasks that allow selecting notification
>   things, or cause confusion on the user's side

One-of tasks/jobs only allow selecting the notification mode, so whether
to use the old 'email to some user' style or the new notification system,
but not any details. So I think it should be fine? Unless I'm missing something.

It would of course be nice to fail a bit more gracefully when privs are missing,
e.g. hide the notifications panel completely if .Audit is missing, or disable
the 'modify' buttons if .Modify is missing... but I think this should be done later,
this potentially requires coordination with PVE.

> - the datastore/job related checks when configuring notifications
>   currently don't allow access to datastores with just Datastore.Backup,
>   and not .Audit, even though such datastores/jobs are fully accessible
>   by the user..

Fixed - the API handler which returns the list of known metadata values now
uses the /admin/datastore API instead of /config/datastore.
The first one requires Datastore.Backup/Datastore.Audit, while the latter
only returns datastores for which the users has at least Datastore.Audit.

> - docs/config/notifications/format.rst has partly invalid content

Fixed - copy/paste mistake :)

> - Sys.Modify is only contained in the Admin role atm, which has a lot of
>   other privs - maybe we want to have our own NotificationsAdmin? not
>   sure whether that makes sense.. maybe it would be better to think
>   about this for a bit and maybe add a priv if we want to make this
>   configurable by "medium privileged" users, since Sys.Modify is quite
>   the heavy hammer anyway..

Good point. I mean on PVE we use Mapping.* and therefore can use
the PVEMapping* roles to to administer notifications, which is 
more of a 'medium sized hammer' ;) . On PBS I did not
find anything more fitting than Sys.*, so maybe it would make sense to
introduce something new, either only a new role or also new privs.

Not sure if we have to do this now or can do this at a later point?
Any way, I'm open for suggestions. :)

-- 
- Lukas

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