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

Status: NEW

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

Description

[Botond Ballo [:botond]]

STR:
  1. Open the My Dashboard page. Leave it in a background tab.
  2. Be away from your computer for a few days, such as from
     your office computer over the weekend.
  3. When coming back, restart the browser (e.g. because a new
     Nightly was installed).
  4. Activate the tab with My Dashboard open.

Expected results:
  The page loads successfully.

Actual results:
  I get a dialog saying:
  
    Failed to load flag list from bugzilla.
    The token is not valid. It could be because you loaded this page more than 3 days ago.


This is a recent regression, it only started happening a couple of weeks ago.

Comment 1

[Botond Ballo [:botond]]

(Reloading the page then makes it load successfully.)

Comment 2

[Mark Côté [:mcote]]

This is the result of the implementation of API keys.  I'll let someone else explain the specific reason (I always get it confused myself), but I will add the comment that we should have the page autorefresh in this case, if possible (bwinton's idea).

Comment 3

[David Lawrence [:dkl]]

For UI elements that make ajax calls to the backend, we use short-lived tokens (< 3 days) as a parameter when making the api call. So this is by design. I admin we need to make the error more user friendly. Maybe it should read:

"The API token is not valid. It could be because you loaded this page more than 3 days ago. Please reload this page to obtain a new API token."

dkl

Comment 4

[David Lawrence [:dkl]]

(In reply to Mark Côté [:mcote] from comment #2)
> but I will add the comment that we should have the page autorefresh in this case, if
> possible (bwinton's idea).

We would need a full page refresh to get a new token, so I am not sure if past ideas were to refresh just the individual lists which happens when you click 'Refresh' for each one. 

We could have the page just refresh once a day which would fix this specific issue and have the individual lists refresh on some user-defined interval more frequently that once a day.

Wonder if it would be an issue if we just refreshed the API token in the background without doing a full page refresh.

dkl

Comment 5

[Botond Ballo [:botond]]

I see the message on show_bug.cgi, too (with the same STR), but there it's just a message at the top of the page, it doesn't prevent the page from loading.

Comment 6

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #4)
> We could have the page just refresh once a day which would fix this specific
> issue and have the individual lists refresh on some user-defined interval
> more frequently that once a day.
> 
> Wonder if it would be an issue if we just refreshed the API token in the
> background without doing a full page refresh.

another option would be to detect when the call failed due to an invalid api token and perform a full page refresh if required.  that would avoid unnecessary api requests.

Comment 7

[:glob ✱]

> Wonder if it would be an issue if we just refreshed the API token in the
> background without doing a full page refresh.

that isn't an option -- we'd have to allow an api request without a valid token which results in a valid token.. this completely defeats the point of having tokens (you'd be able to springboard from a cookie to a token and then make any call).

Comment 8

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #7)
> > Wonder if it would be an issue if we just refreshed the API token in the
> > background without doing a full page refresh.
> 
> that isn't an option -- we'd have to allow an api request without a valid
> token which results in a valid token.. this completely defeats the point of
> having tokens (you'd be able to springboard from a cookie to a token and
> then make any call).

No. My thinking was we would do the refresh well before the current API token has expired 
and would still be required to be valid. Only then would the newer token be given and
the clock started over.

(In reply to Byron Jones ‹:glob› from comment #6)
> > Wonder if it would be an issue if we just refreshed the API token in the
> > background without doing a full page refresh.
> 
> another option would be to detect when the call failed due to an invalid api
> token and perform a full page refresh if required.  that would avoid
> unnecessary api requests.

That works for me as well. Should we just do the page refresh without user approval
or throw a dialog with the error message and have a button that refreshes the page?
If we go with full disclosure, then the latter is preferrable. If we are working towards
automatically refreshing the whole page periodically anyway, then we can just do the
former.

dkl

Comment 9

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #8)
> That works for me as well. Should we just do the page refresh without user
> approval or throw a dialog with the error message and have a button tha
> refreshes the page?

there's no need to prompt the user -- the only two options are "a broken page" or a refresh .. that isn't much of a choice and one the user doesn't need to be involved in :)

> No. My thinking was we would do the refresh well before the current API
> token has expired and would still be required to be valid. Only then would
> the newer token be given and the clock started over.

ah, that make sense.  let's chat about this in tonight's meeting :)

Comment 10

[Botond Ballo [:botond]]

Any update on this? I still see this multiple times a week.

Comment 11

[Ed Morley [:emorley]]

I also see this multiple times a week, since I leave the dashboard pinned as an app-tab.

What makes the error particularly frustrating is the fact that it is shown three times in a row (presumably once for each table).

Comment 12

[Botond Ballo [:botond]]

Any plans to fix this?

Comment 13

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

Yes, by making the dashboard use a long-lived API key stored in localStorage.

Comment 14

[Ed Morley [:emorley]]

In the meantime could the dashboard just reload the page instead of displaying the alert? (Seeing as the dashboard doesn't contain editable fields that could be lost, unlike show_bug.cgi etc)

Comment 17

[Kohei Yoshino [:kohei]]

:dylan says he is working to solve this issue. The authentication will be using a cookie instead of a token.

Comment 18

[Kohei Yoshino [:kohei]]

I’ve sent a PR for this.

Comment 19

[Kohei Yoshino [:kohei]]

Attachment: GitHub Pull Request