Multiple clients and scopes #52

Natim · 2017-11-22T16:21:35Z

f? @rfk @vladikoff

Fixes #51

vladikoff · 2017-11-22T16:22:33Z

Wow! The Speed! 🚤

Natim · 2017-11-22T17:59:25Z

Wow! The Speed! 🚤

To be honest I didn't expect it too be wrapped up so quickly but we paired with @leplatrem 🙌

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/__init__.py

@@ -45,6 +46,10 @@ def includeme(config):
        settings['fxa-oauth.requested_scope'] = settings['fxa-oauth.scope']
        settings['fxa-oauth.required_scope'] = settings['fxa-oauth.scope']

+    resources, scope_routing = parse_resources(settings)


parse_resources() might not be the best name

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

@@ -14,6 +14,8 @@

 logger = logging.getLogger(__name__)

+KEY = 'fxa_verified_token'


REIFY_KEY ?

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

@@ -49,6 +51,7 @@ class FxAOAuthAuthenticationPolicy(base_auth.CallbackAuthenticationPolicy):
    def __init__(self, realm='Realm'):
        self.realm = realm
        self._cache = None
+        self._config = None


leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

-        return self._verify_token(token, request)
+
+        user_id, client_name = self._verify_token(token, request)
+


Don't add suffix if authentication failed, or no specific client name is configured

Is it a comment you want to add?

Yes for the return user_id below

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

+                try:
+                    profile = auth_client.verify_token(token=token, scope=aslist(scope))
+                    user_id = profile['user']
+                    scopes = profile['scope']


nit: plural vs singular

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

+                    # Make sure the bearer token scopes don't match multiple configs.
+                    if len([x for x in request.registry._fxa_oauth_scope_routing.keys()
+                            if x and set(x.split()) & set(scopes)]) > 1:
+                        logger.debug("Invalid FxA token: {} matches multiple config" % scopes)


We might want to warn no?

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

@@ -122,6 +141,11 @@ def _get_cache(self, request):

        return self._cache

+    def callback(self, userid, request):
+        if request.bound_data[KEY][1] != "default":


no safety check here?

Is this adding additional principals, beyond the userid?

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/authentication.py

@@ -122,6 +141,11 @@ def _get_cache(self, request):

        return self._cache

+    def callback(self, userid, request):
+        if request.bound_data[KEY][1] != "default":
+            return ["fxa:{}".format(request.bound_data[KEY][0])]


I think this could be clearer with comments or intermediary variables

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/tests/test_authentication.py

+        api_mocked.return_value = self.profile_data
+        principals = self.policy.effective_principals(self.request)
+        self.assertIn("fxa:33", principals)
+        self.assertIn("33-notes", principals)


AssertNotIn("33") ?

leplatrem · 2017-11-22T21:28:19Z

kinto_fxa/utils.py

+
+
+def parse_resources(settings):
+    resources = OrderedDict()


if settings is not an ordered dict I don't think it makes sense

leplatrem · 2017-11-22T21:28:57Z

Update README / Documentation

rfk · 2017-11-23T00:29:10Z

kinto_fxa/authentication.py

+        if client_name is None or client_name == 'default':
+            return user_id
+
+        return '{}-{}'.format(user_id, client_name)


The doc at [1] suggests this should have an "fxa:" prefix, like "fxa:5998e2e2e81c40bdad6d08ac2773bf61-lockbox".

[1] https://docs.google.com/document/d/19Tg5AyRdHDP7z_J6kWM-JgiO6tB6FmUuEXUweENYZq8/edit?ts=5a155487#

Yes this will be added by kinto.

rfk · 2017-11-23T00:40:22Z

kinto_fxa/authentication.py

+
+                    # Make sure the bearer token scopes don't match multiple configs.
+                    if len([x for x in request.registry._fxa_oauth_scope_routing.keys()
+                            if x and set(x.split()) & set(scopes)]) > 1:


It's not clear to me whether this is doing what you intend. What if the scope routing here was something like:

{ "profile https://identity.mozilla.com/app/notes": "notes", "profile https://identity.mozilla.com/app/lockbox": "lockbox", }

Then it seems to me that a token with "profile https://identity.mozilla.com/app/lockbox" would also match the set-intersection check above for "profile https://identity.mozilla.com/app/notes", because they both contain the "profile" scope. Is that the intended behaviour?

Or, if the keys in _fxa_oauth_scope_routing are only ever intended to be a single scope like "https://identity.mozilla.com/app/notes" rather than a list of scopes, maybe we should explicitly error out rather than doing set(x.split()) and so-on here.

On a side note: there's a lot of string splitting and parsing being done here on each request, I wonder if it would make sense to parse the scopes into a set or frozenset during config loading to make the checking here quicker.

Yes I need to add a few tests for that.

rfk · 2017-11-23T01:07:01Z

kinto_fxa/utils.py

+
+        if client_name in ('relier', 'state', 'webapp'):
+            setting_basename = '{}.{}'.format(client_name, setting_basename)
+            client_name = 'default'


This mixing of client names with other config parameters seems kinda fragile to me. What if a future version of this library introduces a new config parameter, that happens to share a name with a client in my config?

How would it look if, instead, we used something like "fxa-oauth.clients.lockbox.required_scope" in order to ensure there can't be confusion between the client names and other config names?

leplatrem · 2017-11-24T10:25:45Z

README.rst

+    fxa-oauth.clients.todo.required_scope = profile app-todo
+
+
+In that case Kinto will give you two different `user_id`:


Instead of in that case, I would say something like Depending on the requested scopes, Kinto Fxa will assign a user id or another (using suffix)

leplatrem · 2017-11-24T10:25:45Z

README.rst

+
+In that case Kinto will give you two different `user_id`:
+
+  - `fxa:user_id-notes` for the former


fxa:{user_id}-notes ?

leplatrem · 2017-11-24T10:25:45Z

README.rst

+  - `fxa:user_id-notes` for the former
+  - `fxa:user_id-todo` for the later
+
+Note that you can use `fxa:user_id` to explicitely share data between


you can still use the fxa:{user_id} principal

leplatrem · 2017-11-24T10:25:45Z

README.rst

+access the todo app data.
+
+The default buckets will also be isolated, one for `notes` and one for
+`todo`.


leplatrem · 2017-11-24T10:25:45Z

README.rst

+apps for a given FxA user.
+
+If you don't give any specific permission, it will be impossible for
+someone login in with the `app-notes` scope in their Bearer token to


leplatrem · 2017-11-24T10:25:45Z

kinto_fxa/authentication.py

-        return self._verify_token(token, request)
+
+        user_id, client_name = self._verify_token(token, request)
+


Yes for the return user_id below

leplatrem · 2017-11-24T10:25:45Z

kinto_fxa/authentication.py

+                    scope = profile['scope']
+                    client_name = client
+
+                    # Make sure the bearer token scope don't match multiple configs.


leplatrem · 2017-11-24T10:25:45Z

kinto_fxa/authentication.py

+                    client_name = client
+
+                    # Make sure the bearer token scope don't match multiple configs.
+                    if len([x for x in request.registry._fxa_oauth_scope_routing.keys()


Maybe we could declare routing_scopes outside the loop.
Plus we could iterate on it above instead of request.registry._fxa_oauth_scope_routing.items()

leplatrem · 2017-11-24T10:25:45Z

kinto_fxa/authentication.py

+
+                    # Make sure the bearer token scope don't match multiple configs.
+                    if len([x for x in request.registry._fxa_oauth_scope_routing.keys()
+                            if x and set(x.split()).issubset(set(scope))]) > 1:


nit: use intermediary variable to have a something like if len(intersecting_scopes) > 1

Natim · 2017-11-27T15:19:33Z

Tested and configured in the dev instance:

    "user": {
        "bucket": "159e598f-3c2b-6d93-9df9-74a334657c9e", 
        "id": "fxa:6ce43a4a28a74cb99b27243035c7dd55-notes", 
        "principals": [
            "fxa:6ce43a4a28a74cb99b27243035c7dd55-notes", 
            "system.Everyone", 
            "fxa:6ce43a4a28a74cb99b27243035c7dd55", 
            "system.Authenticated"
        ]
    }

Rémy HUBSCHER added 2 commits Nov 22, 2017

Handle multiple FxA clients.

e1d5c94

Make sure to not accept Bearer matching multiple configs.

7865e06

Fixes #51

Natim requested a review from leplatrem Nov 22, 2017

Rémy HUBSCHER added 2 commits Nov 22, 2017

Add a test with kinto-master.

6511635

Fix flake8

3de254c

leplatrem reviewed Nov 22, 2017

View changes

rfk reviewed Nov 23, 2017

View changes

Rémy HUBSCHER added 3 commits Nov 23, 2017

Refactor parse_clients.

f739e67

Add tests to make sure multiple required scopes matches accordingly.

d1c42b6

Updating the README.

4c46b77

leplatrem approved these changes Nov 24, 2017

View changes

@leplatrem review.

071933a

Natim merged commit 6f4308a into master Nov 27, 2017
2 checks passed

2 checks passed

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

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

Natim deleted the multiple-clients-and-scopes branch Nov 27, 2017

Kinto / kinto-fxa

Multiple clients and scopes #52

Multiple clients and scopes #52

Natim commented Nov 22, 2017

vladikoff commented Nov 22, 2017

Natim commented Nov 22, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

leplatrem commented Nov 22, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Natim commented Nov 27, 2017 •

edited

		@@ -14,6 +14,8 @@

		logger = logging.getLogger(__name__)

		KEY = 'fxa_verified_token'

		return self._verify_token(token, request)

		user_id, client_name = self._verify_token(token, request)

		fxa-oauth.clients.todo.required_scope = profile app-todo


		In that case Kinto will give you two different `user_id`:


		In that case Kinto will give you two different `user_id`:

		- `fxa:user_id-notes` for the former

Kinto / kinto-fxa

Multiple clients and scopes #52

Multiple clients and scopes #52

Conversation

Natim commented Nov 22, 2017

vladikoff commented Nov 22, 2017

Natim commented Nov 22, 2017

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

Natim Nov 23, 2017 Author Member

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

rfk Nov 23, 2017

This comment has been minimized.

Natim Nov 23, 2017 Author Member

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

This comment has been minimized.

leplatrem Nov 22, 2017 Contributor

leplatrem commented Nov 22, 2017

This comment has been minimized.

rfk Nov 23, 2017

This comment has been minimized.

Natim Nov 23, 2017 Author Member

This comment has been minimized.

rfk Nov 23, 2017

This comment has been minimized.

Natim Nov 23, 2017 Author Member

This comment has been minimized.

rfk Nov 23, 2017

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

This comment has been minimized.

leplatrem Nov 24, 2017 Contributor

Natim commented Nov 27, 2017 • edited

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

Natim Nov 23, 2017
Author Member

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

Natim Nov 23, 2017
Author Member

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

leplatrem Nov 22, 2017
Contributor

Natim Nov 23, 2017
Author Member

Natim Nov 23, 2017
Author Member

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

leplatrem Nov 24, 2017
Contributor

Natim commented Nov 27, 2017 •

edited