New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle revoked oauth permissions by the user #4074

Merged
merged 10 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented May 9, 2018

If the user revoked our app permission from the external service web page, we are not notified. So, next time we try to use it, we will receive a 401 status code in the first request.

Here we handle this case and we raised an Exception from the Celery task, which is shown in the UI while pulling the Public Task information.

Missing things:

  • implement a proper persistent message that the user can close
  • find a way to make a general test request from create_session instead of paginate

Related: rtfd/readthedocs-corporate#323
Related: #4070
Based on: #4078 (already merged)

@humitos humitos self-assigned this May 10, 2018

# FE keeps waiting forever. Also, if we have 3 accounts
# connected and the first one fails, the other two are not
# executed.
try:

This comment has been minimized.

@humitos

humitos May 17, 2018

Member

This has to be removed from here becase of #4070

@ericholscher ericholscher requested a review from agjohnson May 24, 2018

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 30, 2018

This looks good, but I think @agjohnson is probably the one who should review it, since I'm not as familiar with this code.

# Bad credentials: the token we have in our database is not
# valid. Probably the user has revoked the access to our App. He
# needs to reconnect his account
message = 'Our access to your {} account was revoked. You ' \

This comment has been minimized.

@humitos

humitos Jun 4, 2018

Member

Consdider to use a better general message here and also maybe catch the 404 --in a another PR with more testing, probably.

@humitos

This comment has been minimized.

Member

humitos commented Jun 4, 2018

This is very close to be functional.

I used our custom notification app and the Notification class which uses the SiteBackend and it worked.

captura de pantalla_2018-06-04_17-54-09

There are still some problems:

  1. there is no way to close/accept/dismiss the notification once it's promped to the user
  2. the notification is not immediately shown since it doesn't comes from the PublicTask --we need to refresh the page
  3. once the notification is shown, if I go to the Admin settings, when I come back it disappears (it's marked as read)
@humitos

This comment has been minimized.

Member

humitos commented Jun 4, 2018

Now, after implementing this and re read #4078 I'm thinking that we may want just to raise an Exception inside the task when we receive the 401 which should be catched by that PR and properly shown immediately after sync and not persistent. @agjohnson what do you think?

@humitos

This comment has been minimized.

Member

humitos commented Jun 4, 2018

We decided to not use persistent message for this.

So, I just based this branch in #4078 and raise a simple Exception with this message:

captura de pantalla_2018-06-04_18-28-57

humitos added some commits May 8, 2018

Handle general oauth services errors
This is a way to protect ourselves from random errors when syncing
OAuth services (create repository/organizations).

If a global exception is raised, we just skip this service and
continue with the rest of the connected ones.
Handle revoked oauth permissions by the user
If the user revoked our app permission from the external service web
page, we are not notified. So, next time we try to use it, we will
receive a 401 status code in the first request.

Here we handle this case and we show a Persistent Notification to the
user so he/she knows that a reconnection of the account is needed.
Raise an Exception when permissions revoked
Raise a simple Exception when the permissions from a particular social
service are revoked instead of using a persistent notification.

We want an Exception because it's immediately communicated using the
PublicTask mechanism. Otherwise, the user needs to refresh the page to
see the notification.
@humitos

This comment has been minimized.

Member

humitos commented Jun 11, 2018

This is ready to be merged after rebased and re-tested (QA).

@humitos humitos requested a review from rtfd/core Jun 11, 2018

@agjohnson

LGTM

@agjohnson agjohnson merged commit 1fa6cf6 into master Jun 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/message/revoked-oauth-access branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment