From 45275c38310a429fc291dcecc8a7501dd083ce09 Mon Sep 17 00:00:00 2001
From: Francesco Siddi <francesco.siddi@gmail.com>
Date: Wed, 23 Aug 2017 17:58:52 +0200
Subject: [PATCH] Switch to class-based OAuthUserResponse

Instead of returning an arbirary number of items, we provide a standardized and better documented response.
---
 pillar/auth/oauth.py       | 53 +++++++++++++++++++++++---------------
 pillar/web/users/routes.py |  6 ++---
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/pillar/auth/oauth.py b/pillar/auth/oauth.py
index 05f879f7..b746a860 100644
--- a/pillar/auth/oauth.py
+++ b/pillar/auth/oauth.py
@@ -1,10 +1,22 @@
+import abc
+import attr
 import json
 
 from rauth import OAuth2Service
 from flask import current_app, url_for, request, redirect, session
 
 
-class OAuthSignIn:
+@attr.s
+class OAuthUserResponse:
+    """Represents user information requested to an OAuth provider after
+    authenticating.
+    """
+
+    id = attr.ib(validator=attr.validators.instance_of(str))
+    email = attr.ib(validator=attr.validators.instance_of(str))
+
+
+class OAuthSignIn(metaclass=abc.ABCMeta):
     providers = None
 
     def __init__(self, provider_name):
@@ -13,10 +25,22 @@ class OAuthSignIn:
         self.consumer_id = credentials['id']
         self.consumer_secret = credentials['secret']
 
-    def authorize(self):
+    @abc.abstractmethod
+    def authorize(self) -> redirect:
+        """Redirect to the corret authorization endpoint for the current provider
+
+        Depending on the provider, we sometimes have to specify a different
+        'scope'.
+        """
         pass
 
-    def callback(self):
+    @abc.abstractmethod
+    def callback(self) -> OAuthUserResponse:
+        """Callback performed after authorizing the user
+
+        This is usually a request to a protected /me endpoint to query for
+        user information, such as user id and email address.
+        """
         pass
 
     def get_callback_url(self):
@@ -72,14 +96,9 @@ class BlenderIdSignIn(OAuthSignIn):
         
         # TODO handle exception for failed oauth or not authorized
 
-        me = oauth_session.get('user').json()
-        # TODO handle case when user chooses not to disclose en email
         session['blender_id_oauth_token'] = oauth_session.access_token
-        return (
-            me['id'],
-            me.get('email'),
-            oauth_session.access_token
-        )
+        me = oauth_session.get('user').json()
+        return OAuthUserResponse(str(me['id']), me['email'])
 
 
 class FacebookSignIn(OAuthSignIn):
@@ -115,11 +134,8 @@ class FacebookSignIn(OAuthSignIn):
         )
         me = oauth_session.get('me?fields=id,email').json()
         # TODO handle case when user chooses not to disclose en email
-        return (
-            me['id'],
-            me.get('email'),
-            None
-        )
+        # see https://developers.facebook.com/docs/graph-api/reference/user/
+        return OAuthUserResponse(me['id'], me.get('email'))
 
 
 class GoogleSignIn(OAuthSignIn):
@@ -154,9 +170,4 @@ class GoogleSignIn(OAuthSignIn):
             decoder=decode_json
         )
         me = oauth_session.get('userinfo').json()
-        # TODO handle case when user chooses not to disclose en email
-        return (
-            me['id'],
-            me.get('email'),
-            None
-        )
+        return OAuthUserResponse(str(me['id']), me['email'])
diff --git a/pillar/web/users/routes.py b/pillar/web/users/routes.py
index 85da6036..14033ad2 100644
--- a/pillar/web/users/routes.py
+++ b/pillar/web/users/routes.py
@@ -40,13 +40,13 @@ def oauth_callback(provider):
     if not current_user.is_anonymous:
         return redirect(url_for('main.homepage'))
     oauth = OAuthSignIn.get_provider(provider)
-    social_id, email, access_token = oauth.callback()
-    if social_id is None:
+    oauth_user = oauth.callback()
+    if oauth_user.id is None:
         log.debug('Authentication failed for user with {}'.format(provider))
         return redirect(url_for('main.homepage'))
 
     # Find or create user
-    user_info = {'id': social_id, 'email': email, 'full_name': ''}
+    user_info = {'id': oauth_user.id, 'email': oauth_user.email, 'full_name': ''}
     db_user = find_user_in_db(user_info, provider=provider)
     db_id, status = upsert_user(db_user)
     token = generate_and_store_token(db_id)