Skip to content
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

Closed
wants to merge 65 commits into from

Conversation

tymofij
Copy link
Contributor

@tymofij tymofij commented Dec 2, 2014

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

@coveralls
Copy link

Coverage Status

Coverage increased (+1.98%) when pulling b75f00f on tim/id-token-auth into 440e47f on oleg/create-app.

@tymofij
Copy link
Contributor Author

tymofij commented Dec 2, 2014

@jimabramson @olmar @polesye please take a look
cc @jzoldak @benpatterson

CREATE_FILTER_FIELDS = ('updated', 'created', 'consumer', 'id')
UPDATE_FILTER_FIELDS = ('updated', 'created', 'user', 'consumer')


@permission_classes((HasAccessToken,))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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']
Copy link

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?

Copy link
Contributor Author

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.

@tymofij tymofij changed the title each request should have valid ID Token TNL-782 each request should have valid ID Token Dec 3, 2014
@tymofij tymofij mentioned this pull request Dec 3, 2014
@@ -1,4 +1,5 @@
PACKAGES = notesserver notesapi
.PHONY: requirements

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

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.

@jimabramson
Copy link

@tymofij don't forget to add yourself to AUTHORS

@olmar
Copy link
Contributor

olmar commented Dec 4, 2014

👍 once previously mentioned commnents will be addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants