-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TNL-782 each request should have valid ID Token #2
Conversation
dea3904
to
314f057
Compare
425704f
to
b75f00f
Compare
@jimabramson @olmar @polesye please take a look |
CREATE_FILTER_FIELDS = ('updated', 'created', 'consumer', 'id') | ||
UPDATE_FILTER_FIELDS = ('updated', 'created', 'user', 'consumer') | ||
|
||
|
||
@permission_classes((HasAccessToken,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set it globally? http://www.django-rest-framework.org/api-guide/permissions/#setting-the-permission-policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, that is nice. done aa3c503
raise TokenWrongIssuer | ||
for request_field in ('GET', 'POST', 'DATA'): | ||
if 'user' in getattr(request, request_field): | ||
req_user = getattr(request, request_field)['user'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to move this code in helper method:
def get_user(self, request):
for request_field in ('GET', 'POST', 'DATA'):
data = getattr(request, request_field)
if 'user' in data:
return data['user']
return None
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would requre returning from get_user
not unly user
but request_field
where it was found, for better error log if it does not match. IMHO not worth it.
@@ -1,4 +1,5 @@ | |||
PACKAGES = notesserver notesapi | |||
.PHONY: requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows make requirement
to work, otherwise it will just stop seeing requirements
directory exists.
@tymofij don't forget to add yourself to AUTHORS |
6c63629
to
7ebd7d4
Compare
👍 once previously mentioned commnents will be addressed |
Each incoming request is expected to have signed non-expired JSON Web Token, containing information about user (sub), matching edx-notes client id (to be set in provider's database) and signed with edx-notes client secret (the same).
https://openedx.atlassian.net/browse/TNL-782