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

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: API, defect)

Description

[David Lawrence [:dkl]]

See bug 1001462 for extra details.

Comment 1

[David Lawrence [:dkl]]

Attachment: 1090727_1.patch

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

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 8521521 [details] [diff] [review]
1090727_1.patch

Wrong patch for this bug.

Comment 3

[David Lawrence [:dkl]]

Attachment: 1097813_1.patch

Correct patch :)

Comment 4

[Dylan Hardison [:dylan] (he/him)]

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?

Comment 5

[David Lawrence [:dkl]]

To ssh://[e-mail]/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

Comment 6

[David Lawrence [:dkl]]

(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