From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 699121FF13B for ; Wed, 11 Feb 2026 09:54:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7572114A76; Wed, 11 Feb 2026 09:55:31 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 11 Feb 2026 09:55:23 +0100 Message-Id: Subject: Re: [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints From: "Lukas Wagner" To: "Arthur Bied-Charreton" , X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-10-a.bied-charreton@proxmox.com> In-Reply-To: <20260204161354.458814-10-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770800038366 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.036 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: DZXUZFUM334SPBPBV3DITTRKWDEDFR4B X-Message-ID-Hash: DZXUZFUM334SPBPBV3DITTRKWDEDFR4B X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote: > Add auth-method, as well as optional > oauth2-{client-id,client-secret,tenant-id,refresh-token} parameters to > prepare for OAuth2 support. > > The auth-method parameter was previously implicit and inferred by > proxmox-notify based on the presence of a password. It is now made > explicit, however still kept optional and explicitly inferred in the > {update,create}_endpoint handlers to avoid breaking the API. > > Signed-off-by: Arthur Bied-Charreton > --- > PVE/API2/Cluster/Notifications.pm | 55 +++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notific= ations.pm > index 8b455227..a45a15b2 100644 > --- a/PVE/API2/Cluster/Notifications.pm > +++ b/PVE/API2/Cluster/Notifications.pm > @@ -941,6 +941,13 @@ my $smtp_properties =3D { > default =3D> 'tls', > optional =3D> 1, > }, > + 'auth-method' =3D> { > + description =3D> > + 'Determine which authentication method shall be used for the= connection.', > + type =3D> 'string', > + enum =3D> [qw(google-oauth2 microsoft-oauth2 plain none)], > + optional =3D> 1, > + }, > username =3D> { > description =3D> 'Username for SMTP authentication', > type =3D> 'string', > @@ -951,6 +958,26 @@ my $smtp_properties =3D { > type =3D> 'string', > optional =3D> 1, > }, > + 'oauth2-client-id' =3D> { > + description =3D> 'OAuth2 client ID', > + type =3D> 'string', > + optional =3D> 1, > + }, > + 'oauth2-client-secret' =3D> { > + description =3D> 'OAuth2 client secret', > + type =3D> 'string', > + optional =3D> 1, > + }, > + 'oauth2-tenant-id' =3D> { > + description =3D> 'OAuth2 tenant ID', > + type =3D> 'string', > + optional =3D> 1, > + }, > + 'oauth2-refresh-token' =3D> { > + description =3D> 'OAuth2 refresh token', > + type =3D> 'string', > + optional =3D> 1, > + }, > mailto =3D> { > type =3D> 'array', > items =3D> { > @@ -1108,6 +1135,11 @@ __PACKAGE__->register_method({ > my $mode =3D extract_param($param, 'mode'); > my $username =3D extract_param($param, 'username'); > my $password =3D extract_param($param, 'password'); > + my $auth_method =3D extract_param($param, 'auth-method'); > + my $oauth2_client_secret =3D extract_param($param, 'oauth2-clien= t-secret'); > + my $oauth2_client_id =3D extract_param($param, 'oauth2-client-id= '); > + my $oauth2_tenant_id =3D extract_param($param, 'oauth2-tenant-id= '); > + my $oauth2_refresh_token =3D extract_param($param, 'oauth2-refre= sh-token'); > my $mailto =3D extract_param($param, 'mailto'); > my $mailto_user =3D extract_param($param, 'mailto-user'); > my $from_address =3D extract_param($param, 'from-address'); > @@ -1115,6 +1147,10 @@ __PACKAGE__->register_method({ > my $comment =3D extract_param($param, 'comment'); > my $disable =3D extract_param($param, 'disable'); > =20 > + if (!defined $auth_method) { > + $auth_method =3D defined($password) ? 'plain' : 'none'; > + } > + > eval { > PVE::Notify::lock_config(sub { > my $config =3D PVE::Notify::read_config(); > @@ -1126,6 +1162,11 @@ __PACKAGE__->register_method({ > $mode, > $username, > $password, > + $auth_method, > + $oauth2_client_id, > + $oauth2_client_secret, > + $oauth2_tenant_id, > + $oauth2_refresh_token, > $mailto, > $mailto_user, > $from_address, > @@ -1187,6 +1228,11 @@ __PACKAGE__->register_method({ > my $mode =3D extract_param($param, 'mode'); > my $username =3D extract_param($param, 'username'); > my $password =3D extract_param($param, 'password'); > + my $auth_method =3D extract_param($param, 'auth-method'); > + my $oauth2_client_secret =3D extract_param($param, 'oauth2-clien= t-secret'); > + my $oauth2_client_id =3D extract_param($param, 'oauth2-client-id= '); > + my $oauth2_tenant_id =3D extract_param($param, 'oauth2-tenant-id= '); > + my $oauth2_refresh_token =3D extract_param($param, 'oauth2-refre= sh-token'); > my $mailto =3D extract_param($param, 'mailto'); > my $mailto_user =3D extract_param($param, 'mailto-user'); > my $from_address =3D extract_param($param, 'from-address'); > @@ -1197,6 +1243,10 @@ __PACKAGE__->register_method({ > my $delete =3D extract_param($param, 'delete'); > my $digest =3D extract_param($param, 'digest'); > =20 > + if (!defined $auth_method) { > + $auth_method =3D defined($password) ? 'plain' : 'none'; > + } > + > eval { > PVE::Notify::lock_config(sub { > my $config =3D PVE::Notify::read_config(); > @@ -1208,6 +1258,11 @@ __PACKAGE__->register_method({ > $mode, > $username, > $password, > + $auth_method, > + $oauth2_client_id, > + $oauth2_client_secret, > + $oauth2_tenant_id, > + $oauth2_refresh_token, > $mailto, > $mailto_user, > $from_address, As already explained off-list, I think it's time to switch from a flat list of parameters to passing hashes for the parameters for the `add_smtp_target` and `update_smtp_target` methods. This means, the bindings in proxmox-perl-rs would then directly take SmtpConfig/SmtpPrivateConfig and SmtpConfigUpdater/SmtpPrivateConfigUpdater. Then the call could look something like (not tested) $config->add_smtp_endpoint( $name, { server =3D> $server, port =3D> $port, ... }, { password =3D> $password, ... } ); This makes it much harder to introduce bugs due to parameter ordering. Long-term we should do the same for the other endpoints, but no need to do it in this series, I think. For changes like these and in general it's pretty important to mention any breaking changes in the cover letter and maybe patch description, since the changes done in this series affect multiple packages that *could* be updated independently by our users. For instance, in the cover-letter you could write something like: =20 The patch series requires the following version requirement bumps: pve-manager requires bumped proxmox-perl-rs proxmox-perl-rs requires bumped proxmox-notify* *.) although for this one it's not that critical, since its only a build-dependency, so there is no chance of customer systems breaking due to partial system updates This way the maintainer knows that the version requirements in debian/control must be adapted at some point after applying the patches.