[pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows
Currently, after adding a storage to a pool, opening any edit window
will send a GET request with a superfluous `poolid` parameter and
cause a parameter verification error in the GUI. This breaks all edit
windows of the current session. A workaround is to reload the current
browser session.
This happens because the `PVE.pool.AddStorage` component inadvertently
adds `poolid` to an `extraRequestParams` object that is shared by all
instances of `Proxmox.window.Edit`, affecting all edit windows in the
current session. Fix this by instead creating a new object that is
local to the component.
Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
Signed-off-by: Friedrich Weber
---
Notes:
To check if we have this problem at other places, I did a quick search
for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
the object is created fresh already.
www/manager6/grid/PoolMembers.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 75f20cab..61e27dff 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
me.isAdd = true;
me.url = "/pools/";
me.method = 'PUT';
- me.extraRequestParams.poolid = me.pool;
+ me.extraRequestParams = { 'poolid': me.pool };
Ext.apply(me, {
subject: gettext('Storage'),
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows
On 13/03/2024 09:44, Friedrich Weber wrote:
> Currently, after adding a storage to a pool, opening any edit window
> will send a GET request with a superfluous `poolid` parameter and
> cause a parameter verification error in the GUI. This breaks all edit
> windows of the current session. A workaround is to reload the current
> browser session.
>
> This happens because the `PVE.pool.AddStorage` component inadvertently
> adds `poolid` to an `extraRequestParams` object that is shared by all
> instances of `Proxmox.window.Edit`, affecting all edit windows in the
> current session. Fix this by instead creating a new object that is
> local to the component.
>
> Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
> Signed-off-by: Friedrich Weber
> ---
>
> Notes:
> To check if we have this problem at other places, I did a quick search
> for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
> the object is created fresh already.
Small correction (as spotted by Aaron off-list): In `AddVM` in the same
file [1], we also have a line
22me.extraRequestParams.poolid = me.pool;
This one does not break any edit windows, because the
`extraRequestParams` object is created a few lines above that in the
`Ext.define`, so it is not shared between all `Proxmox.window.Edit`
instances:
9 extraRequestParams: {
10 'allow-move': 1,
11 },
... but if I understand correctly, it *is* shared between all `AddVM`
instances. Again, I think this shouldn't break anything because we reset
`poolid` every time `initComponent` is called, but the pattern is
somewhat error-prone. It might be better to change line 22 to:
22me.extraRequestParams = { 'allow-move': 1, 'poolid': me.pool };
... and remove lines 9-11, to always create a new object in
`initComponent`. What do you think?
[1]
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=385f48fb#l22
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH access-control] api: Prevent TFA from being set up for openid users
Currently it is possible to set up TFA for an OpenID user (as root user),
but it is never requested during the login process for that user.
This patch prevents this and displays an error message with the instruction
to set up TFA using the OpenId server.
Signed-off-by: Markus Frank
---
src/PVE/API2/TFA.pm | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index 13ffc59..5e7e9eb 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -381,6 +381,13 @@ __PACKAGE__->register_method ({
my ($userid, $realm) =
root_permission_check($rpcenv, $authuser, $param->{userid},
$param->{password});
+ my $domain_cfg = cfs_read_file('domains.cfg');
+ my $realm_cfg = $domain_cfg->{ids}->{$realm};
+ if ($realm_cfg->{type} eq "openid") {
+ die "Users of the realm '$realm' with type 'openid' cannot use TFA."
+ ." Using the OpenID server to set up TFA is recommended.\n";
+ }
+
my $type = delete $param->{type};
my $value = delete $param->{value};
if ($type eq 'yubico') {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH backup-qemu] make capi_types module public
Commit c7077bb3 moved ProxmoxBackupHandle into a separate file and
accidentally made it private in the process.
Revert this behaviour by making ProxmoxBackupHandle and
ProxmoxRestoreHandle accessible from outside of the crate again. These
handles are used in the function signatures of several public functions,
therefore it makes sense to have them public as well.
Signed-off-by: Filip Schauer
---
src/lib.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lib.rs b/src/lib.rs
index 02e74f7..9067fc5 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -11,7 +11,7 @@ use proxmox_lang::try_block;
use pbs_api_types::{Authid, BackupDir, BackupNamespace, BackupType, CryptMode};
use pbs_client::BackupRepository;
-mod capi_types;
+pub mod capi_types;
use capi_types::*;
mod commands;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion
The Ceph monitor removal assertion contains a condition that checks
whether the given mon ID actually exists and thus may be removed.
The first part of the condition checks whether the hash returned by
`get_services_info` [0] contains the key "mon.$monid". However, the
hash's keys are never prefixed with "mon.", which makes this check
incorrect.
This is fixed by just using "$monid" directly.
The second part checks whether the mon hashes returned by
Ceph contain the "name" key before comparing the key with the given
mon ID. This key existence check is also incorrect; in particular:
* If the lookup `$_->{name}` evaluates to e.g. "foo", the check
passes, because "foo" is truthy. [1]
* If the lookup `$_->{name}` evaluates to "0", the check fails,
because "0" is falsy (due to it being equivalent to the number 0,
according to Perl [1]).
This is solved by using the inbuilt `exists()` instead of relying on
Perl's definition of truthiness.
[0]:
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
[1]: https://perldoc.perl.org/perldata#Scalar-values
Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
Signed-off-by: Max Carrara
---
PVE/API2/Ceph/MON.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..1737c294 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
my $assert_mon_can_remove = sub {
my ($monhash, $monlist, $monid, $mondir) = @_;
-if (!(defined($monhash->{"mon.$monid"}) ||
- grep { $_->{name} && $_->{name} eq $monid } @$monlist))
+if (!(defined($monhash->{$monid}) ||
+ grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
{
die "no such monitor id '$monid'\n"
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager 2/2] ceph: mon: adapt code style in mon removal assertion
Signed-off-by: Max Carrara
---
PVE/API2/Ceph/MON.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1737c294..f13115a0 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -148,13 +148,13 @@ my $assert_mon_can_remove = sub {
my ($monhash, $monlist, $monid, $mondir) = @_;
if (!(defined($monhash->{$monid}) ||
- grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
+ grep { exists($_->{name}) && $_->{name} eq $monid } $monlist->@*))
{
die "no such monitor id '$monid'\n"
}
die "monitor filesystem '$mondir' does not exist on this node\n" if ! -d
$mondir;
-die "can't remove last monitor\n" if scalar(@$monlist) <= 1;
+die "can't remove last monitor\n" if scalar($monlist->@*) <= 1;
};
my $remove_addr_from_mon_host = sub {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/1] storage/plugins: pass scfg to parse_volname
On Tue, Mar 05, 2024 at 12:13:05PM +0100, Thomas Lamprecht wrote:
> Am 23/02/2024 um 10:24 schrieb Roland Kammerer:
> > This passes the well known $scfg to parse_volname and bumps the API
> > versions accordingly. This allows plugins to access their configuration
> > if necessary.
>
> We discussed this another time here and effectively it can be fine, while
> the need for it seems like a slight smell from our architecture POV, as
> it basically always means that the VMID -> volume mapping is not encoded
> in the name any more (worse UX), it still does not hurt our, or other
> external, existing plug-ins.
>
> So fine to add, but please also the parameter also to the base
> "parse_volname" method including a comment that mentions that this
> is in general not used and only required if the storage cannot
> provide all required information in the volume name.
My thoughts on this: (TLDR: we should just merge it and probably also
consider adding a separate method to get the *format* of a volid)
- Adding the parameter itself is fine, not thinking about how/why it is
used. Generally, it makes sense for all storage API methods to also
know the storage's config anyway.
- Most (if not all) invocations that actually need the owner vmid (which
is the part which becomes expensive here) AFAICT are already within a
more expensive context anyway.
- We have a *lot* of callers which actually only want the disk *format*,
which IMO means we could introduce a separate storage API call for
this (this can be backward compatible with a fallback to the parse
method if the plugin does not provide the new method)
./API2/LXC/Config.pm:199: $format =
(PVE::Storage::parse_volname($storage_cfg, $volid))[6];
^ update_vm call, attaching volumes - needs storage access anyway
./API2/LXC.pm:2345: my $format =
(PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/LXC.pm:2368: my $fmt =
(PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in 'move_volume', storage access already expected
./API2/Qemu.pm:155: my ($vtype) =
PVE::Storage::parse_volname($storecfg, $volid);
^ check_storage_access called from create_vm - creation is already needs
to access the storage anyway and can take different amounts of time
based on storages and is not a hot path anyway
./API2/Qemu.pm:441: my ($vtype) =
PVE::Storage::parse_volname($storecfg, $volid);
^ disk creation... also already expensive and not a hot path...
./API2/Qemu.pm:1640:$format = (PVE::Storage::parse_volname($storecfg,
$volid))[6];
^ update_vm call...
./API2/Qemu.pm:4143:my $format =
(PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/Qemu.pm:4166:my $fmt =
(PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in move_disk... duh :)
./API2/Qemu.pm-4866-my (undef, undef, undef, undef, undef, undef,
$format) =
PVE::Storage::parse_volname($storecfg,
$drive->{file});
^ resize_vm - also fine...
./CLI/pct.pm-247- my (undef, undef, undef, undef, undef, undef,
$format) =
PVE::Storage::parse_volname($storage_cfg,
$volid);
^ in 'pct fsck', fine
Vs the rest:
./API2/LXC.pm-1966- my (undef, undef, $owner, undef, undef, undef,
$format) =
PVE::Storage::parse_volname($storage_cfg,
$volid);
^ a check in a resize operation (not necessarily cheap anyway)
./API2/Qemu.pm:164: (my $vtype, undef, $src_vmid) =
PVE::Storage::parse_volname($storecfg, $src_image);
./API2/Qemu.pm:242:my $src_vmid = (PVE::Storage::parse_volname($storecfg,
$src_volid))[2];
^ both are part of the import-disk code - certainly not cheap to begin
with
./API2/VZDump.pm:295: my (undef, undef, $ownervm) =
PVE::Storage::parse_volname($storage_cfg, $volume);
./CLI/pvesm.pm:181: my (undef, undef, $ownervm) =
PVE::Storage::parse_volname($storage_cfg, $volume);
^ in both cases: permission check in extracting a config from a backup,
so only affects path based storages and isn't particularly cheap anyway
./CLI/pvesr.pm:41:my ($vtype, undef, $ownervm) =
PVE::Storage::parse_volname($storecfg, $volid);
^ replication...
./API2/Storage/FileRestore.pm:201: my (undef, $snap) =
PVE::Storage::parse_volname($cfg, $volid);
^ PBS specific...
./CLI/pve7to8.pm:895: ($vtype) = eval {
PVE::Storage::parse_volname($storage_cfg, $volid); };
^ happens only on major debian releases...
While within the storage implementations a lot of times the need for it
is storage dependent (eg. path based storages need it because the files
are in vmid-named subdirectories), and besides, it's usually within the
context of doing actual storage operations.
Also, the `volume_is_base_and_used()` call Dietmar mentioned only
happens when destroying or migrating VMs and so I'm not all that worried
about storage acce
Re: [pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion
On Wed Mar 13, 2024 at 3:53 PM CET, Max Carrara wrote:
> The Ceph monitor removal assertion contains a condition that checks
> whether the given mon ID actually exists and thus may be removed.
>
> The first part of the condition checks whether the hash returned by
> `get_services_info` [0] contains the key "mon.$monid". However, the
> hash's keys are never prefixed with "mon.", which makes this check
> incorrect.
>
> This is fixed by just using "$monid" directly.
>
> The second part checks whether the mon hashes returned by
> Ceph contain the "name" key before comparing the key with the given
> mon ID. This key existence check is also incorrect; in particular:
> * If the lookup `$_->{name}` evaluates to e.g. "foo", the check
> passes, because "foo" is truthy. [1]
> * If the lookup `$_->{name}` evaluates to "0", the check fails,
> because "0" is falsy (due to it being equivalent to the number 0,
> according to Perl [1]).
>
> This is solved by using the inbuilt `exists()` instead of relying on
> Perl's definition of truthiness.
>
> [0]:
> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
> [1]: https://perldoc.perl.org/perldata#Scalar-values
>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
> Signed-off-by: Max Carrara
> ---
> PVE/API2/Ceph/MON.pm | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..1737c294 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
> my $assert_mon_can_remove = sub {
> my ($monhash, $monlist, $monid, $mondir) = @_;
>
> -if (!(defined($monhash->{"mon.$monid"}) ||
> - grep { $_->{name} && $_->{name} eq $monid } @$monlist))
not sure if splitting the fix and the code style clean up makes sense
here but otherwise this works as advertised. So:
Tested-by: Stefan Sterz
> +if (!(defined($monhash->{$monid}) ||
> + grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
> {
> die "no such monitor id '$monid'\n"
> }
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
