webplatform / annotation-service

Hypothes.is’ container project to customize for notes.webplatform.org
1 stars 0 forks source link

Making Authentication settings configurable per deployment #25

Closed renoirb closed 9 years ago

renoirb commented 9 years ago

Provided we want to deploy notes-server on staging, we need to be able to configure where to delegate authentication.

Question: Is there anything I’m missing to have configurable authentication parameters?

Config proposal

webplatform.accounts_auth: https://oauth.accounts.webplatformstaging.org/v1/authorization
webplatform.accounts_token: https://oauth.accounts.webplatformstaging.org/v1/token
webplatform.accounts_session: https://profile.accounts.webplatformstaging.org/v1/session

Patch proposal

diff --git a/notes_server/__init__.py b/notes_server/__init__.py
index dce93f4..a426d63 100644
--- a/notes_server/__init__.py
+++ b/notes_server/__init__.py
@@ -19,10 +19,6 @@ import logging
 log = logging.getLogger(__name__)

-AUTHZ_ENDPOINT = 'https://oauth.accounts.webplatform.org/v1/authorization'
-TOKEN_ENDPOINT = 'https://oauth.accounts.webplatform.org/v1/token'
-SESSION_ENDPOINT = 'https://profile.accounts.webplatform.org/v1/session/'
-

 class OAuthConsumer(Consumer):
     def __init__(self, key, **kwargs):
@@ -88,7 +84,7 @@ def login(request):
             prepped = provider.prepare_request(req)
             provider.token = provider.send(prepped).json()
             provider._client.access_token = provider.token['access_token']
-            profile = provider.get(SESSION_ENDPOINT + 'read').json()
+            profile = provider.get(settings['webplatform.accounts_session'] + '/read').json()
             provider_login = profile['username']
             userid = 'acct:{}@{}'.format(provider_login, request.domain)
             request.response.headerlist.extend(remember(request, userid))
@@ -113,7 +109,7 @@ def logout(request):
 def session_recover(request):
     payload = request.params.get('recoveryPayload')
     profile = requests.get(
-        SESSION_ENDPOINT + 'recover',
+        settings['webplatform.accounts_session'] + '/recover',
         headers={'Authorization': 'Session {}'.format(payload)}
     ).json()
     provider_login = profile.get('username')
@@ -131,10 +127,10 @@ def includeme(config):
     registry.registerUtility(OAuthConsumer, IConsumerClass)
     config.include('h')

-    authz_endpoint = settings.get('webplatform.authorize', AUTHZ_ENDPOINT)
+    authz_endpoint = settings.get('webplatform.authorize', settings['webplatform.accounts_auth'])
     config.add_route('authorize', authz_endpoint)

-    token_endpoint = settings.get('webplatform.token', TOKEN_ENDPOINT)
+    token_endpoint = settings.get('webplatform.token', settings['webplatform.accounts_token'])
     config.add_route('token', token_endpoint)

     config.add_route('login', '/login')
tilgovi commented 9 years ago

The first two changes are right, but the second two are not.

The get method on Python dictionaries has a second argument which is the default if the key does not exist.

In other words, the authz_endpoint and token_endpoint are already both configurable as webplatform.authorize and webplatform.token in the .ini.

renoirb commented 9 years ago

Right, I didn’t notice at first. settings['foo.bar'] or settings.get('foo.bar', 'default_value') access the same value but if foo.bar isn’t defined default_value will be given. I’ll take a look at it on Monday

renoirb commented 9 years ago

Updated patch proposal. What do you think @tilgovi ?

diff --git i/development.ini w/development.ini
index 135af51..fd6efe2 100644
--- i/development.ini
+++ w/development.ini
@@ -4,6 +4,10 @@ use: egg:notes_server
 api.endpoint: /api
 api.key: https://notes.webplatform.org

+webplatform.authorize: 'https://oauth.accounts.webplatform.org/v1/authorization'
+webplatform.token: 'https://oauth.accounts.webplatform.org/v1/token'
+webplatform.session: 'https://profile.accounts.webplatform.org/v1/session'
+
 es.host: http://localhost:9200

 mail.default_sender: "WebPlatform Specs Notes Archiver" <notifier-notes@webplatform.org>
diff --git i/notes_server/__init__.py w/notes_server/__init__.py
index dce93f4..7919bdb 100644
--- i/notes_server/__init__.py
+++ w/notes_server/__init__.py
@@ -21,7 +21,7 @@ log = logging.getLogger(__name__)

 AUTHZ_ENDPOINT = 'https://oauth.accounts.webplatform.org/v1/authorization'
 TOKEN_ENDPOINT = 'https://oauth.accounts.webplatform.org/v1/token'
-SESSION_ENDPOINT = 'https://profile.accounts.webplatform.org/v1/session/'
+SESSION_ENDPOINT = 'https://profile.accounts.webplatform.org/v1/session'

 class OAuthConsumer(Consumer):
@@ -88,7 +88,8 @@ def login(request):
             prepped = provider.prepare_request(req)
             provider.token = provider.send(prepped).json()
             provider._client.access_token = provider.token['access_token']
-            profile = provider.get(SESSION_ENDPOINT + 'read').json()
+            session_endpoint = settings.get('webplatform.session', SESSION_ENDPOINT) + '/read'
+            profile = provider.get(session_endpoint).json()
             provider_login = profile['username']
             userid = 'acct:{}@{}'.format(provider_login, request.domain)
             request.response.headerlist.extend(remember(request, userid))
@@ -112,8 +113,9 @@ def logout(request):
 @view_config(route_name='recover')
 def session_recover(request):
     payload = request.params.get('recoveryPayload')
+    session_endpoint = settings.get('webplatform.session', SESSION_ENDPOINT) + '/recover'
     profile = requests.get(
-        SESSION_ENDPOINT + 'recover',
+        session_endpoint,
         headers={'Authorization': 'Session {}'.format(payload)}
     ).json()
     provider_login = profile.get('username')
diff --git i/production.ini w/production.ini
index 1838f15..8e86bdb 100644
--- i/production.ini
+++ w/production.ini
@@ -27,6 +27,10 @@ use: egg:notes_server
 api.endpoint: /api
 api.key: https://notes.webplatform.org

+webplatform.authorize: 'https://oauth.accounts.webplatform.org/v1/authorization'
+webplatform.token: 'https://oauth.accounts.webplatform.org/v1/token'
+webplatform.session: 'https://profile.accounts.webplatform.org/v1/session'
+
 # ElasticSearch configuration
 #es.host: http://localhost:9200
 #es.index: annotator
renoirb commented 9 years ago

Deployed and working