Bug 1209148 - Stop using short-life token for internal REST API auth, which is problematic especially on My Dashboard, and rely on login cookie instead

[kyoshino]

Description

• Internal Bugzilla_api_token is problematic and redundant. The existing login cookie can be used instead for API auth.
• Public Bugzilla_token is a different thing, and it should not be affected by this change.

Bug

Bug 1209148 - Stop using short-life token for internal REST API auth, which is problematic especially on My Dashboard, and rely on login cookie instead

[kyoshino]

So... I tried, and looks like this is working. The real culprit is $login_cookie = '' in Cookie.pm.

[kyoshino]

/rest/bug/attachment/${id} I want to use for Bug 1496207 doesn't work when the Bugzilla_api_token param is set in the query string. It's a PUT request. I would like to this PR get reviewed and merged so the problem should be gone.

The requested method 'Bugzilla::Attachment::set_Bugzilla_api_token' was not found.

[kyoshino]

I just encountered the invalid token error on My Dashboard 😐

[floatingatoll]

I encountered this today after updating Nightly (I skipped the broken-cookie build), and am glad to see we're resolving it.

[kyoshino]

This is basically a removal so no code review needed. Just want to make sure this works as expected and no security issues introduced.

[floatingatoll]

@kyoshino What circumstances can activate this code block today?

https://github.com/mozilla-bteam/bmo/pull/782/files#diff-e432c9f5510c145bfbb4a9eb373e5209L107

            # forbid logging in with a cookie if only api-keys are allowed
            if (i_am_webservice() && !$is_internal) {

[kyoshino]

I meant: no review for newly added code is required...

And I think removing the forbid logging in with a cookie if only api-keys are allowed part is the right thing. It is the source of the issue, and we will instead rely on cookies.

[floatingatoll]

(I'd planned to ask that question in any case, but thank you for clarifying all the same!)

[kyoshino]

Maybe @floatingatoll is right. That particular part should not be removed after all. I'm reverting it and only remove the is_internal check. But hmm, what happens then? I wasn't aware of the api_key_only setting.

[dylanwh]

I think api_key only might just have been for preventing CSRF attacks, which is now mitigated by SameSite cookies.

[kyoshino]

So can we remove the api_key_only setting also?

[dklawren]

https://github.com/mozilla-bteam/bmo/blob/master/docs/en/rst/api/core/v1/general.rst

mentions Bugzilla_token and short form token= so the docs need to be updated as well.

[kyoshino]

It’s very confusing but Bugzilla_token or token is a long-life token which is different from the undocumented Bugzilla_api_token param for internal API calls. We are going to only remove the latter in favour of cookies.

[dylanwh]

since dkl requeted changes, I'll do the same so this drops off my radar.

[kyoshino]

This PR needs to be updated once #917 is merged.