[pbs-devel] [PATCH proxmox-backup 1/3] fix #3939: add default-realm field to node config

[pbs-devel] [PATCH proxmox-backup 1/3] fix #3939: add default-realm field to node config

[Matthias Heiserer]

This is probably not ideal, but I like it better than storing the
default value in the domains config, especially as pam and pbs
auth are hardcoded in the domains endpoint.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 pbs-api-types/src/lib.rs |  2 +-
 src/api2/node/config.rs  |  4 ++++
 src/config/node.rs       | 12 ++++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 421566f7..e1544d76 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -59,7 +59,7 @@ pub use userid::Userid;
 pub use userid::{Realm, RealmRef};
 pub use userid::{Tokenname, TokennameRef};
 pub use userid::{Username, UsernameRef};
-pub use userid::{PROXMOX_GROUP_ID_SCHEMA, PROXMOX_TOKEN_ID_SCHEMA, PROXMOX_TOKEN_NAME_SCHEMA};
+pub use userid::{PROXMOX_GROUP_ID_SCHEMA, PROXMOX_TOKEN_ID_SCHEMA, PROXMOX_TOKEN_NAME_SCHEMA, PROXMOX_AUTH_REALM_SCHEMA};
 
 #[macro_use]
 mod user;
diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 3b267adc..068d54ca 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -66,6 +66,8 @@ pub enum DeletableProperty {
     default_lang,
     /// Delete any description
     description,
+    /// Delete the default-realm property.
+    default_realm,
 }
 
 #[api(
@@ -127,6 +129,7 @@ pub fn update_node_config(
                 DeletableProperty::ciphers_tls_1_2 => { config.ciphers_tls_1_2 = None; },
                 DeletableProperty::default_lang => { config.default_lang = None; },
                 DeletableProperty::description => { config.description = None; },
+                DeletableProperty::default_realm => { config.default_realm = None; },
             }
         }
     }
@@ -143,6 +146,7 @@ pub fn update_node_config(
     if update.ciphers_tls_1_2.is_some() { config.ciphers_tls_1_2 = update.ciphers_tls_1_2; }
     if update.default_lang.is_some() { config.default_lang = update.default_lang; }
     if update.description.is_some() { config.description = update.description; }
+    if update.default_realm.is_some() { config.default_realm = update.default_realm; }
 
     crate::config::node::save_config(&config)?;
 
diff --git a/src/config/node.rs b/src/config/node.rs
index ac6774e3..c6f2e50d 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -10,7 +10,7 @@ use proxmox_http::ProxyConfig;
 
 use pbs_api_types::{
     EMAIL_SCHEMA, MULTI_LINE_COMMENT_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA,
-    OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
+    OPENSSL_CIPHERS_TLS_1_3_SCHEMA, PROXMOX_AUTH_REALM_SCHEMA
 };
 
 use pbs_buildcfg::configdir;
@@ -175,7 +175,11 @@ pub enum Translation {
         "description" : {
             optional: true,
             schema: MULTI_LINE_COMMENT_SCHEMA,
-        }
+        },
+        "default-realm": {
+            schema: PROXMOX_AUTH_REALM_SCHEMA,
+            optional: true,
+        },
     },
 )]
 #[derive(Deserialize, Serialize, Updater)]
@@ -222,6 +226,10 @@ pub struct NodeConfig {
     /// Node description
     #[serde(skip_serializing_if = "Option::is_none")]
     pub description: Option<String>,
+
+    /// Default realm for authenticating
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub default_realm: Option<String>,
 }
 
 impl NodeConfig {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/3] fix #3939: set default value in domains endpoint

[Matthias Heiserer]

Because the default realm is stored in node.cfg, here we have to add
it to the returned information.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 src/api2/access/domain.rs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/api2/access/domain.rs b/src/api2/access/domain.rs
index 3518e5ca..1dbc8a0e 100644
--- a/src/api2/access/domain.rs
+++ b/src/api2/access/domain.rs
@@ -5,7 +5,7 @@ use serde_json::{json, Value};
 
 use proxmox_router::{Router, RpcEnvironment, Permission};
 use proxmox_schema::api;
-
+use crate::config::node;
 use pbs_api_types::BasicRealmInfo;
 
 #[api(
@@ -24,17 +24,19 @@ use pbs_api_types::BasicRealmInfo;
 /// Authentication domain/realm index.
 fn list_domains(mut rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<BasicRealmInfo>, Error> {
     let mut list = Vec::new();
+    let default_realm = node::config()?.0.default_realm;
 
     list.push(serde_json::from_value(json!({
         "realm": "pam",
         "type": "pam",
         "comment": "Linux PAM standard authentication",
-        "default": Some(true),
+        "default": default_realm.as_ref().map(|value|value == "pam"),
     }))?);
     list.push(serde_json::from_value(json!({
         "realm": "pbs",
         "type": "pbs",
         "comment": "Proxmox Backup authentication server",
+        "default": default_realm.as_ref().map(|value|value == "pbs"),
     }))?);
 
     let (config, digest) = pbs_config::domains::config()?;
@@ -42,6 +44,11 @@ fn list_domains(mut rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<BasicRealmInf
     for (_, (section_type, v)) in config.sections.iter() {
         let mut entry = v.clone();
         entry["type"] = Value::from(section_type.clone());
+        entry["default"] = Value::from(
+            default_realm
+                .as_ref()
+                .map_or(false, |value| value == &entry["realm"]),
+        );
         list.push(serde_json::from_value(entry)?);
     }
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 2/3] fix #3939: set default value in domains endpoint

[Thomas Lamprecht]

On 23.03.22 14:01, Matthias Heiserer wrote:
> Because the default realm is stored in node.cfg, here we have to add
> it to the returned information.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  src/api2/access/domain.rs | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 

> @@ -24,17 +24,19 @@ use pbs_api_types::BasicRealmInfo;
>  /// Authentication domain/realm index.
>  fn list_domains(mut rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<BasicRealmInfo>, Error> {
>      let mut list = Vec::new();
> +    let default_realm = node::config()?.0.default_realm;

feels not ideal to pull in the node config here, if we really need to add a config
(see my bugzilla reply asking if the requester would be OK with just making the
realm box stateful), then I'd actually add it in the domain config as priority
flag (allow 0 to 100, default 50) which is then used for sorting the realms and
also for auto-selecting the highest priority + name sorted realm.

Same level of complexity in terms of what we need to safe (one new struct member),
but avoids the need to always load+parse an extra config and also more features we
can use it for.

Please note also that we'd like to have feature parity for the stuff that exists
in PVE or PMG too.

Re: [pbs-devel] [PATCH proxmox-backup 2/3] fix #3939: set default value in domains endpoint

[Matthias Heiserer]

On 11.04.2022 10:12, Thomas Lamprecht wrote:
> On 23.03.22 14:01, Matthias Heiserer wrote:
>> Because the default realm is stored in node.cfg, here we have to add
>> it to the returned information.
>>
>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>> ---
>>   src/api2/access/domain.rs | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
> 
>> @@ -24,17 +24,19 @@ use pbs_api_types::BasicRealmInfo;
>>   /// Authentication domain/realm index.
>>   fn list_domains(mut rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<BasicRealmInfo>, Error> {
>>       let mut list = Vec::new();
>> +    let default_realm = node::config()?.0.default_realm;
> 
> feels not ideal to pull in the node config here, if we really need to add a config
> (see my bugzilla reply asking if the requester would be OK with just making the
> realm box stateful), then I'd actually add it in the domain config as priority
> flag (allow 0 to 100, default 50) which is then used for sorting the realms and
> also for auto-selecting the highest priority + name sorted realm.
If I'm not mistaken the realm selector on the login screen is already 
stateful.
Sorting sounds interesting, but how would we go about the default 
pam/pbs auth? They are not in the domains config, so i guess we have to 
add them there.
> 
> Same level of complexity in terms of what we need to safe (one new struct member),
> but avoids the need to always load+parse an extra config and also more features we
> can use it for.
> 
> Please note also that we'd like to have feature parity for the stuff that exists
> in PVE or PMG too.
Not sure I understand the point on feature parity. You mean that the 
default realm should be set in the realms view?

Re: [pbs-devel] [PATCH proxmox-backup 2/3] fix #3939: set default value in domains endpoint

[Thomas Lamprecht]

On 11.04.22 12:20, Matthias Heiserer wrote:
> On 11.04.2022 10:12, Thomas Lamprecht wrote:
>> On 23.03.22 14:01, Matthias Heiserer wrote:
>>> Because the default realm is stored in node.cfg, here we have to add
>>> it to the returned information.
>>>
>>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>>> ---
>>>   src/api2/access/domain.rs | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>
>>> @@ -24,17 +24,19 @@ use pbs_api_types::BasicRealmInfo;
>>>   /// Authentication domain/realm index.
>>>   fn list_domains(mut rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<BasicRealmInfo>, Error> {
>>>       let mut list = Vec::new();
>>> +    let default_realm = node::config()?.0.default_realm;
>>
>> feels not ideal to pull in the node config here, if we really need to add a config
>> (see my bugzilla reply asking if the requester would be OK with just making the
>> realm box stateful), then I'd actually add it in the domain config as priority
>> flag (allow 0 to 100, default 50) which is then used for sorting the realms and
>> also for auto-selecting the highest priority + name sorted realm.
> If I'm not mistaken the realm selector on the login screen is already stateful.
> Sorting sounds interesting, but how would we go about the default pam/pbs auth? They are not in the domains config, so i guess we have to add them there.

Yes, if the priority would be set the built-in realms would show up in
the configuration too.

>>
>> Same level of complexity in terms of what we need to safe (one new struct member),
>> but avoids the need to always load+parse an extra config and also more features we
>> can use it for.
>>
>> Please note also that we'd like to have feature parity for the stuff that exists
>> in PVE or PMG too.
> Not sure I understand the point on feature parity. You mean that the default realm should be set in the realms view?

That whatever we add as feature for PBS, it should also be added to PVE/PMG, at least
if sensible; doesn't have to be in the same series, but it should be planned.

[pbs-devel] [PATCH proxmox-backup 3/3] fix #3939: ui: default realm setting in NodeOptionView

[Matthias Heiserer]

Again, different than we do it in PVE, because default-realm
is stored in node.cfg.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/config/NodeOptionView.js | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/www/config/NodeOptionView.js b/www/config/NodeOptionView.js
index bb12513b..f3652a53 100644
--- a/www/config/NodeOptionView.js
+++ b/www/config/NodeOptionView.js
@@ -53,8 +53,31 @@ Ext.define('PBS.NodeOptionView', {
 	},
     ],
 
+    add_pmxRealmComboBox_row: function(name, text, opts) {
+	let me = this;
+	opts = opts || {};
+	me.rows = me.rows || {};
+	me.rows[name] = {
+	    required: true,
+	    header: text,
+	    defaultValue: opts.defaultValue,
+	    renderer: opts.renderer,
+	    editor: {
+		xtype: 'proxmoxWindowEdit',
+		subject: text,
+		items: {
+		    xtype: 'pmxRealmComboBox',
+		    name: name,
+		},
+	    },
+	};
+    },
+
     initComponent: function() {
 	let me = this;
+	me.add_pmxRealmComboBox_row('default-realm', gettext('Default realm'), {
+	    defaultValue: 'pam',
+	});
 
 	me.callParent();
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 3/3] fix #3939: ui: default realm setting in NodeOptionView

[Thomas Lamprecht]

On 23.03.22 14:01, Matthias Heiserer wrote:
> Again, different than we do it in PVE, because default-realm
> is stored in node.cfg.
> 

While this patch may be obsolete in the current form with my bz comment and
review on the backend, still some notes.

> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  www/config/NodeOptionView.js | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/www/config/NodeOptionView.js b/www/config/NodeOptionView.js
> index bb12513b..f3652a53 100644
> --- a/www/config/NodeOptionView.js
> +++ b/www/config/NodeOptionView.js
> @@ -53,8 +53,31 @@ Ext.define('PBS.NodeOptionView', {
>  	},
>      ],
>  
> +    add_pmxRealmComboBox_row: function(name, text, opts) {

I'd figure that camels and snakes don't mix well, at least in casing style ;-)

But in general: this is unnecessary, as currently used you can just add another
static entry in the `gridRows` array, and if the user of this whole component
needs to be able to config something (which they ain't with your approach, but
it feels like that was the aim?) a simple (c)bind can avoid such intermediate
creation helper.

> +	let me = this;
> +	opts = opts || {};
> +	me.rows = me.rows || {};
> +	me.rows[name] = {
> +	    required: true,
> +	    header: text,
> +	    defaultValue: opts.defaultValue,
> +	    renderer: opts.renderer,
> +	    editor: {
> +		xtype: 'proxmoxWindowEdit',
> +		subject: text,
> +		items: {
> +		    xtype: 'pmxRealmComboBox',
> +		    name: name,
> +		},
> +	    },
> +	};
> +    },
> +
>      initComponent: function() {
>  	let me = this;
> +	me.add_pmxRealmComboBox_row('default-realm', gettext('Default realm'), {
> +	    defaultValue: 'pam',
> +	});
>  
>  	me.callParent();
>  

Re: [pbs-devel] [PATCH proxmox-backup 3/3] fix #3939: ui: default realm setting in NodeOptionView

[Matthias Heiserer]

On 11.04.2022 10:13, Thomas Lamprecht wrote:
> On 23.03.22 14:01, Matthias Heiserer wrote:
>> Again, different than we do it in PVE, because default-realm
>> is stored in node.cfg.
>>
> 
> While this patch may be obsolete in the current form with my bz comment and
> review on the backend, still some notes.
> 
>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>> ---
>>   www/config/NodeOptionView.js | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/www/config/NodeOptionView.js b/www/config/NodeOptionView.js
>> index bb12513b..f3652a53 100644
>> --- a/www/config/NodeOptionView.js
>> +++ b/www/config/NodeOptionView.js
>> @@ -53,8 +53,31 @@ Ext.define('PBS.NodeOptionView', {
>>   	},
>>       ],
>>   
>> +    add_pmxRealmComboBox_row: function(name, text, opts) {
> 
> I'd figure that camels and snakes don't mix well, at least in casing style ;-)
> 
> But in general: this is unnecessary, as currently used you can just add another
> static entry in the `gridRows` array, and if the user of this whole component
> needs to be able to config something (which they ain't with your approach, but
> it feels like that was the aim?) a simple (c)bind can avoid such intermediate
> creation helper.
I don't think I can drop this function, because for each row entry the 
ObjectGrid searches for a function named `add_${rowdef.xtype}_row` and 
throws an error if none can be found. That's also the reason for the 
strange name.
Maybe I misunderstand something?
> 
>> +	let me = this;
>> +	opts = opts || {};
>> +	me.rows = me.rows || {};
>> +	me.rows[name] = {
>> +	    required: true,
>> +	    header: text,
>> +	    defaultValue: opts.defaultValue,
>> +	    renderer: opts.renderer,
>> +	    editor: {
>> +		xtype: 'proxmoxWindowEdit',
>> +		subject: text,
>> +		items: {
>> +		    xtype: 'pmxRealmComboBox',
>> +		    name: name,
>> +		},
>> +	    },
>> +	};
>> +    },
>> +
>>       initComponent: function() {
>>   	let me = this;
>> +	me.add_pmxRealmComboBox_row('default-realm', gettext('Default realm'), {
This call is unnecessary,  I should put it in gridRows.
>> +	    defaultValue: 'pam',
>> +	});
>>   
>>   	me.callParent();
>>   
> 

Re: [pbs-devel] [PATCH proxmox-backup 3/3] fix #3939: ui: default realm setting in NodeOptionView

[Thomas Lamprecht]

On 11.04.22 12:03, Matthias Heiserer wrote:
>>>
>>> diff --git a/www/config/NodeOptionView.js b/www/config/NodeOptionView.js
>>> index bb12513b..f3652a53 100644
>>> --- a/www/config/NodeOptionView.js
>>> +++ b/www/config/NodeOptionView.js
>>> @@ -53,8 +53,31 @@ Ext.define('PBS.NodeOptionView', {
>>>       },
>>>       ],
>>>   +    add_pmxRealmComboBox_row: function(name, text, opts) {
>>
>> I'd figure that camels and snakes don't mix well, at least in casing style 😉
>>
>> But in general: this is unnecessary, as currently used you can just add another
>> static entry in the `gridRows` array, and if the user of this whole component
>> needs to be able to config something (which they ain't with your approach, but
>> it feels like that was the aim?) a simple (c)bind can avoid such intermediate
>> creation helper.
> I don't think I can drop this function, because for each row entry the ObjectGrid searches for a function named `add_${rowdef.xtype}_row` and throws an error if none can be found. That's also the reason for the strange name.
> Maybe I misunderstand something?

Sorry, I meant adding it statically to the `rows` array.

An yeah forgot the "nice" details of ObjectGrid's gridRow, it really should be
cleaned up sometimes..