Re: [pve-devel] [PATCH pve-manager 1/1] fix #7149: ui: prohibit realm update on login

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

stumbled upon this when writing the debian/changelog for a new package
version bump: subjet is rather misleading, this does not update anything
on login, a side-effect of this change is that the pre-selected realm saved
in the browser state, which is then also selected on login.
But after this patch, a realm update is still not prohibited for when a
user changes the realm on login, unlike your subject suggests, and that is
a good thing, as doing so would introduce a regression.

A better subject would have been:

"ui: user create: do not change pre-selected realm in browser state"

or:

"ui: realm selector: do no leak changes on user create to local users default"

or:

"ui: do not set realm selected on user create as default realm of logged-in user"

plus prefixing the "fix #..." is naturally fine in our current documented
style.

Just to give a few example of what I would prefer due to being much clearer
in what this commit does, there isn't one right style

On 18/12/2025 13:42, Dominik Rusovac wrote:
> The realm selection during user creation is no longer stateful. Hence,
Wording the commit message in this form makes it sound like the realm
selection was changed to not be stateful in a *previous* commit and that
caused some regression or the like that this commit fixes. Rather just use
the present tense, as a commit changes (from its own POV) something in its
present, past things is rather useful to describe the status quo before this
commit, or other commits that came before it in the commit log chain.

> as desired, adding a new user does not affect the realm on the login
> page.
Besides some potential language improvements I'd find something like:

"Make the authentication realm selector from the user creation dialogue
stateless. This avoids odd effects for the logged in user, as we use that
browser-local saved state for the pre-selected realm on a fresh login.
So if, e.g., a @pam user created and @pve user, on the next login that
@pam user suddenly had the @pve realm pre-selected."

more telling.

Noting his all in such detail because git commit messages are quite
important, and while they most often are really fine to be relatively
short, they definitively should not be confusing and suggest different
changes that what's in them. That can throw one of during checking the
git log, bisecting or also when writing changelogs and not looking
closely, and then that might leak from that even in the release nodes
and proliferate even to various other parts (nowadays some LLM might
even pick it up and invent some further BS on top of it...).

Anyhow, should have been caught on review, so no fault from your
side, but please try to keep this somewhat in mind for future
commits/patches.

> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>   www/manager6/dc/UserEdit.js | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/www/manager6/dc/UserEdit.js b/www/manager6/dc/UserEdit.js
> index 7541816b..84d9cfa8 100644
> --- a/www/manager6/dc/UserEdit.js
> +++ b/www/manager6/dc/UserEdit.js
> @@ -99,6 +99,7 @@ Ext.define('PVE.dc.UserEdit', {
>               column1.splice(1, 0, {
>                   xtype: 'pmxRealmComboBox',
>                   name: 'realm',
> +                stateful: false, // realm is not saved between page reloads
>                   fieldLabel: gettext('Realm'),
>                   allowBlank: false,
>                   matchFieldWidth: false,



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