On Mon Feb 9, 2026 at 9:34 AM CET, Arthur Bied-Charreton wrote:
> On Fri, Feb 06, 2026 at 04:00:42PM +0100, Lukas Wagner wrote:
>> Hi Arthur, thanks for the patch!
>>
>> some notes inline.
>>
>> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
>> > [...]
>> > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
>> > index bc63e19d..52493ef7 100644
>> > --- a/proxmox-notify/Cargo.toml
>> > +++ b/proxmox-notify/Cargo.toml
>> > @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
>> > lettre = { workspace = true, optional = true }
>> > tracing.workspace = true
>> > mail-parser = { workspace = true, optional = true }
>> > +oauth2 = { version = "5.0.0" }
>>
>> This misses a 'default-features = false' here, otherwise it will still
>> pull in reqwest.
>>
>> Also, the oauth2 and ureq deps should be optional, since they are only
>> required by the smtp endpoint, which is gated behind a (default)
>> feature.
>>
>> As an example, see how the `lettre` crate is handled, that one is also
>> only needed for the smtp backend.
>>
>> In general, make sure that something like
>> `cargo build --no-default-features --features smtp`
>> works (also cross-check with other features).
>>
>> > [...]
>> > pub mod endpoints;
>> > pub mod renderer;
>> > pub mod schema;
>> > +pub mod xoauth2;
>>
>> This module is only needed for the 'smtp' feature, so this should be
>> gated with a
>>
>> #[cfg(feature = 'smtp')]
>>
>> you might need to adapt some of the optional deps in Cargo.toml, e.g.
>> due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
>>
> Will fix the feature/dependencies issues in v2, thanks for the examples!
>
>> >
>> > #[derive(Debug)]
>> > pub enum Error {
>> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
>> > new file mode 100644
>> > index 00000000..66faabfa
>> > --- /dev/null
>> > +++ b/proxmox-notify/src/xoauth2.rs
>> > @@ -0,0 +1,146 @@
>> > +use oauth2::{
>> > + basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret,
>> > RefreshToken, TokenResponse,
>> > + TokenUrl,
>> > +};
>> > +
>> > +use crate::Error;
>> > +
>> > +/// `oauth2` dropped support for the `ureq` backend, this newtype
>> > circumvents this
>> > +/// by implementing the `SyncHttpClient` trait. This allows us to avoid
>> > pulling in
>> > +/// a different backend like `reqwest`.
>>
>> Maybe clarify here that Debian patched out the ureq backend due to a
>> version mismatch of the packaged ureq crate and the one pulled in by the
>> oauth crate. Once this is resolved we might drop this custom client
>> implementation again. oauth2 uses an old version of ureq; we might be
>> able to contribute a PR to update it to a newer version.
>>
>> You mentioned it in the commit message, but this would be useful here as
>> well.
>>
> I already opened a PR in oauth2 [0], no answer yet. Will add more
> details & a link to it to the doc comment.
>
> [0] https://github.com/ramosbugs/oauth2-rs/pull/338
>
quoting from your PR:
'when trying to use the ureq backend for oauth2, I unfortunately had to
find out that while it is documented as a feature in the crate docs, it
is no longer available. This seems to be due to the API changes
introduced by ureq 3.x.'
This might be a bit confusing for the maintainer, you should probably
explain that you are using the Debian-packaged version of oauth2 which
patched out the support for the ureq backend due to the version
conflict.
>>
>> In general, please add doc-strings for 'pub' functions. Also - I have
>> not checked the other patches, yet, but also consider using 'pub(crate)'
>> if these types are only needed inside this crate (which I think might be
>> the case for these).
>>
> Will do, thanks :)
>> > +
>> > +/// Microsoft Identity Platform refresh tokens replace themselves
>> > +/// upon every use, however the old one does *not* get revoked.
>> > +///
>> > +/// This means that every access token request yields both an access
>> > +/// token AND a new refresh token, which should replace the old one.
>> > +/// The old one should then be discarded.
>>
>> This actually raises a very interesting question. Consider the case of a
>> read-only cluster filesystem due to a missing quorum:
>>
>> - state file is read
>> - one or more smtp endpoints refresh their tokens while sending or due
>> to the periodic refresh
>> - state file is written, but fails due to pmxcfs being R/O
>>
>> I guess we would need to find some way here to not 'lose' the token.
>>
>> Some ideas:
>> - check *before* refreshing if we can actually write -> but this is
>> still racy
>> - when the write fails, save the token in some other location that is
>> not in the pmxcfs and then later, when pmxcfs is writable again,
>> update the statefile there - this could also be weird, in case
>> *multiple* nodes tried to send notifications at roughly the same
>> time - which one is the "correct" token then? Hopefully only one of
>> these nodes should be able to refresh the token in the first place,
>> but still something to keep in mind I think.
>>
>> Maybe there is a better way though, these were just some initial ideas...
>>
> I may have formulated this doc comment too conservatively: according to the
> Microsoft docs on token lifetime [0], while the refresh token does get
> "replaced" at every use, the replaced token is not revoked, and each
> token has a static lifetime of 90 days.
>
> This means that in this case, I don't think we really *care* which token is
> the correct one, since they are all still valid. We can afford to lose
> some and persist the first new token we get after the file system
> becomes writable again.
>
> (All of this assuming that the Microsoft docs are correct, and the
> implementation does not bear any surprises, could not test it myself yet.)
>
> If the FS stays read-only for 90+ days, we need user interaction again to
> get a new refresh token anyway, since even the freshest of tokens would be
> expired by then. Now that I think about it, we probably need a way to
> request a new authorization from the user in the frontend.
Ah, that's good to know - so that is hopefully not be an issue in the
real world (but maybe still warrant a note in the documentation). Also
let's hope that this does not become an issue with any other email
provider we might support in the future.
>
> [0]
> https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
>
>> > +///
>> > +///
>> > https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
>> > +pub fn get_microsoft_token(
>> > + client_id: ClientId,
>> > + client_secret: ClientSecret,
>> > + tenant_id: &str,
>> > + refresh_token: RefreshToken,
>> > +) -> Result<TokenExchangeResult, Error> {
>> > + let client = BasicClient::new(client_id)
>> > + .set_client_secret(client_secret)
>> > + .set_auth_uri(
>> > +
>> > AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
>> > + .map_err(|e| Error::Generic(format!("invalid auth URL:
>> > {e}")))?,
>> > + )
>> > + .set_token_uri(
>> > + TokenUrl::new(format!(
>> > +
>> > "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token";
>> > + ))
>> > + .map_err(|e| Error::Generic(format!("invalid token URL:
>> > {e}")))?,
>> > + );
>> > +
>> > + let token_result = client
>> > + .exchange_refresh_token(&refresh_token)
>> > + .request(&UreqSyncHttpClient::default())
>> > + .map_err(|e| Error::Generic(format!("could not get access token:
>> > {e}")))?;
>>
>> apologies for the annoying error handling in proxmox-notify, I've been
>> meaning to make it more convenient and rethink the error variants a bit,
>> but I simply have not gotten to it yet :)
>>
>
> I may not have helped by using Error::Generic everywhere ^^'
> Maybe we could add an AuthenticationError variant, and pull in thiserror
> to streamline the error handdling a little? Might be something for a
> follow-up series though.
>
Definitely something for a future patch series. While this will
*technically* be a breaking change for this crate, none of the existing
callers do anything with the returned errors except logging it anyway.
I have some other ideas and changes planned for proxmox-notify anyway
(like pulling out the static context, as explained my integration
testing proposal on pdm-devel [0]), so the error refactoring might
be something that I would include there.
[0]
https://lore.proxmox.com/all/[email protected]/T/#u
>> > +
>> > + Ok(TokenExchangeResult {
>> > + access_token: token_result.access_token().clone(),
>> > + refresh_token: token_result.refresh_token().cloned(),
>> > + })
>> > +}
