449139 - config.cgi needs information about which groups are valid in which products

Status: ASSIGNED

(Bugzilla :: Administration, task)

Description

[Max Kanat-Alexander]

For people who are writing Bugzilla clients or automation tools for Bugzilla, config.cgi needs to include information about which groups are selectable for each product. We don't need to include information about mandatory groups, because clients don't need to know about those, I'm pretty sure.

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

How about this?

Gerv

Comment 2

[Max Kanat-Alexander]

Comment on attachment 405468 [details] [diff] [review]
Patch v.1

No, that doesn't tell you what each group is doing. Integrators will want to know whether groups are supposed to be Shown/Default/Mandatory for MemberControl and OtherControl. They may also want to know that certain products have editing and entry limitations to certain groups--however, we don't currently expose the names of those groups, so we should only display entry if user.can_enter_product and canedit if user.can_edit_product.

Comment 3

[Gervase Markham [:gerv]]

So basically, you want to dump the config information for each flag in each product, as well as having a reference to the data in the <groups> section at the bottom of the file?

Gerv

Comment 4

[Max Kanat-Alexander]

Right.

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.2

OK, how about this? My RDF-fu is not strong, and I didn't quite understand Max's comment about now showing certain groups at certain times, so this is probably wrong. Further guidance on how to get it right would be appreciated :-) But both this version and the previous one provide enough info for my usage, at least.

Gerv

Comment 7

[Frank Becker]

Attachment: patch, V3

(In reply to comment #5)
> Created an attachment (id=405838) [details]
> Patch v.2
> 
> OK, how about this? My RDF-fu is not strong, and I didn't quite understand
> Max's comment about now showing certain groups at certain times, so this is
> probably wrong. Further guidance on how to get it right would be appreciated
> :-) But both this version and the previous one provide enough info for my
> usage, at least.
> 
> Gerv

What do you think about this patch?

I include all the information from .../editproducts.cgi?action=editgroupcontrols... without the count of bugs.

Comment 8

[Frank Becker]

Attachment: patch, V4

Sorry,

I did not include the information for the special groups 
"chartgroup", "insidergroup", "timetrackinggroup", "querysharegroup" in patch V3.

For V4 I do include this.

Comment 9

[Gervase Markham [:gerv]]

Hi Frank,

Thanks for your patch. It may be a little while before I get a chance to look at it, though. I'm currently focussed on the JSON version.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Frank: Max is probably the right man to review this officially, but here are some initial comments.

I believe we consider the names of groups which a user is not in confidential (e.g. you don't want a user to know you have a group called TopSecretCustomer unless they are an employee of TopSecretCustomer). So putting unprotected group names in the config.cgi output, as you've done with the special groups, probably isn't good. Similarly, you should only provide info about groups the user can see. See my patch for how I did that.

Is there any reason you switched to doing the query for groups yourself, when my patch uses the built-in objects to supply the information?

Gerv

Comment 11

[Frank Becker]

Attachment: patch, V5

(In reply to comment #10)
> Frank: Max is probably the right man to review this officially, but here are
> some initial comments.
> 
> I believe we consider the names of groups which a user is not in confidential
> (e.g. you don't want a user to know you have a group called TopSecretCustomer
> unless they are an employee of TopSecretCustomer). So putting unprotected group
> names in the config.cgi output, as you've done with the special groups,
> probably isn't good. Similarly, you should only provide info about groups the
> user can see. See my patch for how I did that.
> 
> Is there any reason you switched to doing the query for groups yourself, when
> my patch uses the built-in objects to supply the information?

> 
> Gerv


Sorry my fault. There is no reason so I now use your patch as a base and only change a few points.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 412055 [details] [diff] [review]
patch, V5

>+$vars->{'insidergroup'}      = Bugzilla->params->{'insidergroup'};
>+$vars->{'chartgroup'}        = Bugzilla->params->{'chartgroup'};
>+$vars->{'timetrackinggroup'} = Bugzilla->params->{'timetrackinggroup'};
>+$vars->{'querysharegroup'}   = Bugzilla->params->{'querysharegroup'};

  You can use Param('some_parameter') in the templates to get params.

>Index: template/en/default/config.rdf.tmpl
>@@ -30,6 +31,8 @@
> <bz:installation rdf:about="[% urlbase FILTER xml %]">
>   <bz:install_version>[% constants.BUGZILLA_VERSION FILTER html %]</bz:install_version>
>   <bz:maintainer>[% Param('maintainer') FILTER html %]</bz:maintainer>
>+  <bz:user_login>[% user.login FILTER html %] </bz:user_login>
>+  <bz:user_id>[% user.id FILTER html %] </bz:user_id>

  Hmm, maybe you should have an [% IF user.id %] wrapped around those so that they only show up if you're logged in? user.login is kind of "undefined  behavior" if you're not logged in.

>@@ -114,6 +117,7 @@
> 
> [% END %]
> 
>+[% all_visible_groups = {} %]

  Hmm, should this be directly above the FOREACH instead?

>@@ -148,6 +152,35 @@
>+                [% FOREACH group = product.groups_valid %]

  Umm, I think that groups_valid can expose groups that the user isn't supposed to know about. I'm not entirely certain, though. It doesn't check user->id though, I can see that in its code.

>+                    <bz:group rdf:about="[% urlbase FILTER xml %]group.cgi?id=[% group.id FILTER url_quote 
>+                              %]&amp;product=[% product.name FILTER url_quote %]">

  Is that how other per-product about="" attributes look?

>+                      <bz:othercontrol>
>+                        [% "NA" IF group_controls.othercontrol == constants.CONTROLMAPNA %]

  I wonder, since this is an API, should we be using the numeric values instead?

>+    <Seq>
>+      [% FOREACH group = all_visible_groups.values.sort('name') %]

  The only problem with sorting there is that it will be case-sensitive.

>+  <bz:specialgroups>
>+      [% IF user.in_group(chartgroup) %]
>+      <bz:chartgroup>[% chartgroup FILTER html %]</bz:chartgroup>
>+      [% END %]
>+      [% IF user.in_group(insidergroup) %]

  You can use user.is_insider there.

>+      [% IF user.in_group(timetrackinggroup) %]

  You can use user.is_timetracker there.


  By the way, in the future these won't be parameters anymore--they'll just be groups that other groups can inherit from.

Comment 13

[Frank Becker]

Attachment: patch, V6

In reply to comment #12)
> (From update of attachment 412055 [details] [diff] [review])
> >+$vars->{'insidergroup'}      = Bugzilla->params->{'insidergroup'};
> >+$vars->{'chartgroup'}        = Bugzilla->params->{'chartgroup'};
> >+$vars->{'timetrackinggroup'} = Bugzilla->params->{'timetrackinggroup'};
> >+$vars->{'querysharegroup'}   = Bugzilla->params->{'querysharegroup'};
> 
>   You can use Param('some_parameter') in the templates to get params.

Done.

> 
> >Index: template/en/default/config.rdf.tmpl
> >@@ -30,6 +31,8 @@
> > <bz:installation rdf:about="[% urlbase FILTER xml %]">
> >   <bz:install_version>[% constants.BUGZILLA_VERSION FILTER html %]</bz:install_version>
> >   <bz:maintainer>[% Param('maintainer') FILTER html %]</bz:maintainer>
> >+  <bz:user_login>[% user.login FILTER html %] </bz:user_login>
> >+  <bz:user_id>[% user.id FILTER html %] </bz:user_id>
> 
>   Hmm, maybe you should have an [% IF user.id %] wrapped around those so that
> they only show up if you're logged in? user.login is kind of "undefined 
> behavior" if you're not logged in.

Done

> 
> >@@ -114,6 +117,7 @@
> > 
> > [% END %]
> > 
> >+[% all_visible_groups = {} %]
> 
>   Hmm, should this be directly above the FOREACH instead?

Now it is directly above the FOREACH
> 
> >@@ -148,6 +152,35 @@
> >+                [% FOREACH group = product.groups_valid %]
> 
>   Umm, I think that groups_valid can expose groups that the user isn't supposed
> to know about. I'm not entirely certain, though. It doesn't check user->id
> though, I can see that in its code.
> 
> >+                    <bz:group rdf:about="[% urlbase FILTER xml %]group.cgi?id=[% group.id FILTER url_quote 
> >+                              %]&amp;product=[% product.name FILTER url_quote %]">
> 
>   Is that how other per-product about="" attributes look?

This is changed to no longer use product.name
> 
> >+                      <bz:othercontrol>
> >+                        [% "NA" IF group_controls.othercontrol == constants.CONTROLMAPNA %]
> 
>   I wonder, since this is an API, should we be using the numeric values
> instead?

The numeric value is used now.

> 
> >+    <Seq>
> >+      [% FOREACH group = all_visible_groups.values.sort('name') %]
> 
>   The only problem with sorting there is that it will be case-sensitive.

Now I use sort by id
> 
> >+  <bz:specialgroups>
> >+      [% IF user.in_group(chartgroup) %]
> >+      <bz:chartgroup>[% chartgroup FILTER html %]</bz:chartgroup>
> >+      [% END %]
> >+      [% IF user.in_group(insidergroup) %]
> 
>   You can use user.is_insider there.

Done.

> 
> >+      [% IF user.in_group(timetrackinggroup) %]
> 
>   You can use user.is_timetracker there.
> 
Done.
> 
>   By the way, in the future these won't be parameters anymore--they'll just be
> groups that other groups can inherit from.

Because we do not have all the needed information now, we change this when you done this.

Comment 14

[Frank Becker]

(In reply to comment #13)
> Created attachment 438580 [details] [diff] [review]
> patch, V6

Can someone review this?

Comment 15

[Gervase Markham [:gerv]]

mkanat: polite ping?

Gerv

Comment 16

[Frank Becker]

Any news on this on?

Comment 17

[Max Kanat-Alexander]

Comment on attachment 438580 [details] [diff] [review]
patch, V6

Review of attachment 438580 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/config.rdf.tmpl
@@ +152,4 @@
>                  </Seq>
>                </bz:target_milestones>
>              [% END %]
> +            [% IF product.groups_valid.size > 0 %]

This is probably going to damage config.cgi performance pretty badly for installs that have a lot of groups, since calling groups_valid is fairly database-intensive.

What you'll want to do is modify Bugzilla::Product::preload to pre-load this information, but only when a specific flag is passed to preload. (You'll want to make the second argument to preload() into a hashref, so that we can pass many of these flags.)

@@ +153,5 @@
>                </bz:target_milestones>
>              [% END %]
> +            [% IF product.groups_valid.size > 0 %]
> +
> +              <bz:groups>

We're re-using bz:groups for product controls and the general group descriptions. Perhaps call this bz:product_groups or something?

@@ +160,5 @@
> +                    [% IF user.in_group_id(group.id)%]
> +                      [% all_visible_groups.${group.id} = group %]
> +                      [% group_controls = product.group_controls.${group.id} %]
> +                      <li>
> +                        <bz:group rdf:about="[% urlbase FILTER xml %]group.cgi?id=[% group.id FILTER url_quote%]">

Nit: These lines look super-long, way longer than 80 characters. Perhaps you should put some of this stuff into a BLOCK?

@@ +271,5 @@
> +  <bz:groups>
> +    <Seq>
> +      [% FOREACH group = user.groups.sort('id') %]
> +        <li>
> +          <bz:group rdf:about="[% urlbase FILTER xml %]group.cgi?id=[% group.id FILTER url_quote %]">

Same note here.

@@ +283,5 @@
> +      [% END %]
> +    </Seq>
> +  </bz:groups>
> +
> +[% IF Param('chartgroup') || Param('querysharegroup') || user.is_insider || user.is_timetracker %]

Just always display this information. If it's empty, it's empty.

@@ +285,5 @@
> +  </bz:groups>
> +
> +[% IF Param('chartgroup') || Param('querysharegroup') || user.is_insider || user.is_timetracker %]
> +  <bz:specialgroups>
> +    [% IF Param('chartgroup') %]

Which also means don't wrap any of these in IF statements.

Comment 18

[Frank Becker]

Attachment: patch V7

Sorry for the long delay until i continue with this!

Comment 19

[Frank Becker]

Attachment: patch V8

Sorry patch V7 was not the actual on.

Comment 20

[Frédéric Buclin]

Comment on attachment 613122 [details] [diff] [review]
patch V8

>=== modified file 'Bugzilla/Product.pm'

>     if ($preload_flagtypes) {
>         $_->flag_types foreach @$products;
>+        $_->groups_valid foreach @$products;
>     }

We don't want groups to be loaded every time we touch flags. They are unrelated. Please don't put this call here.



>=== modified file 'template/en/default/config.rdf.tmpl'

>+[% IF user.id %]
>+  <bz:user_login>[% user.login FILTER html %]</bz:user_login>
>+  <bz:user_id>[% user.id FILTER html %]</bz:user_id>
>+[% END %]

Why is this information useful?


>+                  <bz:group rdf:about="[% escaped_urlbase %]group.cgi?id=
>+                    [% group.id FILTER uri %]">
>+                    <bz:name>[% group_controls.name FILTER xml %]</bz:name>

This is not the right place to call rdf:about. This should be <li resource=""> as groups are already listed elsewhere. This is how we do it for e.g. components, milestones or versions.


>+                    <bz:editcomponents>
>+                      [% group_controls.editcomponents FILTER xml %]</bz:editcomponents>
>+                    <bz:editbugs>
>+                      [% group_controls.editbugs FILTER xml %]</bz:editbugs>
>+                    <bz:canconfirm>
>+                      [% group_controls.canconfirm FILTER xml %]</bz:canconfirm>

This information is not disclose when using the web UI. There is no reason to disclose this information here unless the user has editcomponents privileges.


>+      [% FOREACH group = user.groups.sort('id') %]

There is no need to call .sort(). user.groups is already sorted alphabetically.


>+  <bz:specialgroups>

I think special_groups would be a better name. Also, this information should only be accessible to users with tweakparams privileges. This is confidential information.