201018 - editusers.cgi never calls DeriveGroup prior to changing a bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Dave Miller [:justdave]]

This means if you edit a user's groups, and they have privileges which are
inherited from a group you changed, those privilege changes aren't taken into
account.

I fixed this on Zippy's install by calling DeriveGroup from editusers after
changing the user's groups.  Patch coming

Comment 1

[Dave Miller [:justdave]]

Attachment: Patch v1

Comment 2

[Bradley Baetz (:bbaetz)]

This shouldn't be needed. Bugzilla::login calls |&::ConfirmGroup($userid);| in
CVS, and this was in the old login code too.

Comment 3

[Dave Miller [:justdave]]

How does it know it needs to update though?

changing a user's groups doesn't change the timestamps on the groups...

the only way to make ConfirmGroups work in that case is to set the
refreshed_when on the user to Some Date In The Far Past...

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 119673 [details] [diff] [review]
Patch v1

Thats not good enough, because you could be changing the login_name too, which
affects regexps.

Also, given the lack of transaction safety, to be really safe you should start
off by setting refreshed_when to <some date in the past>, then rederive at the
end (just do it unconditionally, I think, althogh you can have a flag for
email+grop changes only, I guess). That protects against server failure in the
middle.

Comment 5

[Dave Miller [:justdave]]

Attachment: Patch v2

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 119674 [details] [diff] [review]
Patch v2

OK, this works.

Do we need a checksetup fix for this? We should add one, unconditionally for
now, but then conditional on the next schema change when we have one.

Comment 7

[Dave Miller [:justdave]]

probably wouldn't hurt.

UPDATE profiles SET refreshed_when='1900-01-01 00:00:00'

would probably do the trick.

Comment 8

[Bradley Baetz (:bbaetz)]

Can we get this in now, or is this going to wait for the release? It doesn't
affect 2.16. Theres a schema patch (bug 180086) which shoudl go in in the next
few days, and we can put it in with that.

If you want this to wait, though, then we'll have issues with people who upgrade
to that before the release, and we'll have to make it unconditional until the
change after that.

Comment 9

[Bradley Baetz (:bbaetz)]

I'm waiting on bug 180086 for a reply to a mail I sent to developers@ about
checma change locations. In teh meantime, we can put this in to always run the
refersh, although that schema change also depends on the answer to my message...

http://bugzilla.org/cgi-bin/mj_wwwusr?func=archive-index-date&list=developers&extra=200304

is the post.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: adds schema change too

Just the schema change; no other changes from the previous patch.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 121585 [details] [diff] [review]
adds schema change too

r=myk; works for me too

Comment 12

[Bradley Baetz (:bbaetz)]

Fixed for 2.17.4:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.229; previous revision: 1.228
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.43; previous revision: 1.42
done

Comment 13

[Dave Miller [:justdave]]

Security Advisory has been posted, removing security group