-
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
Changes from 64 commits
e237e94
96f8e2d
95765d5
6e9d2a7
0029c07
8b380ad
06c069d
988a3e4
97d8ede
d9cfbc6
0d7e901
693a928
c475d4f
a849820
c70bd5d
df5f511
0ef2b13
782ed10
0f77d84
b2bb6ca
78a1e92
6a032e8
976de54
f977dfe
3dd0846
b834963
a8aaab7
26854cd
d97c1ab
e83952d
f467f74
44e717b
296d3a0
03aa08c
97bc56c
4226868
a481a49
310973e
7464dc8
c89ad45
ed62505
6884926
ccf6a6c
d9f39f8
ce8abdb
5d6d09f
3f736ef
9dc382f
fdb3abe
e7254a6
70d57d9
943234b
75cad0b
6a2ed52
37024de
9bbf32e
e0eacd9
ee1dec2
440e47f
b75f00f
acce93e
aa3c503
164535b
54091d2
e0c80ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[run] | ||
omit = notesserver/settings* | ||
*wsgi.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Python artifacts | ||
*.pyc | ||
|
||
# Tests / Coverage reports | ||
.coverage | ||
.tox | ||
coverage/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[pep8] | ||
ignore=E501 | ||
max_line_length=119 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
language: python | ||
python: 2.7 | ||
|
||
services: | ||
- elasticsearch | ||
|
||
install: | ||
- pip install -r requirements/test.txt | ||
- git fetch origin master:refs/remotes/origin/master # https://github.com/edx/diff-cover#troubleshooting | ||
- pip install coveralls | ||
|
||
script: | ||
- make validate | ||
|
||
after_success: | ||
- coveralls |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Oleg Marshev <oleg@edx.org> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
How To Contribute | ||
================= | ||
|
||
Contributions are very welcome. | ||
|
||
Please read `How To Contribute <https://github.com/edx/edx-platform/blob/master/CONTRIBUTING.rst>`_ for details. | ||
|
||
Even though it was written with ``edx-platform`` in mind, the guidelines | ||
should be followed for Open edX code in general. |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
PACKAGES = notesserver notesapi | ||
.PHONY: requirements | ||
|
||
validate: test.requirements test coverage | ||
|
||
test: clean | ||
./manage.py test --settings=notesserver.settings.test --with-coverage --with-ignore-docstrings \ | ||
--exclude-dir=notesserver/settings --cover-inclusive --cover-branches \ | ||
--cover-html --cover-html-dir=build/coverage/html/ \ | ||
--cover-xml --cover-xml-file=build/coverage/coverage.xml \ | ||
$(foreach package,$(PACKAGES),--cover-package=$(package)) \ | ||
$(PACKAGES) | ||
|
||
run: | ||
./manage.py runserver 0.0.0.0:8042 | ||
|
||
shell: | ||
./manage.py shell | ||
|
||
clean: | ||
coverage erase | ||
|
||
quality: | ||
pep8 --config=.pep8 $(PACKAGES) | ||
pylint $(PACKAGES) | ||
|
||
diff-coverage: | ||
diff-cover build/coverage/coverage.xml --html-report build/coverage/diff_cover.html | ||
|
||
diff-quality: | ||
diff-quality --violations=pep8 --html-report build/coverage/diff_quality_pep8.html | ||
diff-quality --violations=pylint --html-report build/coverage/diff_quality_pylint.html | ||
|
||
coverage: diff-coverage diff-quality | ||
|
||
create-index: | ||
python manage.py create_index | ||
|
||
requirements: | ||
pip install -q -r requirements/base.txt --exists-action=w | ||
|
||
test.requirements: requirements | ||
pip install -q -r requirements/test.txt --exists-action=w | ||
|
||
develop: test.requirements | ||
pip install -q -r requirements/local.txt --exists-action=w |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
Part of `edX code`__. | ||
|
||
__ http://code.edx.org/ | ||
|
||
edX Student Notes API |build-status| |coverage-status| | ||
====================================================== | ||
|
||
This is a backend store for edX Student Notes. | ||
|
||
Overview | ||
-------- | ||
|
||
The edX Notes API is designed to be compatible with the | ||
`Annotator <http://annotatorjs.org/>`__. | ||
|
||
Getting Started | ||
--------------- | ||
|
||
1. You'll need a recent version `ElasticSearch <http://elasticsearch.org>`__ (>=1.0.0) | ||
installed. | ||
|
||
2. Install the requirements: | ||
|
||
:: | ||
|
||
$ make develop | ||
|
||
3. Create index and put mapping: | ||
|
||
:: | ||
|
||
$ make create-index | ||
|
||
4. Run the server: | ||
|
||
:: | ||
|
||
$ make run | ||
|
||
Running Tests | ||
------------- | ||
|
||
Run ``make validate`` install the requirements, run the tests, and run | ||
lint. | ||
|
||
License | ||
------- | ||
|
||
The code in this repository is licensed under version 3 of the AGPL unless | ||
otherwise noted. | ||
|
||
Please see ``LICENSE.txt`` for details. | ||
|
||
How To Contribute | ||
----------------- | ||
|
||
Contributions are very welcome. | ||
|
||
Please read `How To Contribute <https://github.com/edx/edx-platform/blob/master/CONTRIBUTING.rst>`_ for details. | ||
|
||
Even though it was written with ``edx-platform`` in mind, the guidelines | ||
should be followed for Open edX code in general. | ||
|
||
Reporting Security Issues | ||
------------------------- | ||
|
||
Please do not report security issues in public. Please email security@edx.org | ||
|
||
Mailing List and IRC Channel | ||
---------------------------- | ||
|
||
You can discuss this code on the `edx-code Google Group`__ or in the | ||
``edx-code`` IRC channel on Freenode. | ||
|
||
__ https://groups.google.com/forum/#!forum/edx-code | ||
|
||
.. |build-status| image:: https://travis-ci.org/edx/edx-notes-api.svg?branch=master | ||
:target: https://travis-ci.org/edx/edx-notes-api | ||
.. |coverage-status| image:: https://coveralls.io/repos/edx/edx-notes-api/badge.png?branch=master | ||
:target: https://coveralls.io/r/edx/edx-notes-api?branch=master |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
#!/usr/bin/env python | ||
import os | ||
import sys | ||
|
||
if __name__ == "__main__": | ||
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "notesserver.settings.dev") | ||
|
||
from django.core.management import execute_from_command_line | ||
|
||
execute_from_command_line(sys.argv) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
from django.core.management.base import BaseCommand | ||
from annotator.annotation import Annotation | ||
|
||
|
||
class Command(BaseCommand): | ||
help = 'Creates the mapping in the index.' | ||
|
||
def handle(self, *args, **options): | ||
Annotation.create_all() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from django.conf.urls import patterns, url, include | ||
|
||
urlpatterns = patterns( | ||
'', | ||
url(r'^v1/', include('notesapi.v1.urls', namespace='v1')), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
import jwt | ||
import logging | ||
from django.conf import settings | ||
from rest_framework.permissions import BasePermission | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class TokenWrongIssuer(Exception): | ||
pass | ||
|
||
|
||
class HasAccessToken(BasePermission): | ||
""" | ||
Allow requests having valid ID Token. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also leave a comment about raw token format? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
|
||
https://tools.ietf.org/html/draft-ietf-oauth-json-web-token-31 | ||
Expected Token: | ||
Header { | ||
"alg": "HS256", | ||
"typ": "JWT" | ||
} | ||
Claims { | ||
"sub": "<USER_ID>", | ||
"exp": <EXPIRATION TIMESTAMP>, | ||
"iat": <ISSUED TIMESTAMP>, | ||
"aud": "<CLIENT ID" | ||
} | ||
Should be signed with CLIENT_SECRET | ||
""" | ||
def has_permission(self, request, view): | ||
if getattr(settings, 'DISABLE_TOKEN_CHECK', False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we should add http://www.django-rest-framework.org/api-guide/authentication/#custom-authentication and use default https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/permissions.py#L41 then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this app does not use |
||
return True | ||
token = request.META.get('HTTP_X_ANNOTATOR_AUTH_TOKEN', '') | ||
if not token: | ||
logger.debug("No token found in headers") | ||
return False | ||
try: | ||
data = jwt.decode(token, settings.CLIENT_SECRET) | ||
auth_user = data['sub'] | ||
if data['aud'] != settings.CLIENT_ID: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to raise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think so. They are expected to be always present, and their default values are good for developing the application. |
||
raise TokenWrongIssuer | ||
for request_field in ('GET', 'POST', 'DATA'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why just do not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do not use |
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. that would requre returning from |
||
if req_user == auth_user: | ||
return True | ||
else: | ||
logger.debug("Token user {auth_user} did not match {field} user {req_user}".format( | ||
auth_user=auth_user, field=request_field, req_user=req_user | ||
)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
logger.info("No user was present to compare in GET, POST or DATA") | ||
except jwt.ExpiredSignature: | ||
logger.exception("Token was expired: {}".format(token)) | ||
except jwt.DecodeError: | ||
logger.exception("Could not decode token {}".format(token)) | ||
except TokenWrongIssuer: | ||
logger.exception("Token has wrong issuer {}".format(token)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if these cases need to be logged at all, i'm not sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my line of thinking was that cases of wrong issue or bad token might be more attention-worthy than just user mismatch, but on a second thought they are not. changing to |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import jwt | ||
from calendar import timegm | ||
from datetime import datetime, timedelta | ||
from django.conf import settings | ||
|
||
|
||
def get_id_token(user): | ||
now = datetime.utcnow() | ||
return jwt.encode({ | ||
'aud': settings.CLIENT_ID, | ||
'sub': user, | ||
'iat': timegm(now.utctimetuple()), | ||
'exp': timegm((now + timedelta(seconds=300)).utctimetuple()), | ||
}, settings.CLIENT_SECRET) |
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 seeingrequirements
directory exists.