236678 - Clean up access to COOKIE global

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Christian Reis]

We should clean up access to COOKIE global. Callsites involved:

  attachment.cgi 
  buglist.cgi
  post_bug.cgi
  enter_bug.cgi
  process_bug.cgi
  move.pl
  query.cgi
  show_bug.cgi
  userlist.cgi

Note that 

  $::COOKIE{'Bugzilla_login'};

is supposed to become either:

  Bugzilla->user->login

or

  [% user.login %]

Comment 1

[Christian Reis]

My patch for bug 226754 kills COOKIE{'Bugilla_logincookie'}.

Comment 2

[Dave Miller [:justdave]]

OK, after all the dependencies are resolved, you can as a patch to this bug
remove the backward compatibility stuff from

CGI.pl:
417:    $::COOKIE{$name} = $::cgi->cookie($name);

Bugzilla.pm:
61:    # $::COOKIE{'Bugzilla_login'} from a userid to a loginname
84:        $::COOKIE{'Bugzilla_login'} = $_user->login;
128:    delete $::COOKIE{"Bugzilla_login"};

Remember that nothing should be using $cgi->cookie("Bugzilla_login") outside of
the Auth stuff.  Everyone else should be using Bugzilla->user->login.

If you want, the COOKIE stuff is few and far enough between that it would
probably work as a single patch to this bug to wipe them all out at once.  If
you do that, remove %COOKIE from the summary on the rest of the dependent bugs,
and move their dependencies back the main %FORM bug once this one is ready to go in.

Comment 3

[Dave Miller [:justdave]]

The only file with %COOKIE in it that's not mentioned on one of the dependent
bugs is Bugzilla/Bug.pm.

394:      && (defined $::COOKIE{"Bugzilla_login"}) 
395:        && ($::COOKIE{"Bugzilla_login"} =~ /$movers/);

It'd be a waste to file a separate bug for it if you wind up rolling it all into
one patch anyway. :)

Comment 4

[Christian Reis]

The final cleanup should also apply to Bugzilla.pm and globals.pl, which have
the backwards compatibility code that provides COOKIE in the first place.

Comment 5

[Christian Reis]

I've started the COOKIE revolution separately so we get at least some traction here.

Comment 6

[Christian Reis]

Dave, it seems that the code in Bugzilla::Bug->user isn't being used anywhere.
Or is it?

Comment 7

[Christian Reis]

Hmm, yes it is: in knob.html.tmpl. So that means I have to actually understand
it :-(

Comment 8

[Christian Reis]

Attachment: kiko_v1: COOKIE KILLER

This eliminates the last traces of cookie off the map. It got a bit grotesque
because there was a weird logic error in Bug->user that required doing some
groovy changes there and I decided to de-$::userid-ify it while I was at it (in
part because the old code was hard to read). I think it's much nicer of course.

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 153960 [details] [diff] [review]
kiko_v1: COOKIE KILLER

>+    my $canedit = (!Bugzilla->user)
>+                  || $isreporter
>+                  || &::UserInGroup("editbugs")

ewww...  I see :: still  ;)

Try Bugzilla->user->in_group("editbugs")

>-    # NB - Can't delete from $cgi->cookie, so the logincookie data will
>-    # remain there; it's only used in Bugzilla::Auth::CGI->logout anyway
>-    # People shouldn't rely on the cookie param for the username
>-    # - use Bugzilla->user instead!
>+    # NB - Won't delete from $cgi->cookie, so the logincookie data will
>+    # and should remain there. Don't rely on it: use Bugzilla->user
>+    # instead!

Can't or won't?  If it's possible, why don't we?

Comment 10

[Christian Reis]

Attachment: kiko_v2: COOKIE KILLER 2.0

Comment 11

[Christian Reis]

Attachment: kiko_v2: COOKIE KILLER 2.0 (the real one)

Comment 12

[Joel Peshkin]

Comment on attachment 154053 [details] [diff] [review]
kiko_v2: COOKIE KILLER 2.0 (the real one)

I think the logic is wrong for cancinfirm,  If a user is both the reporter and
the assignee, they used to be able to confirm.	Now they would no longer be
able to.

Comment 13

[Christian Reis]

You're right, but only because the code is now understandable <wink>. I'll have
it fixed in the afternoon.

Comment 14

[Christian Reis]

Attachment: kiko_v3: okidoki

Comment 15

[Joel Peshkin]

Comment on attachment 154325 [details] [diff] [review]
kiko_v3: okidoki

r=joel
Interestingly, a user without canconfirm could assign a bug to himself, confirm
it, and then reassign it. If they figure that out, though, they deserve
confirm.

Comment 16

[Christian Reis]

Well, if you wanted to revoke that right, you should never have brought up the
issue in the first place <wink>. Thanks.

Comment 17

[Christian Reis]

Checked in:

/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.40; previous revision: 1.39
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.14; previous revision: 1.13
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.212; previous revision: 1.211
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/README,v  <--  README
new revision: 1.2; previous revision: 1.1

I should have done this long ago. As it stands, thanks!

Comment 18

[Christian Reis]

Oops. Some bits were still left in Auth/Login/WWW.pm -- just murdered them too.