Bug 458870

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 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

*** Bug 427913 has been marked as a duplicate of this bug. ***

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 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

Thanks guys, This seems to be working well.

This is live now. Closing.