[pve-devel] Pool View

[Andrew Thrift]

Exported Variable:

The particular variable referenced is the canonical list of Valid
Privileges, necessary for assessing whether any given Privilege is, in fact
on the list of valid and assignable Privileges. The change made allows the
variable to referenced outside the scope of the module that contains it; so
that it can be re-used in other modules (specifically in
PVE::API2::Cluster). I would like to highlight that it is NOT really a
Global Variable. It is a lexically scoped, exported variable, available to
other modules within the PVE package by reference to
PVE:AccessControl::valid_privs (eg, it does not collide with other
declarations of valid_privs in other modules). If a different approach is
preferred, please advise what the desired strategy is - if it is strictly
necessary, then for example a utility function could be added to
PVE::AccessControl to export the values contained in the valid_privs
variable instead. If the Proxmox has some specific design goals in mind the
patch can be altered to accommodate.


check_any call for valid_privs:

This check is assessing whether the user has any permissions assigned to
the pool and therefore, by extension, whether they should have visibility
of the Pool at all, or whether the Pool should be omitted from their view
of the list of Pools. In other words, if the user has no rights assigned to
the Pool, then their console/screen should not be cluttered with it in this
view (as they can not perform any actions on it). This check achieves that
goal.


!important tag on image url:

This was included in the original cut of the patch to cover a specific
browser incompatibility scenario; where the author observed that the image
was not showing in its desired situation. This is a common problem with
older versions of Internet Explorer and occasionally I have observed it in
Chrome as well (where a less-specific CSS selector overrides a
more-specific CSS selector).


On Mon, Jul 21, 2014 at 5:40 PM, Dietmar Maurer <dietmar at proxmox.com> wrote:

> here are a few comments and question (inline):
>
> > diff -Nuarp usr/share/perl5/PVE/AccessControl.pm
> > usr/share/perl5/PVE/AccessControl.pm
> > --- usr/share/perl5/PVE/AccessControl.pm      2014-06-27
> 16:23:34.305587119
> > +1200
> > +++ usr/share/perl5/PVE/AccessControl.pm      2014-06-27
> 16:23:44.077692909
> > +1200
> > @@ -492,7 +492,7 @@ my $privgroups = {
> >      },
> >  };
> >
> > -my $valid_privs = {};
> > +our $valid_privs = {};
>
> using global vars is bad design!
>
> >  my $special_roles = {
> >      'NoAccess' => {}, # no priviledges
> > diff -Nuarp usr/share/perl5/PVE/API2/Cluster.pm
> > usr/share/perl5/PVE/API2/Cluster.pm
> > --- usr/share/perl5/PVE/API2/Cluster.pm       2014-06-27
> 16:22:37.302970017
> > +1200
> > +++ usr/share/perl5/PVE/API2/Cluster.pm       2014-06-27
> 16:26:30.939493106
> > +1200
> > @@ -19,6 +19,8 @@ use PVE::RESTHandler;
> >  use PVE::RPCEnvironment;
> >  use PVE::JSONSchema qw(get_standard_option);
> >
> > +use PVE::AccessControl qw(valid_privs);
> > +
> >  use base qw(PVE::RESTHandler);
> >
> >  __PACKAGE__->register_method ({
> > @@ -160,12 +162,20 @@ __PACKAGE__->register_method({
> >       my $vmlist = PVE::Cluster::get_vmlist() || {};
> >       my $idlist = $vmlist->{ids} || {};
> >
> > +     my $storage_pool = {};
> > +
> > +     foreach my $pool (keys(%{$usercfg->{pools}})) {
> > +             foreach my $storage (keys(%{$usercfg->{pools}->{$pool}-
> > >{storage}})) {
> > +                     $storage_pool->{$storage}=$pool;
> > +             }
> > +     }
> > +
> >       my $pooldata = {};
> >       if (!$param->{type} || $param->{type} eq 'pool') {
> >           foreach my $pool (keys %{$usercfg->{pools}}) {
> >               my $d = $usercfg->{pools}->{$pool};
> >
> > -             next if !$rpcenv->check($authuser, "/pool/$pool", [
> > 'Pool.Allocate' ], 1);
> > +             next if !$rpcenv->check_any($authuser, "/pool/$pool", [keys
> > $PVE::AccessControl::valid_privs], 1);
>
> Please can you explain that change?
>
> >
> >               my $entry = {
> >                   id => "/pool/$pool",
> > @@ -185,9 +195,9 @@ __PACKAGE__->register_method({
> >
> >               my $data = $idlist->{$vmid};
> >               my $entry = PVE::API2Tools::extract_vm_stats($vmid, $data,
> > $rrd);
> > -             if ($entry->{uptime}) {
> > -                 if (my $pool = $usercfg->{vms}->{$vmid}) {
> > -                     if (my $pe = $pooldata->{$pool}) {
> > +             if (my $pool = $usercfg->{vms}->{$vmid}) {
> > +                 if (my $pe = $pooldata->{$pool}) {
> > +                     if ($entry->{uptime}) {
> >                           $pe->{uptime} = $entry->{uptime} if !$pe-
> > >{uptime} || $entry->{uptime} > $pe->{uptime};
> >                           $pe->{mem} = 0 if !$pe->{mem};
> >                           $pe->{mem} += $entry->{mem};
> > @@ -198,6 +208,7 @@ __PACKAGE__->register_method({
> >                           $pe->{maxcpu} = 0 if !$pe->{maxcpu};
> >                           $pe->{maxcpu} += $entry->{maxcpu};
> >                       }
> > +                     $entry->{pool} = $pe->{pool};
> >                   }
> >               }
> >
> > @@ -228,6 +239,9 @@ __PACKAGE__->register_method({
> >                   next if !PVE::Storage::storage_check_enabled($cfg,
> > $storeid, $node, 1);
> >
> >                   my $entry =
> > PVE::API2Tools::extract_storage_stats($storeid, $scfg, $node, $rrd);
> > +                 if (exists $storage_pool->{$entry->{storage}}) {
> > +                     $entry->{"pool"}=$storage_pool->{$entry-
> > >{storage}};
> > +                 }
> >                   push @$res, $entry;
> >               }
> >           }
> > diff -Nuarp usr/share/pve-manager/css/ext-pve.css usr/share/pve-
> > manager/css/ext-pve.css
> > --- usr/share/pve-manager/css/ext-pve.css     2014-06-27
> 16:33:46.291799762
> > +1200
> > +++ usr/share/pve-manager/css/ext-pve.css     2014-06-27
> 16:34:56.083492567
> > +1200
> > @@ -108,7 +108,7 @@
> >  .x-tree-node-pool,
> >  .x-grid-tree-pool-expanded .x-tree-node-pool
> >  {
> > -    background-image:url(../images/connect_established.png);
> > +    background-image:url(../images/connect_established.png) !important;
>
> why is this required?
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.proxmox.com/pipermail/pve-devel/attachments/20140804/240523dc/attachment.htm>