Bug 458870 - Need to have updatePerms() ported to bz3.2
Need to have updatePerms() ported to bz3.2
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: WebService (Show other bugs)
3.2
All Linux
medium Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
:
: 427913 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-12 15:19 EDT by Toshio Kuratomi
Modified: 2014-08-18 09:16 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-25 17:28:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
v1 for webservice function updatePerms (6.17 KB, patch)
2008-08-15 00:15 EDT, Noura El hawary
dkl: review-
Details | Diff
v2 for webservice function bugzilla.updatePerms (8.09 KB, patch)
2008-08-19 03:37 EDT, Noura El hawary
dkl: review+
Details | Diff

  None (edit)
Description Toshio Kuratomi 2008-08-12 15:19:42 EDT
Description of problem:
To set Fedora contributors up with proper permissions in bugzilla (for reviewing packages, triaging bugs, etc) we need to create bugzilla users from time to time and add or remove them from the fedora_contrib group.  With the old BZ we used the xml interface updatePerms() but that was not ported in the initial BZ-3.2 migration.

We need either updatePerms() ported or something equivalent that lets us modify which groups a user belongs to.



Steps to Reproduce:
1. 
$ python

import xmlrpclib
server = xmlrpclib.Server('https://bugzilla.redhat.com/xmlrpc.cgi')
server.bugzilla.updatePerms(EMAIL, 'add', 'fedora_contrib', USER, PASSWD)

Actual results:

xmlrpclib.Fault: <Fault Client: 'Failed to locate method (updatePerms) in class (bugzilla) at /usr/lib/perl5/5.8.8/CGI/Carp.pm line 314.\n'>


Expected results:
method found and either lets me change permissions or throws an error because the user doesn't exist.

Additional info:
Comment 1 Noura El hawary 2008-08-15 00:15:37 EDT
Created attachment 314372 [details]
v1 for webservice function updatePerms

Attached is a patch for webservice fuction updatePerms , basically there is no base code yet in bugzilla 3.2 to perform this functionality , it is done in 3.2 in editusers.cgi using direct sql queries, so have done the same for the webservice function updatePerms, to call the function here is an example:

my $call = $rpc->call( 'bugzilla.updatePerms', $user, 'add' , ["canconfirm","fedora_contrib"], $username, $password );

patch is now on bz-web2 if you would like to test.

Please review.

Thanks,
Noura
Comment 2 David Lawrence 2008-08-15 16:01:35 EDT
Comment on attachment 314372 [details]
v1 for webservice function updatePerms

>+sub updatePerms {
>+    my ($self, $user, $cmd, $groups, $username, $password ) = @_;
>+
>+    $user || ThrowCodeError( 'param_required', { param => 'user' } );
>+    $cmd || ThrowCodeError( 'param_required', { param => 'command' } );
>+

We should check that $cmd is one of two choices (add|remove) and throw error if 
not stating what the choices are.

>+    # Try to login if $username and $password provided.
>+    xmlrpc_client_login($username, $password, LOGIN_REQUIRED);
>+
>+    $logger->debug("Starting: $user, $cmd, $groups, $username");
>+
>+    my $userid = Bugzilla->user->id;
>+    my $editusers = Bugzilla->user->in_group('editusers'); 
>+
>+    # Reject access if there is no sense in continuing.
>+    $editusers
>+        || Bugzilla->user->can_bless()
>+        || ThrowUserError("auth_failure", {group  => "editusers",
>+                                           reason => "cant_bless",
>+                                           action => "edit",
>+                                           object => "users"});
>+
>+
>+    # Return if user does not exist
>+    my $uid = Bugzilla::User::login_to_id($user);
>+    my $dbh = Bugzilla->dbh;
>+
>+    # Lock tables during the check+update session.
>+    $dbh->bz_start_transaction();
>+
>+    foreach my $group ( @{$groups} ) {
>+        my $sth = $dbh->prepare("SELECT id FROM groups WHERE name = " . $dbh->quote($group));
>+        $sth->execute();
>+        my $groupid = $sth->fetchrow;
>+        next if !$groupid;

We need to check that the current user has bless privileges for the current group being added/removed:

          next if !$editusers && !Bugzilla->user->can_bless($groupid);

>+
>+        if ( $cmd =~ 'add' ) {
>+            my $sth2 = 
>+                $dbh->prepare("SELECT group_id FROM user_group_map WHERE user_id = $uid "
>+                . "AND group_id = $groupid AND isbless = 0");
>+            $sth2->execute();
>+            my $result = $sth2->fetchrow;
>+            if ( !$result ) {
>+                $dbh->do("INSERT INTO user_group_map (user_id,group_id,isbless,grant_type) "
>+                . "VALUES ($uid,$groupid,0," . GRANT_DIRECT . ")");
>+            }
>+        }
>+        if ( $cmd =~ 'rem' ) {
>+            my $sth3 =
>+                $dbh->prepare("SELECT group_id FROM user_group_map WHERE user_id = $uid "
>+                . "AND group_id = $groupid AND isbless = 0 AND grant_type = " . GRANT_DIRECT);
>+            $sth3->execute();
>+            my $result = $sth3->fetchrow;
>+            if ($result) {
>+                $dbh->do("DELETE FROM user_group_map WHERE user_id = $uid "
>+                       . "AND group_id = $groupid AND isbless = 0 AND grant_type = " . GRANT_DIRECT);
>+            }
>+        }

We need to update the profiles_activity table with the changes.

From editusers.cgi (line 314):

    if (@groupsAddedTo || @groupsRemovedFrom) {
        $dbh->do(qq{INSERT INTO profiles_activity (
                           userid, who,
                           profiles_when, fieldid,
                           oldvalue, newvalue
                          ) VALUES (
                           ?, ?, now(), ?, ?, ?
                          )
                   },
                 undef,
                 ($otherUserID, $userid,
                  get_field_id('bug_group'),
                  join(', ', @groupsRemovedFrom), join(', ', @groupsAddedTo)));
    }


The rest looks good.
Dave
Comment 3 David Lawrence 2008-08-18 14:08:25 EDT
*** Bug 427913 has been marked as a duplicate of this bug. ***
Comment 4 Noura El hawary 2008-08-19 03:37:43 EDT
Created attachment 314515 [details]
v2 for webservice function bugzilla.updatePerms

Thanks for the review Dave, here is another version of the patch with your suggestions applied.

Noura
Comment 5 David Lawrence 2008-08-20 00:26:44 EDT
Comment on attachment 314515 [details]
v2 for webservice function bugzilla.updatePerms

>+    # Return if user does not exist
>+    my $uid = Bugzilla::User::login_to_id($user);

Need to throw error if username passed in is invalid:

      if (!$uid) {
          ThrowUserError('invalid_username', { name => $user });
      }

>+    foreach my $group ( @{$groups} ) {
>+        my $sth = $dbh->prepare("SELECT id FROM groups WHERE name = " . $dbh->quote($group));
>+        $sth->execute();
>+        my $groupid = $sth->fetchrow;
>+        next if ((!$groupid) || (!Bugzilla->user->can_bless($groupid)));

Nit-pick: Are all of the parens necessary? Could it be simplified to be more readable?

          next if (!$groupid || !Bugzilla->user->can_bless($groupid));

>+        if ( $cmd =~ 'add' ) {

Nit-pick: No need to do regexp on the add/remove keywords since we already check to make sure
they are either add or remove exactly early in the subroutine.

          if ( $cmd eq 'add) {

>+            my $sth2 =
>+                $dbh->prepare("SELECT group_id FROM user_group_map WHERE user_id = $uid "
>+                . "AND group_id = $groupid AND isbless = 0");
>+            $sth2->execute();
>+            my $result = $sth2->fetchrow;
>+            if ( !$result ) {
>+                $dbh->do("INSERT INTO user_group_map (user_id,group_id,isbless,grant_type) "
>+                . "VALUES ($uid,$groupid,0," . GRANT_DIRECT . ")");
>+                push(@groupsAddedTo, $group);
>+            }
>+        }
>+        if ( $cmd =~ 'rem' ) {
>+            my $sth3 =
>+                $dbh->prepare("SELECT group_id FROM user_group_map WHERE user_id = $uid "
>+                . "AND group_id = $groupid AND isbless = 0 AND grant_type = " . GRANT_DIRECT);
>+            $sth3->execute();
>+            my $result = $sth3->fetchrow;
>+            if ($result) {
>+                $dbh->do("DELETE FROM user_group_map WHERE user_id = $uid "
>+                       . "AND group_id = $groupid AND isbless = 0 AND grant_type = " . GRANT_DIRECT);
>+                push(@groupsRemovedFrom, $group);
>+            }
>+        }
>+    }


For future optimization (don't need to do right now) we could possibly more the SQL outside of the loop and use placeholders so that the statements can be cached in case we have alot of add/remove operations in a single call.

For example:

my $select_gid_sth = $dbh->prepare("SELECT id FROM groups WHERE name = ?");

my $select_group_sth = $dbh->prepare(
    "SELECT group_id FROM user_group_map WHERE user_id = ? " .
    "AND group_id = ? AND isbless = 0 AND grant_type = " . GRANT_DIRECT);

my $add_group_sth = $dbh->prepare(
    "INSERT INTO user_group_map (user_id,group_id,isbless,grant_type) " .
    "VALUES (?, ?, 0, " . GRANT_DIRECT . ");

my $remove_group_sth = $dbh->prepare(
    "DELETE FROM user_group_map WHERE user_id = ? " . 
    "AND group_id = ? AND isbless = 0 AND grant_type = " . GRANT_DIRECT);

foreach my $group ( @{$groups} ) {       
    $select_gid_sth->execute($group);
    my $groupid = $select_gid_sth->fetchrow;
    next if (!$groupid || !Bugzilla->user->can_bless($groupid));

    if ($cmd eq 'add') {
        $select_group_sth->execute($uid, $groupid);
        my $result = $select_group_sth->fetchrow;
        if (!$result) {
            $add_group_sth->execute($uid, $groupid);
            push(@groupsAddedTo, $group);
        }
    }  
    if ($cmd eq 'rem') {
        $select_group_sth->execute($uid, $groupid);
        my $result = $select_group_sth->fetchrow;
        if ($result) {
            $remove_group_sth->execute($uid, $groupid);
            push(@groupsRemovedFrom, $group);
        }
    }
}

But we can do this later if it would hold up getting this out to the users.

Go ahead and check in when you are ready.

Dave
Comment 6 Toshio Kuratomi 2008-08-25 17:03:00 EDT
Thanks guys, This seems to be working well.
Comment 7 David Lawrence 2008-08-25 17:28:45 EDT
This is live now. Closing.

Note You need to log in before you can comment on or make changes to this bug.