Token auth for WebSocket API #4334
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A later commit in this branch will introduce fetching of an access token from a given query string parameter. To support fetching both the default token (from the `Authorization` header), and from a given query string parameter, we have to refactor this `request.auth_token` property to be a function call. Since the calling code will be able to specify the key of the query string parameter, and within the same request we might even want to get different auth tokens. This means that we can't just use the pyramid `reify` functionality for request properties anymore. To prevent us to load the same token from the database multiple times within a request this commit includes a new `AuthTokenFetcher` which is a callable object that takes care of caching the fetched token internally.
This authentication policy only checks for the token in the `access_token` query string parameter. To achieve this the `AuthTokenFetcher` will accept an optional argument which is the key of the query string parameter.
This was referenced Jan 27, 2017
nickstenning
suggested changes
Jan 30, 2017
There was a problem hiding this comment.
This looks good, and it works fine, but I think adding a separate TokenParameterAuthenticationPolicy has overcomplicated things a little.
request.auth_token exists so that TokenAuthenticationPolicy doesn't need to know where the token actually comes from, so it should be reusable without modification. You could just add a single conditional to h.auth.tokens.auth_token to fetch the token from the parameter if the request is for the websocket endpoint:
def auth_token(request):
if request.path == '/ws':
token = request.params.get('access_token')
if token is None:
token = _token_from_header(request)
...Wouldn't that be simpler?
The audience check was a protection to ensure that we don't use a token destined for someone else, but we only ever generate these JWT tokens for ourselves. So there is no need for checking the audience anymore. This should not compromise security, because as long as the server-side client secret is not compromised, there is no way to tamper with the JWT token. This will allow us to use the legacy JWT tokens for authenticating websocket connections when the web service and the websocket service run on different hosts (in localhost we have: `http://localhost:5000` for the web service, and `http://localhost:5001` for the websocket service).
Codecov Report@@ Coverage Diff @@
## master #4334 +/- ##
==========================================
+ Coverage 93.83% 93.86% +0.03%
==========================================
Files 311 311
Lines 17119 17286 +167
Branches 1019 1041 +22
==========================================
+ Hits 16064 16226 +162
- Misses 953 954 +1
- Partials 102 106 +4
Continue to review full report at Codecov.
|
|
I have tested this branch with hypothesis/client#200 and this diff to disable the fallback to cookie auth: diff --git a/h/auth/__init__.py b/h/auth/__init__.py
index ecbab58..230e28a 100644
--- a/h/auth/__init__.py
+++ b/h/auth/__init__.py
@@ -27,7 +27,7 @@ TOKEN_PARAM_POLICY = TokenParameterAuthenticationPolicy(callback=groupfinder, de
DEFAULT_POLICY = AuthenticationPolicy(api_policy=TOKEN_POLICY,
fallback_policy=TICKET_POLICY)
-WEBSOCKET_POLICY = MultiAuthenticationPolicy([TOKEN_PARAM_POLICY, TOKEN_POLICY, TICKET_POLICY])
+WEBSOCKET_POLICY = MultiAuthenticationPolicy([TOKEN_PARAM_POLICY])This now works as expected. |
|
Closing in favour of #4345 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will add support for authenticating websocket connections with a query string parameter, e.g.
wss://hypothes.is/ws?access_token={token}. This token can be a new OAuth 2 token, a developer token, or a legacy client JWT token.After some more research, @robertknight found out that HTTP basic auth support for WebSockets is not fully supported by all browsers, so this supersedes #4330.
I had to move quite a bit of things around for a nice implementation of this, as we can't import any models in the Python files that contain the authentication policies.
I suggest @robertknight should try out his client branch with this, and since there are quite a lot of authz related changes it would be great to get at least @nickstenning's eyes on this, others are welcome as well.