1014345 - Add Group.get RPC call

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Simon Green]

We have Group.create and Group.update, but not a Group.get function. Luckily brc having one lying around (author unknown)

Comment 1

[Simon Green]

Attachment: bug1014345-v1.patch

This actually differs a bit from the Red Hat version.

Comment 2

[Simon Green]

Attachment: bug1014345-v1.patch

One whitespace character removed :)

Comment 3

[:glob ✱]

Comment on attachment 8426818 [details] [diff] [review]
bug1014345-v1.patch

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

::: Bugzilla/WebService/Group.pm
@@ +132,5 @@
> +
> +    # If the user doesn't have creategroup privilege, make sure they have
> +    # bless privileges on the groups
> +    if (! $creategroups) {
> +    }

this code should either:
1. do something
2. be removed

@@ +181,5 @@
> +        if ($visibleGroups) {
> +            $query .= qq{, user_group_map AS ugm
> +                         WHERE ugm.user_id = profiles.userid
> +                           AND ugm.isbless = 0
> +                           AND ugm.group_id IN ($visibleGroups)

use $dbh->sql_in()

@@ +195,5 @@
> +        $nextCondition = 'AND';
> +    }
> +    if (!$visibleGroups) {
> +        ThrowUserError("auth_failure", {reason  => "not_visible",
> +                                        action  => "access"

you need to pass in an 'object' to the auth_failure error.

this incorrectly throws an error in the following scenario:
- user is not in creategroups
- user has bless access to a single group
- Groups.get is called with membership => 1, but without providing an id and/or name

actual:
"Sorry, there are visibility restrictions on certain user groups, and so you are not authorized to access ."

expected:
returned membership for all groups which the user can bless


also note the error message incorrectly states that "there are visibility restrictions", even if none are in place.

@@ +200,5 @@
> +                      });
> +    }
> +
> +    my $grouplist = join(',',
> +        @{Bugzilla::Group->flatten_group_membership($group->id)});

nit: add a space after @{ and before the closing }

@@ +201,5 @@
> +    }
> +
> +    my $grouplist = join(',',
> +        @{Bugzilla::Group->flatten_group_membership($group->id)});
> +    $query .= " $nextCondition ugm.group_id IN($grouplist) ";

use $dbh->sql_in() here too.

i don't see how it's possible for $nextCondition to be anything other than AND

@@ +475,5 @@
> +
> +If the user is not a member of the "creategroups" group, but they are in the
> +"editusers" group or have bless privileges to the groups they require
> +membership information for, the is_active, is_bug_group and user_regexp values
> +are not supplied.

i'd like to see it clarified that you have to be in the creategroups group _unless_ you're requesting membership information.

Comment 4

[Simon Green]

Attachment: bug1014345-v3.patch

Comment 5

[:glob ✱]

Comment on attachment 8429625 [details] [diff] [review]
bug1014345-v3.patch

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

calling as an unblessed user (no access to edit any groups or user), i get different results if i ask for group membership or not.

setting membership => 0, i get "Sorry, you aren't a member of the 'creategroups' group, and so you are not authorized to access groups."
setting membership => 1, i don't get an error, instead an empty group array is returned.

the behaviour should be identical in both cases.


otherwise this looks great :)

::: Bugzilla/WebService/Group.pm
@@ +209,5 @@
> +            email_enabled     => $self->type('boolean', $_->email_enabled),
> +            login_denied_text => $self->type('string', $_->disabledtext),
> +        }} @$user_objects;
> +
> +        return \@users;

nit: weird indentation

@@ +466,5 @@
> +that you have bless privileges for will be returned.
> +
> +=over
> +
> +=item C<ids> (array) - An array of integers representing group ids.

these params are formatted different from the other two methods in this file.

Comment 6

[Simon Green]

Attachment: bug1014345-v4.patch

Comment 7

[:glob ✱]

Comment on attachment 8435408 [details] [diff] [review]
bug1014345-v4.patch

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

the issue with different error messages from the last review hasn't been fixed.
requiring login is great, however i'm not calling the method unauthenticated - it's an authenticated call made by an unblessed user.

Comment 8

[Simon Green]

Attachment: bug1014345-v5.patch

Comment 9

[:glob ✱]

Comment on attachment 8444183 [details] [diff] [review]
bug1014345-v5.patch

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

r=glob, with fix-on-commit for the error code.

::: Bugzilla/WebService/Constants.pm
@@ +141,4 @@
>      auth_invalid_email           => 302,
>      extern_id_conflict           => -303,
>      auth_failure                 => 304,
> +    group_cannot_view            => 304,

as this is different from an authentication failure (it's a permissions issue), it shouldn't return the same code as auth_failure.

the code should be in the 800-900 range

Comment 10

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   2db0429..2cb56fb  master -> master

Comment 11

[Frédéric Buclin]

Comment on attachment 8444183 [details] [diff] [review]
bug1014345-v5.patch

>=== modified file 'Bugzilla/WebService/Group.pm'

>+sub get {

>+    my $all_groups = $user->in_group('edituser') || $user->in_group('creategroups');
>+    if (!$all_groups && ! scalar(@{$user->bless_groups})) {

There is no 'edituser' group. It's 'editusers'. Also, it's much cleaner to call $user->can_bless instead of scalar(@{$user->bless_groups}).

Comment 12

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #11) 
> >+    my $all_groups = $user->in_group('edituser') || $user->in_group('creategroups');
> >+    if (!$all_groups && ! scalar(@{$user->bless_groups})) {
> 
> There is no 'edituser' group. It's 'editusers'. Also, it's much cleaner to
> call $user->can_bless instead of scalar(@{$user->bless_groups}).

To get this fix into 4.5.5 I am trying to push out today, I went ahead and committed the changes needed.

To ssh://[e-mail]/bugzilla/bugzilla.git
   a61e957..cf3e8bc  master -> master

dkl

Comment 13

[Frédéric Buclin]

Added to relnotes for 5.0rc1.