284263 - Inherited bless permissions without inherited membership do not work

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Marc Schumann [:Wurblzap]]

(In reply to bug 119485, comment #36)

This addresses the following:

> >+    $vars->{'restrictablegroups'} = groupsUserMayBless($user, 'id', 'name');
> 
>   That duplicates Bugzilla->user->bless_groups, I'm pretty sure, since I
> checked that code in.
[...]
> >+# Give a list of IDs of groups the user may bless.
> >+sub groupsUserMayBless {
> 
>   As I mentioned above, you can do most of this with
> Bugzilla->user->bless_groups. You could have also done it with
> UserCanBlessGroups() or whatever it was called before in globals.pl.

Comment 1

[Marc Schumann [:Wurblzap]]

It turns out groupsUserMayBless is doing its indirect bless permission check
incorrectly, which makes a move to bless_groups even more interesting.
Requesting blocking2.20.

Comment 2

[Max Kanat-Alexander]

Maybe 2.20.1 -- I *think* the only thing this will cause will be cosmetic errors
on the editusers.cgi page when you are looking at what groups a user inherits
for blessing, in a few cases.

However, if we can get the patch r+'ed before we release 2.20, I'd be all for
getting it in on the 2.20 branch. :-)

Comment 3

[Marc Schumann [:Wurblzap]]

Attachment: Patch

(In reply to comment #2)
> Maybe 2.20.1 -- I *think* the only thing this will cause will be cosmetic

Unfortunately not...

o Let user UA be a member of group GA
o Let group GA inherit bless membership for group GB
o User UA should be allowed to bless GB, but isn't :(

Comment 4

[Max Kanat-Alexander]

Yeah, in light of the fact that this actually breaks bless inheritance, it
should block. :-(

Comment 5

[Marc Schumann [:Wurblzap]]

When reviewing the patch, please note especially that groupsUserMayBless
contained a derive_groups call while bless_groups does not. This works for me.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Unrotted after checkin of bug 284264.

Comment 7

[Joel Peshkin]

Comment on attachment 190387 [details] [diff] [review]
Patch

code reads OK.	I just need to test it.  Has anyone else already tested it?

Comment 8

[Marc Schumann [:Wurblzap]]

No, probably just I.

Comment 9

[Joel Peshkin]

Comment on attachment 190387 [details] [diff] [review]
Patch

r=joel
I'd like either a 2xr or someone to do some more testing before it lands.

Comment 10

[Frédéric Buclin]

Comment on attachment 190387 [details] [diff] [review]
Patch

works well. r=LpSolit

Comment 11

[Frédéric Buclin]

oops. The patch doesn't apply cleanly on 2.20.

Comment 12

[Marc Schumann [:Wurblzap]]

Attachment: 2.20 backport

(In reply to comment #11)
> oops. The patch doesn't apply cleanly on 2.20.

Yes, that's because bug 284264 didn't go onto the branch. The backport
therefore contains parts of it.

Comment 13

[Max Kanat-Alexander]

This should get approval re-requested when the branch patch is ready.

Comment 14

[Frédéric Buclin]

Comment on attachment 192064 [details] [diff] [review]
2.20 backport

tested, works correctly.

Comment 15

[Frédéric Buclin]

requesting approval again

Comment 16

[Frédéric Buclin]

tip:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.96; previous revision: 1.95
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.73; previous revision: 1.72
done


2.20rc2:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.90.2.3; previous revision: 1.90.2.2
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61.2.5; previous revision: 1.61.2.4
done