WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
392 stars 630 forks source link

Dashboard does not inform user when they need to reauthorize OAuth #317

Closed ragesoss closed 2 years ago

ragesoss commented 9 years ago

In some cases, a user's OAuth credentials may become invalid, causing the WikiEdits.get_tokens method (and the subsequent edit attempt) to fail. If get_tokens fails with the error mwoauth-invalid-authorization then we should prompt the user to reauthorize.

ragesoss commented 9 years ago

A first pass at this has been implemented; the user gets logged out and there is a flash message informing the user of that they need to log in again.

However, the flash message doesn't play well with React. As long as the user is still on the same course page (and therefore within the same react-root, I guess?) the page will not render the flash message or show the user that they have been logged out.

How can we push the error message and logged_out status to the user through React?

thenickcox commented 8 years ago

@ragesoss bump. @bmathews seemed to address this at least. Any further action required?

ragesoss commented 8 years ago

@thenickcox There's still a ways go with this:

Here's a flow for testing it:

Currently, there is no notification for some cases — I can only get a notification for the 'enroll a student' case — and the UI still shows the user as logged in.

thenickcox commented 8 years ago

Let's talk about an approach for this. GMail has a UI for this, where you sign out in another tab, then switch back and it pops up an alert and signs you out. Implementation-wise, it seems to poll some sort of auth endpoint to make sure you're still logged in. Not totally sold on that approach, but that's certainly one way to do it. Thoughts?

ragesoss commented 8 years ago

As a first step, we can probably at least handle the situation where the notification works: make the frontend reload the page if the Unauthorized error is caught. (That might make it so that the flash notification that explains the logout situation will appear, as well, so that we don't have to worry about the lost notification.)

I don't think we necessarily need to poll for status, if we can instead return an indication of success / failure for each edits that could trigger a logout. The logout will almost always happen immediately after a user attempts to save a change that triggers a wiki edit. Although it's possible to get logged out in other situations, those are rare edge cases. The edits that don't trigger error notifications might be because our system returns success (of the POST to dashboard) before it makes the on-wiki edit; if that's the case, we can bring the wiki edit into the loop and make sure that we return info about failed edits in the promises.

On the other hand, a polling system might be usefully more generally in the long term... for example, for pushing notifications that are independent of actions taken by the user. ('Your student User:Thenickcox just made an edit that got flagged for plagiarism.')

thenickcox commented 8 years ago

I didn't quite have a "polling system" in mind so much as a simple API endpoint that could be polled with a single value. It'd literally be a setTimeout call. We could use the same approach for things like plagiarism, too. Could be we proxy a call to a few APIs and return a few values. I guess that'd be a "polling system". Seems like we could just poll https://en.wikipedia.org/w/api.php?action=query&meta=tokens&format=json every 30s and check for the csrf_token key, right?

Edit

Or, like, have the front end query /auth-check, which calls WikiEdits.get_tokens(current_user), and checks that response? That'd be pretty quick.

Edit 2

The reason I'd advocate for polling is that we'd ideally like to notify the user before they make an edit that won't get saved because they're logged out.

ragesoss commented 8 years ago

That's possible (and we already have a method for it, WikiEdits.verify_oauth_credentials(current_user)) but that's not where the problem here is. Once the system tries to make an edit and fails because of oauth credentials, the server immediately sets their oauth token to invalid. The user also gets logged out by the application controller when current_user has an invalid token. However, the frontend doesn't update to reflect this.

We could just poll our own server — maybe make ApplicationController#check_for_expired_oauth_credentials handle json requests differently from html requests — and it will solve most of the problem.

It might also avoid some lost edits if we did recheck oauth credentials once (via WikiEdits.verify_oauth_credentials(current_user), right when a user loads a course. We probably don't need to keep polling that continually though.

thenickcox commented 8 years ago

Wait, ApplicationController#check_for_expired… only checks to see if I have a valid Wiki token ( unless current_user && current_user.wiki_token == 'invalid'). I just revoked access, and it's not signing me out. It doesn't actually make a request. Don't we actually have to call WikiEdits.verify_oauth_credentials(current_user) to make a check?

ragesoss commented 8 years ago

Yes... but most of the time we already know — the user already had a failed token request and got their wiki token set to invalid — and we just need to make the client realize this. But, as I said, doing that call once, when the user first loads a course page, would probably be helpful to catch some auth problems before the user tries to make an edit. (That's why I created the method in the first place, but I never got around to implementing any actual usage of it.)

ragesoss commented 8 years ago

After 0ce32db23169b4e8c806643c342b9304d261d52a, this should be less pressing of an issue. It would still be good to reload the page if the dashboard learns that an oauth token is invalid by trying and failing to save an edit.

ragesoss commented 6 years ago

This hasn't been much of an issue since that last related commit, although it does happen every once in a while.

ragesoss commented 2 years ago

I think the current situation is fine. This does not come up commonly by any stretch.