Clean up access to COOKIE global

RESOLVED FIXED in Bugzilla 2.20

Status

()

RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: kiko, Assigned: kiko)

Tracking

2.17.7
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

15 years ago
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 %]
(Assignee)

Updated

15 years ago
Blocks: 225818
(Assignee)

Comment 1

15 years ago
My patch for bug 226754 kills COOKIE{'Bugilla_logincookie'}.
Depends on: 226754
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.
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. :)
(Assignee)

Comment 4

15 years ago
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.
(Assignee)

Updated

14 years ago
Depends on: 251933, 251935
No longer depends on: 238875, 238876
(Assignee)

Updated

14 years ago
Depends on: 251937
(Assignee)

Updated

14 years ago
No longer depends on: 238873
(Assignee)

Comment 5

14 years ago
I've started the COOKIE revolution separately so we get at least some traction here.
(Assignee)

Updated

14 years ago
Depends on: 252378
(Assignee)

Updated

14 years ago
No longer depends on: 238870
(Assignee)

Updated

14 years ago
Depends on: 252379
(Assignee)

Updated

14 years ago
No longer depends on: 238866
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

14 years ago
Created attachment 153960 [details] [diff] [review]
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.
Assignee: nobody → kiko
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #153960 - Flags: review?(bugreport)
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?
(Assignee)

Comment 10

14 years ago
Created attachment 154052 [details] [diff] [review]
kiko_v2: COOKIE KILLER 2.0
Attachment #153960 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154052 - Flags: review?(bugreport)
(Assignee)

Updated

14 years ago
Attachment #153960 - Flags: review?(bugreport)
(Assignee)

Updated

14 years ago
Attachment #154052 - Attachment is obsolete: true
Attachment #154052 - Flags: review?(bugreport)
(Assignee)

Comment 11

14 years ago
Created attachment 154053 [details] [diff] [review]
kiko_v2: COOKIE KILLER 2.0 (the real one)
(Assignee)

Updated

14 years ago
Attachment #154053 - Flags: review?(bugreport)

Comment 12

14 years ago
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.
Attachment #154053 - Flags: review?(bugreport) → review-
(Assignee)

Comment 13

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

Comment 14

14 years ago
Created attachment 154325 [details] [diff] [review]
kiko_v3: okidoki
(Assignee)

Updated

14 years ago
Attachment #154053 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154325 - Flags: review?(bugreport)

Comment 15

14 years ago
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.
Attachment #154325 - Flags: review?(bugreport) → review+
(Assignee)

Comment 16

14 years ago
Well, if you wanted to revoke that right, you should never have brought up the
issue in the first place <wink>. Thanks.
Flags: approval?
Flags: approval? → approval+
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 17

14 years ago
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

14 years ago
Oops. Some bits were still left in Auth/Login/WWW.pm -- just murdered them too.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.