Skip to content

Commit

Permalink
Handle revoked oauth permissions by the user (#4074)
Browse files Browse the repository at this point in the history
* 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.

* Remove unnecessary exception handling

* Style

* Add a user persistent notification when social access is revoked

* 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.

* Remove invalid imports

* Improve comment

* Add final dot to the phrase

* Remove unused return
  • Loading branch information
humitos authored and agjohnson committed Jun 14, 2018
1 parent aafd455 commit 1fa6cf6
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 10 deletions.
17 changes: 13 additions & 4 deletions readthedocs/notifications/backends.py
@@ -1,3 +1,4 @@
# -*- coding: utf-8 -*-
"""
Pluggable backends for the delivery of notifications.
Expand All @@ -6,7 +7,9 @@
displayed on the site.
"""

from __future__ import absolute_import
from __future__ import (
absolute_import, division, print_function, unicode_literals)

from builtins import object
from django.conf import settings
from django.http import HttpRequest
Expand All @@ -15,7 +18,7 @@

from readthedocs.core.utils import send_email

from .constants import LEVEL_MAPPING, REQUIREMENT, HTML
from .constants import HTML, LEVEL_MAPPING, REQUIREMENT


def send_notification(request, notification):
Expand Down Expand Up @@ -53,14 +56,20 @@ class EmailBackend(Backend):
name = 'email'

def send(self, notification):
# FIXME: if the level is an ERROR an email is received and sometimes
# it's not necessary. This behavior should be clearly documented in the
# code
if notification.level >= REQUIREMENT:
send_email(
recipient=notification.user.email,
subject=notification.get_subject(),
template='core/email/common.txt',
template_html='core/email/common.html',
context={
'content': notification.render(self.name, source_format=HTML),
'content': notification.render(
self.name,
source_format=HTML,
),
},
request=self.request,
)
Expand Down Expand Up @@ -91,7 +100,7 @@ def send(self, notification):
level=LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT),
message=notification.render(
backend_name=self.name,
source_format=HTML
source_format=HTML,
),
extra_tags='',
user=notification.user,
Expand Down
24 changes: 23 additions & 1 deletion readthedocs/oauth/services/base.py
Expand Up @@ -8,6 +8,7 @@
from datetime import datetime

from allauth.socialaccount.models import SocialAccount
from allauth.socialaccount.providers import registry
from builtins import object
from django.conf import settings
from oauthlib.oauth2.rfc6749.errors import InvalidClientIdError
Expand Down Expand Up @@ -56,6 +57,10 @@ def get_adapter(self):
def provider_id(self):
return self.get_adapter().provider_id

@property
def provider_name(self):
return registry.by_id(self.provider_id).name

def get_session(self):
if self.session is None:
self.create_session()
Expand Down Expand Up @@ -131,6 +136,22 @@ def paginate(self, url, **kwargs):
"""
try:
resp = self.get_session().get(url, data=kwargs)

# TODO: this check of the status_code would be better in the
# ``create_session`` method since it could be used from outside, but
# I didn't find a generic way to make a test request to each
# provider.
if resp.status_code == 401:
# 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
raise Exception(
'Our access to your {provider} account was revoked. '
'Please, reconnect it from your social account connections.'.format(
provider=self.provider_name,
),
)

next_url = self.get_next_url_to_paginate(resp)
results = self.get_paginated_results(resp)
if next_url:
Expand Down Expand Up @@ -202,4 +223,5 @@ def is_project_service(cls, project):
# TODO Replace this check by keying project to remote repos
return (
cls.url_pattern is not None and
cls.url_pattern.search(project.repo) is not None)
cls.url_pattern.search(project.repo) is not None
)
13 changes: 8 additions & 5 deletions readthedocs/oauth/tasks.py
@@ -1,11 +1,14 @@
"""Tasks for OAuth services"""
# -*- coding: utf-8 -*-
"""Tasks for OAuth services."""

from __future__ import (
absolute_import, division, print_function, unicode_literals)

from __future__ import absolute_import
from django.contrib.auth.models import User

from readthedocs.core.utils.tasks import PublicTask
from readthedocs.core.utils.tasks import permission_check
from readthedocs.core.utils.tasks import user_id_matches
from readthedocs.core.utils.tasks import (
PublicTask, permission_check, user_id_matches)

from .services import registry


Expand Down

0 comments on commit 1fa6cf6

Please sign in to comment.