Re: [pve-devel] [PATCH v1 manager 4/5] change 'root@pam' checks with 'SuperUser' capability check

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> 'root@pam' has the privilege by default (since it's an SA), so we can
> drop the string comparisons all around and check that privilege instead
> when deciding to enable/disable buttons or views
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  www/manager6/Utils.js          | 3 ++-
>  www/manager6/dc/Config.js      | 2 +-
>  www/manager6/dc/UserView.js    | 2 +-
>  www/manager6/lxc/Options.js    | 2 +-
>  www/manager6/lxc/Resources.js  | 2 +-
>  www/manager6/node/Config.js    | 2 +-
>  www/manager6/window/Migrate.js | 4 ++--
>  7 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index aafe359a..31ab94e8 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1656,7 +1656,8 @@ Ext.define('PVE.Utils', {
>  
>      showCephInstallOrMask: function(container, msg, nodename, callback) {
>  	if (msg.match(/not (installed|initialized)/i)) {
> -	    if (Proxmox.UserName === 'root@pam') {
> +	    let caps = Ext.state.Manager.get('GuiCap');
> +	    if (caps.node.SuperUser) {

but if you change this here, you also need to change the backend - as 
this is currently root-only (the API path called by the Ceph install 
wizard requires Sys.Console which is not a given just because you have 
SuperUser, and the ceph_install handling itself requires root@pam - the 
user is then presented with a login shell). so either this remains 
root-only for now (like the upgrade thing - both have the same problem 
after all!), but then please add a comment why or mention that in the 
commit message - or you find a good safe solution, then please argue why 
it is safe ;)

>  		container.el.mask();
>  		if (!container.down('pveCephInstallWindow')) {
>  		    var isInstalled = !!msg.match(/not initialized/i);
> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
> index 9c54b19d..917c426f 100644
> --- a/www/manager6/dc/Config.js
> +++ b/www/manager6/dc/Config.js
> @@ -197,7 +197,7 @@ Ext.define('PVE.dc.Config', {
>  		});
>  	    }
>  
> -	    if (Proxmox.UserName === 'root@pam') {
> +	    if (caps.dc.SuperUser) {

the plugins actually require 'Sys.Modify', and the account endpoints are 
unqualified (root-only) or open to everyone (those returning 
schema/static stuff for front-end re-use) at the moment but that can 
probably be re-evaluated. in any case, switching this just in the 
front-end cannot work..

>  		me.items.push({
>  		    xtype: 'pveACMEClusterView',
>  		    title: 'ACME',
> diff --git a/www/manager6/dc/UserView.js b/www/manager6/dc/UserView.js
> index bbfc4f7c..fe0c0149 100644
> --- a/www/manager6/dc/UserView.js
> +++ b/www/manager6/dc/UserView.js
> @@ -29,7 +29,7 @@ Ext.define('PVE.dc.UserView', {
>  	    selModel: sm,
>  	    baseurl: '/access/users/',
>  	    dangerous: true,
> -	    enableFn: rec => caps.access['User.Modify'] && rec.data.userid !== 'root@pam',
> +	    enableFn: rec => caps.access['User.Modify'] && !caps.access.SuperUser,

no rationale given for the different way of accessing - I'll leave it to 
more JS affine reviewers to decide whether this is sensible or not, but 
please provide the reason WHY this doesn't use `caps.access['SuperUser']`

also, it's wrong - a SuperUser still requires User.Modify to modify 
users, so this either needs to stay as it is or simply drop the root@pam 
shortcut.

>  	    callback: () => reload(),
>  	});
>  	let run_editor = function() {
> diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
> index f2661dfc..f8eb8a5c 100644
> --- a/www/manager6/lxc/Options.js
> +++ b/www/manager6/lxc/Options.js
> @@ -136,7 +136,7 @@ Ext.define('PVE.lxc.Options', {
>  	    features: {
>  		header: gettext('Features'),
>  		defaultValue: Proxmox.Utils.noneText,
> -		editor: Proxmox.UserName === 'root@pam' || caps.vms['VM.Allocate']
> +		editor: caps.vms.SuperUser || caps.vms['VM.Allocate']
>  		    ? 'PVE.lxc.FeaturesEdit' : undefined,
>  	    },
>  	    hookscript: {
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 15ee3c67..2081b4a2 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -257,7 +257,7 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    var isUsedDisk = isDisk && !isUnusedDisk;
>  
>  	    var noedit = rec.data.delete || !rowdef.editor;
> -	    if (!noedit && Proxmox.UserName !== 'root@pam' && key.match(/^mp\d+$/)) {
> +	    if (!noedit && !caps.vms.SuperUser && key.match(/^mp\d+$/)) {
>  		var mp = PVE.Parser.parseLxcMountPoint(value);
>  		if (mp.type !== 'volume') {
>  		    noedit = true;
> diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
> index 68f80391..9f49f0dd 100644
> --- a/www/manager6/node/Config.js
> +++ b/www/manager6/node/Config.js
> @@ -236,7 +236,7 @@ Ext.define('PVE.node.Config', {
>  		    itemId: 'apt',
>  		    upgradeBtn: {
>  			xtype: 'pveConsoleButton',
> -			disabled: Proxmox.UserName !== 'root@pam',
> +			disabled: !caps.nodes.SuperUser,

we discussed this in depth and said we'll keep the upgrade console root 
only for now.. also, the backend isn't change to allow this for 
SuperUser so it's moot anyway?

>  			text: gettext('Upgrade'),
>  			consoleType: 'upgrade',
>  			nodename: nodename,
> diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
> index 1c23abb3..20fcf81d 100644
> --- a/www/manager6/window/Migrate.js
> +++ b/www/manager6/window/Migrate.js
> @@ -52,8 +52,8 @@ Ext.define('PVE.window.Migrate', {
>  		    }
>  	    },
>  	    setLocalResourceCheckboxHidden: function(get) {
> -		if (get('running') || !get('migration.hasLocalResources') ||
> -		    Proxmox.UserName !== 'root@pam') {
> +		let caps = Ext.state.Manager.get('GuiCap');
> +		if (get('running') || !get('migration.hasLocalResources') || caps.vms.SuperUser) {
>  		    return true;
>  		} else {
>  		    return false;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>