backport upstream bug 1001462 to bmo/4.2 to fix issue with using tokens with webservice rest api

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Production
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

See bug 1001462 for extra details.
Posted patch 1090727_1.patch (obsolete) — Splinter Review
Also made some extra backport from upstream to no longer need the hack in User.login to construct the token from the CGI cookies.

dkl
Attachment #8521521 - Flags: review?(dylan)
Comment on attachment 8521521 [details] [diff] [review]
1090727_1.patch

Wrong patch for this bug.
Attachment #8521521 - Attachment is obsolete: true
Attachment #8521521 - Flags: review?(dylan)
Correct patch :)
Attachment #8522346 - Flags: review?(dylan)
Comment on attachment 8522346 [details] [diff] [review]
1097813_1.patch

Review of attachment 8522346 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan but fix the re-declaration of $user in Bugzilla/WebService/User.pm before commit.

::: Bugzilla/WebService/User.pm
@@ +65,5 @@
>  sub login {
>      my ($self, $params) = @_;
> +
> +    # Check to see if we are already logged in
> +    my $user = Bugzilla->user;

This variable name is already declared later on.
Possible fixes:

1) Inline the later use (see comment below)
2) user a different name
3) don't re-declare it below

For #3, I'd declare $user under my ($self, ...).

It looks like in the upstream patch $user is not redeclared, so the right fix is probably just to do that?

@@ +80,2 @@
>      my $user = Bugzilla->login();
> +    return $self->_login_to_hash($user);

This could be inlined as return $self->_login_to_hash(Bugzilla->login);

@@ +398,5 @@
>  
> +sub _login_to_hash {
> +    my ($self, $user) = @_;
> +    my $item = { id => $self->type('int', $user->id) };
> +    if ($user->{_login_token}) {

Hmm, directly accessing a hash slot on an object? Is that kosher? seems pretty icky to me. Is this a performance optimization? If so maybe we should leave a comment here indicating that?
Attachment #8522346 - Flags: review?(dylan) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   ae3093f..9876111  master -> master

(In reply to Dylan William Hardison [:dylan] from comment #4)
> > +sub _login_to_hash {
> > +    my ($self, $user) = @_;
> > +    my $item = { id => $self->type('int', $user->id) };
> > +    if ($user->{_login_token}) {
> 
> Hmm, directly accessing a hash slot on an object? Is that kosher? seems
> pretty icky to me. Is this a performance optimization? If so maybe we should
> leave a comment here indicating that?

Relatively harmless and done quite a bit in the code but admittedly bad practice in general. Will file an upstream bug to add an accessor/setter to the User.pm module and update the needed places.

Thanks
dkl
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(In reply to David Lawrence [:dkl] from comment #5)
> Relatively harmless and done quite a bit in the code but admittedly bad
> practice in general. Will file an upstream bug to add an accessor/setter to
> the User.pm module and update the needed places.

Actually now that I think about it, login_token will go away for 6.0 anyway in favor of api keys so we will just leave this as it is ($user->{_login_token}).

dkl
You need to log in before you can comment on or make changes to this bug.