Access Token middleware #14
Conversation
README.rst
Outdated
| ------------------------- | ||
|
|
||
| General | ||
| ~~~~~~~ |
There was a problem hiding this comment.
In m'n proefschrift kon ik toe met 3 header levels. Da's toch niet nodig in een readme? Max twee.
Liever h2 "general usage", "view", enz.
Anders lijkt het op de 3di docs waar de adviseurs absoluut niet zonder vijf heading levels kunnen geloof ik...
README.rst
Outdated
|
|
||
| The nens-auth-client library exposes one django application: ``nens_auth_client``. | ||
| The django built-in apps ``auth``, ``sessions`` and ``contenttypes`` are | ||
| also required, but they probably are already there. | ||
| Add these to the ``INSTALLED_APPS`` setting: | ||
| Add these to the ``INSTALLED_APPS`` setting:: |
There was a problem hiding this comment.
Kan er nog iets over de volgorde bij? Da's de standaard vraag die ik me altijd bij dit soort settings stel. Of er iets specifiek voor/na een ander moet.
There was a problem hiding this comment.
Order is alleen belangrijk bij het overriden van een template / translation / mgmt command:
https://stackoverflow.com/questions/31925458/importance-of-apps-orders-in-installed-apps
| # Do something only if there is a Bearer token and there is no user. | ||
| if not (token and request.user.is_anonymous): | ||
| return self.get_response(request) |
There was a problem hiding this comment.
Dit klopt met de docstring. Bearer token wordt genegeerd als er al een reguliere gebruiker is.
Is het niet iets dat op een mogelijk probleem wijst? Als een ingelogde gebruiker OOK nog eens een bearer token heeft? Even loggen zou het uitzoeken van problemen kunnen verhelpen.
| # Do something only if there is a Bearer token and there is no user. | |
| if not (token and request.user.is_anonymous): | |
| return self.get_response(request) | |
| # Do something only if there is a Bearer token and there is no user. | |
| if not token: | |
| return self.get_response(request) | |
| if not request.user.is_anonymous): | |
| logger.warn("Aargh, bearer token maar toch ingelogd!!!") | |
| return self.get_response(request) |
There was a problem hiding this comment.
Dit zie ik wel als een user error, en niet een voor developers. Bij Lizard's setup krijg je nu een sentry melding om je oren.
In de RFC voor bearer tokens lees ik dat je een 400 zou moeten geven in dit geval (2 authenticatie methodes in 1):
invalid_request
The request is missing a required parameter, includes an
unsupported parameter or parameter value, repeats the same
parameter, uses more than one method for including an access
token, or is otherwise malformed. The resource server SHOULD
respond with the HTTP 400 (Bad Request) status code.
Misschien is dat het duidelijkst voor iedereen? Anders krijg je misschien situaties waarin het onduidelijk is waarom een Bearer token wordt genegeerd. "Oooh ik had nog een session cookie"
nens_auth_client/oauth.py
Outdated
| from django.conf import settings | ||
| from django.utils.module_loading import import_string | ||
|
|
||
| import requests | ||
|
|
||
|
|
||
| # Create the global OAuth registry | ||
| oauth = OAuth() |
There was a problem hiding this comment.
Caps? OAUTH_REGISTRY? In hoeverre is het privé in dit bestand?
There was a problem hiding this comment.
Caps is geloof ik alleen voor echt constantes. Hij wordt hier en daar geimporteerd om de cognito client te krijgen en de OpenID Connect flow te doen (zie views.py).
Zal ik er oauth_registry van maken?
|
@reinout ik heb al je punten bekeken. Een open issue is nog: wat te doen als er een session cookie en een bearer token is? Zie mijn commentaar boven. |
|
Ik laat het open punt even over aan #15 |
See the issue description in #11
This appeared to be a lot harder than I anticipated. authlib only has some low-level JWT methods to validate access tokens, so I had to write part of the validation myself (unlike with the ID Tokens in the
authorizeview)On top of that, AWS Cognito does not adhere to the RFC for Access Tokens. I decided to stick to the RFC in the implementation, but add a preprocessor_access_token function that adapts the access token so that it conforms to the RFC.
There is some duplicate logic in the middleware and in the authorize view. I didn't want to touch that because the diff of this PR is already pretty large.