From f9d22cc56d2a22b6e70029c3e470e5a82df686c0 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Sun, 27 Aug 2023 22:53:50 +0300 Subject: [PATCH 1/6] Added configuration needed for Django to successfully build absolute URIs in the dase of the app begin behind a proxy. --- teknologr/.env.example | 6 ++++++ teknologr/teknologr/settings.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/teknologr/.env.example b/teknologr/.env.example index 3f3d16d7..b82700e5 100644 --- a/teknologr/.env.example +++ b/teknologr/.env.example @@ -1,5 +1,11 @@ # This file contains configuration and settings variables that should be different in production and development environments. +# The host name used for teknologr.io, for example teknologr.mysite.com +TEKNOLOGR_HOST= + +# Whether or not requests to teknologr.io are forwarded by a proxy or not +IS_BEHIND_PROXY=True + # Secret key, change it maybe? SECRET_KEY=SomeRandomSecretKey diff --git a/teknologr/teknologr/settings.py b/teknologr/teknologr/settings.py index d86eb634..9b9356db 100644 --- a/teknologr/teknologr/settings.py +++ b/teknologr/teknologr/settings.py @@ -49,7 +49,24 @@ # SECURITY WARNING: don't run with debug turned on in production! DEBUG = env('DEBUG', True) + +''' +If a proxy (or similar) is used to handle requests to Teknologr, the request can be modified in some ways which can have unwanted side-effects in Django. + +One such problem occurs when an absoule uri (to for example /api/members/) is being built by Django, because the host name is taken from a request. If that request have been forwarded by some proxy it's 'HTTP_HOST' header will no longer be the orignal host name (let's say teknologr.com) but instead for example localhost:8000. The resulting uri is then 'http://localhost:8000/api/members/' while the correct uri is 'https://teknolog.com/api/members/'. + +The solution is to instead read the host name from the 'X-Forwarded-Host' (https://http.dev/x-forwarded-host) header in the request, which should be set by the proxy to the target host, as specified by the reqeust originator, if the request is forwarded. This is done in Django using the USE_X_FORWARDED_HOST setting. + +However, this will only resolve the problem with the host name, not the protocol part of the uri. Since the forwarded request can be done with HTTP, even if the original request was made with HTTPS, Django migth incorrectly deduce that a request is not secure even if the orignal request was (or vice verca). Using the SECURE_PROXY_SSL_HEADER setting Django will look a the specified header (usually the 'X-Forwarded-Proto' header: https://http.dev/x-forwarded-proto) rather than the request url to deduce if a request is secure or not. Note that the header needs to be in the format as used by request.META, i.e. all caps and (most likely) prefixed with 'HTTP_'. +''' ALLOWED_HOSTS = ['localhost'] +TEKNOLOGR_HOST = env('TEKNOLOGR_HOST', None) +if TEKNOLOGR_HOST: + ALLOWED_HOSTS.append(TEKNOLOGR_HOST) + +if env('IS_BEHIND_PROXY', False): + USE_X_FORWARDED_HOST = True + SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https') # Application definition From 4d9005cfe47bc42f8a0715f41db07907c5aa2c03 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Mon, 28 Aug 2023 00:24:32 +0300 Subject: [PATCH 2/6] Changed the API root view to only show enpoints that are accessible to the user. Also added the other named endpoints to the list. --- teknologr/api/tests.py | 4 ++-- teknologr/api/urls.py | 43 +++++++++++++++++++++++++++++++++++++----- teknologr/api/views.py | 4 ++-- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/teknologr/api/tests.py b/teknologr/api/tests.py index 5e53cad5..d30ea368 100644 --- a/teknologr/api/tests.py +++ b/teknologr/api/tests.py @@ -693,10 +693,10 @@ def test_get_for_user(self): self.login_user() response = self.get_all() self.check_status_code(response, status.HTTP_200_OK) - self.assertEqual(10, len(response.json())) + self.assertEqual(8, len(response.json())) def test_get_for_superuser(self): self.login_superuser() response = self.get_all() self.check_status_code(response, status.HTTP_200_OK) - self.assertEqual(10, len(response.json())) + self.assertEqual(16, len(response.json())) diff --git a/teknologr/api/urls.py b/teknologr/api/urls.py index 414b0005..87d884e4 100644 --- a/teknologr/api/urls.py +++ b/teknologr/api/urls.py @@ -1,16 +1,49 @@ from django.conf.urls import url, include -from rest_framework import routers, permissions +from rest_framework.routers import APIRootView, DefaultRouter +from rest_framework.permissions import IsAuthenticated, IsAdminUser from api.views import * -class RootView(routers.APIRootView): +class TeknologrRootView(APIRootView): permission_classes = (permissions.IsAuthenticated, ) name = 'Katalogen root API' description = '' + router_list_name = None + router_registry = None + + @property + def api_root_dict(self): + ''' + Overriding the parent attribute to be able to filter views based on permissions. + + This is accessed by the get() method and is by default provided by the router instead. + ''' + d = {} + is_staff = self.request.user.is_staff + for prefix, viewset, basename in self.router_registry: + # Include the route unless it is staff only + if not is_staff and permissions.IsAdminUser in viewset.permission_classes: + continue + d[prefix] = self.router_list_name.format(basename=basename) + + # Also add the other named API endpoints to the list (mainly the dumps) + # XXX: Is there a way to check the permissions agains the user for these too? + if is_staff: + for url in urlpatterns: + if hasattr(url, 'name') and url.name: + d[url.name] = url.name + + return d + +class TeknologrRouter(DefaultRouter): + APIRootView = TeknologrRootView + + def get_api_root_view(self, api_urls=None): + return self.APIRootView.as_view(router_list_name=self.routes[0].name, router_registry=self.registry) + # Routers provide an easy way of automatically determining the URL conf. # NOTE: Use for example {% url 'api:member-list' %} to access these urls -router = routers.DefaultRouter() -router.APIRootView = RootView +router = TeknologrRouter() router.register(r'members', MemberViewSet) router.register(r'grouptypes', GroupTypeViewSet) router.register(r'groups', GroupViewSet) @@ -30,7 +63,7 @@ class RootView(routers.APIRootView): url(r'^multi-groupmemberships/$', multi_group_memberships_save), url(r'^multi-functionaries/$', multi_functionaries_save), url(r'^multi-decorationownerships/$', multi_decoration_ownerships_save), - url(r'^multi-applicantsubmissions/$', multi_applicant_submissions, name='multi_applicant_submissions'), + url(r'^multi-applicantsubmissions/$', multi_applicant_submissions), url(r'^accounts/ldap/(\d+)/$', LDAPAccountView.as_view()), url(r'^accounts/ldap/change_pw/(\d+)/$', change_ldap_password), url(r'^accounts/bill/(\d+)/$', BILLAccountView.as_view()), diff --git a/teknologr/api/views.py b/teknologr/api/views.py index 3fba193f..a7de42e3 100644 --- a/teknologr/api/views.py +++ b/teknologr/api/views.py @@ -24,7 +24,7 @@ # ViewSets define the view behavior. -class APIPermissions(permissions.BasePermission): +class IsStaffOrReadOnly(permissions.BasePermission): def has_permission(self, request, view): # Do not allow anything for un-authenticated users if not request.user.is_authenticated: @@ -39,7 +39,7 @@ def has_permission(self, request, view): class BaseModelViewSet(viewsets.ModelViewSet): # Use custom permissions - permission_classes = (APIPermissions, ) + permission_classes = (IsStaffOrReadOnly, ) def get_serializer(self, *args, **kwargs): serializer_class = self.get_serializer_class() From ad79bb7c3f7da8d5c8a34dd8aa9fa608faa58d13 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Mon, 28 Aug 2023 00:52:04 +0300 Subject: [PATCH 3/6] Fixed order of 'try' and 'with LDAPAccountManager()'. This resolves issue #174. --- teknologr/api/views.py | 52 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/teknologr/api/views.py b/teknologr/api/views.py index a7de42e3..4f189017 100644 --- a/teknologr/api/views.py +++ b/teknologr/api/views.py @@ -260,11 +260,11 @@ class LDAPAccountView(APIView): def get(self, request, member_id): member = get_object_or_404(Member, id=member_id) result = {} - with LDAPAccountManager() as lm: - try: + try: + with LDAPAccountManager() as lm: result = {'username': member.username, 'groups': lm.get_ldap_groups(member.username)} - except LDAPError as e: - return Response(str(e), status=400) + except LDAPError as e: + return Response(str(e), status=400) return Response(result, status=200) @@ -280,11 +280,11 @@ def post(self, request, member_id): if member.username: return Response("Member already has LDAP account", status=400) - with LDAPAccountManager() as lm: - try: + try: + with LDAPAccountManager() as lm: lm.add_account(member, username, password) - except LDAPError as e: - return Response(str(e), status=400) + except LDAPError as e: + return Response(str(e), status=400) # Store account details member.username = username @@ -306,11 +306,11 @@ def delete(self, request, member_id): if member.bill_code: return Response("BILL account must be deleted first", status=400) - with LDAPAccountManager() as lm: - try: + try: + with LDAPAccountManager() as lm: lm.delete_account(member.username) - except LDAPError as e: - return Response(str(e), status=400) + except LDAPError as e: + return Response(str(e), status=400) # Delete account information from user in db member.username = None @@ -327,15 +327,15 @@ def change_ldap_password(request, member_id): if not password: return Response("password field missing", status=400) - with LDAPAccountManager() as lm: - try: + try: + with LDAPAccountManager() as lm: lm.change_password(member.username, password) if mailToUser: status = mailNewPassword(member, password) if not status: return Response("Password changed, failed to send mail", status=500) - except LDAPError as e: - return Response(str(e), status=400) + except LDAPError as e: + return Response(str(e), status=400) return Response(status=200) @@ -464,8 +464,8 @@ def post(self, request, applicant_id): # Create an LDAP account if the application included a username and the username is not taken if username and not Member.objects.filter(username=username).exists(): - with LDAPAccountManager() as lm: - try: + try: + with LDAPAccountManager() as lm: import secrets password = secrets.token_urlsafe(16) lm.add_account(new_member, username, password) @@ -478,14 +478,14 @@ def post(self, request, applicant_id): if not status: return Response(f'LDAP account created, failed to send mail to {new_member}', status=400) - # LDAP account creation failed (e.g. if the account already exists) - except LDAPError as e: - return Response(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) - # Updating the username field failed, remove the created LDAP account - # as it is not currently referenced by any member. - except IntegrityError as e: - lm.delete_account(username) - return Response(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) + # LDAP account creation failed (e.g. if the account already exists) + except LDAPError as e: + return Response(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) + # Updating the username field failed, remove the created LDAP account + # as it is not currently referenced by any member. + except IntegrityError as e: + lm.delete_account(username) + return Response(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) return Response(status=200) From 8243243c2a4dc80a9281842481f568d61b237a77 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Mon, 28 Aug 2023 01:26:06 +0300 Subject: [PATCH 4/6] Override rest_framework/api.html template to include the correct path to favicon.ico. This resolves issue #175. --- teknologr/api/templates/rest_framework/api.html | 8 ++++++++ teknologr/teknologr/settings.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 teknologr/api/templates/rest_framework/api.html diff --git a/teknologr/api/templates/rest_framework/api.html b/teknologr/api/templates/rest_framework/api.html new file mode 100644 index 00000000..33c0e7fb --- /dev/null +++ b/teknologr/api/templates/rest_framework/api.html @@ -0,0 +1,8 @@ +{% extends "rest_framework/base.html" %} + +{% load static %} + +{% block style %} +{{ block.super }} + +{% endblock %} diff --git a/teknologr/teknologr/settings.py b/teknologr/teknologr/settings.py index 9b9356db..ab60bb8c 100644 --- a/teknologr/teknologr/settings.py +++ b/teknologr/teknologr/settings.py @@ -102,7 +102,7 @@ TEMPLATES = [ { 'BACKEND': 'django.template.backends.django.DjangoTemplates', - 'DIRS': [], + 'DIRS': [os.path.join(BASE_DIR, 'api/templates')], 'APP_DIRS': True, 'OPTIONS': { 'context_processors': [ From ea1aab8c6746283bae6f72367841d9fec98f3742 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Mon, 28 Aug 2023 01:52:48 +0300 Subject: [PATCH 5/6] Fixed HTK dump url pattern to allow ending backslash. --- teknologr/api/tests_dumps.py | 13 ++++++++++++- teknologr/api/urls.py | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/teknologr/api/tests_dumps.py b/teknologr/api/tests_dumps.py index 441a04de..b2af1b02 100644 --- a/teknologr/api/tests_dumps.py +++ b/teknologr/api/tests_dumps.py @@ -67,7 +67,7 @@ def test_get_for_superuser(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(self.response, response.json()) -class HTK(BaseClass, DumpsTestCases): +class HTK_Full(BaseClass, DumpsTestCases): path = f'/api/dump-htk/' response = [{ 'id': 1, @@ -78,6 +78,17 @@ class HTK(BaseClass, DumpsTestCases): 'decorations': ['Hedersmedlem: 1999-01-01'], }] +class HTK_One(BaseClass, DumpsTestCases): + path = f'/api/dump-htk/1/' + response = { + 'id': 1, + 'name': 'Sverker Svakar von Teknolog', + 'functionaries': [f'Funkkis: {today} > 2999-01-01'], + 'groups': [], + 'membertypes': ['Ordinarie Medlem: 1999-01-01 > None'], + 'decorations': ['Hedersmedlem: 1999-01-01'], + } + class Modulen(BaseClass, DumpsTestCases): path = f'/api/dump-modulen/' response = [{ diff --git a/teknologr/api/urls.py b/teknologr/api/urls.py index 87d884e4..3adfc7f3 100644 --- a/teknologr/api/urls.py +++ b/teknologr/api/urls.py @@ -68,7 +68,7 @@ def get_api_root_view(self, api_urls=None): url(r'^accounts/ldap/change_pw/(\d+)/$', change_ldap_password), url(r'^accounts/bill/(\d+)/$', BILLAccountView.as_view()), url(r'^applicants/make-member/(\d+)/$', ApplicantMembershipView.as_view()), - url(r'^dump-htk/(\d+)?$', dump_htk, name='dump_htk'), + url(r'^dump-htk/(?:(\d+)/)?$', dump_htk, name='dump_htk'), url(r'^dump-modulen/$', dump_modulen, name='dump_modulen'), url(r'^dump-active/$', dump_active, name='dump_active'), url(r'^dump-arsk/$', dump_arsk, name='dump_arsk'), From 2c84c24c7247235bba1b7c7d73d86e88014285a2 Mon Sep 17 00:00:00 2001 From: Filip Stenbacka Date: Mon, 28 Aug 2023 02:07:05 +0300 Subject: [PATCH 6/6] Minor fixes. --- ...ests_generikey.py => tests_members_by_mt.py} | 6 +++--- .../{tests_bill.py => tests_mt_by_member.py} | 14 +++++++------- teknologr/api/urls.py | 10 +++++----- teknologr/api/views.py | 17 +++++++---------- 4 files changed, 22 insertions(+), 25 deletions(-) rename teknologr/api/{tests_generikey.py => tests_members_by_mt.py} (95%) rename teknologr/api/{tests_bill.py => tests_mt_by_member.py} (87%) diff --git a/teknologr/api/tests_generikey.py b/teknologr/api/tests_members_by_mt.py similarity index 95% rename from teknologr/api/tests_generikey.py rename to teknologr/api/tests_members_by_mt.py index f674f988..30c8827f 100644 --- a/teknologr/api/tests_generikey.py +++ b/teknologr/api/tests_members_by_mt.py @@ -45,7 +45,7 @@ def setUp(self): def login_superuser(self): self.client.login(username='superuser', password='teknolog') -class GenerikeyTestCases(): +class TestCases(): def test_get_for_anonymous_users(self): response = self.get('PH') self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) @@ -91,7 +91,7 @@ def test_get_double(self): self.assertEqual(response.json(), self.double) -class GenerikeyStudynumbersTestCases(BaseClass, GenerikeyTestCases): +class StudynumbersByMTTests(BaseClass, TestCases): normal = ['123456', '654321'] null = ['123456', None] double = ['654321', None] @@ -100,7 +100,7 @@ def get(self, type): return self.client.get(f'/api/membersByMemberType/{type}/') -class GenerikeyUsernamesTestCases(BaseClass, GenerikeyTestCases): +class UsernamesByMTTests(BaseClass, TestCases): normal = ['vonteks1', 'vonteks2'] null = ['vonteks1', None] double = ['vonteks2', None] diff --git a/teknologr/api/tests_bill.py b/teknologr/api/tests_mt_by_member.py similarity index 87% rename from teknologr/api/tests_bill.py rename to teknologr/api/tests_mt_by_member.py index 0e80939f..fbe7927b 100644 --- a/teknologr/api/tests_bill.py +++ b/teknologr/api/tests_mt_by_member.py @@ -29,7 +29,7 @@ def login_superuser(self): self.client.login(username='superuser', password='teknolog') -class BILLTestCases: +class TestCases: def test_get_for_anonymous_users(self): response = self.get() self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) @@ -48,11 +48,11 @@ def test_get_for_superuser(self): else: self.assertEqual(self.response, response.json()) -class BILLByUsernameTestCases(BILLTestCases): +class ByUsernameTestCases(TestCases): def get(self): return self.client.get(f'/api/memberTypesForMember/username/{self.username}/') -class BILLByStudynumberTestCases(BILLTestCases): +class ByStudynumberTestCases(TestCases): def get(self): return self.client.get(f'/api/memberTypesForMember/studynumber/{self.studynumber}/') @@ -77,26 +77,26 @@ def get(self): } } -class BILLByInvalidUsernameTests(BaseClass, BILLByUsernameTestCases): +class ByInvalidUsernameTests(BaseClass, ByUsernameTestCases): def setUp(self): super().setUp() self.username = 'invalid' self.response = None -class BILLByValidUsernameTests(BaseClass, BILLByUsernameTestCases): +class ByValidUsernameTests(BaseClass, ByUsernameTestCases): def setUp(self): super().setUp() self.username = self.member.username self.response = RESPONSE -class BILLByInvalidStudynumberTests(BaseClass, BILLByStudynumberTestCases): +class ByInvalidStudynumberTests(BaseClass, ByStudynumberTestCases): def setUp(self): super().setUp() self.studynumber = '123321' self.response = None -class BILLByValidStudynumberTests(BaseClass, BILLByStudynumberTestCases): +class ByValidStudynumberTests(BaseClass, ByStudynumberTestCases): def setUp(self): super().setUp() self.studynumber = self.member.student_id diff --git a/teknologr/api/urls.py b/teknologr/api/urls.py index 3adfc7f3..b64ca5fd 100644 --- a/teknologr/api/urls.py +++ b/teknologr/api/urls.py @@ -4,7 +4,7 @@ from api.views import * class TeknologrRootView(APIRootView): - permission_classes = (permissions.IsAuthenticated, ) + permission_classes = (IsAuthenticated, ) name = 'Katalogen root API' description = '' router_list_name = None @@ -21,7 +21,7 @@ def api_root_dict(self): is_staff = self.request.user.is_staff for prefix, viewset, basename in self.router_registry: # Include the route unless it is staff only - if not is_staff and permissions.IsAdminUser in viewset.permission_classes: + if not is_staff and IsAdminUser in viewset.permission_classes: continue d[prefix] = self.router_list_name.format(basename=basename) @@ -74,8 +74,8 @@ def get_api_root_view(self, api_urls=None): url(r'^dump-arsk/$', dump_arsk, name='dump_arsk'), url(r'^dump-regemails/$', dump_reg_emails, name='dump_reg_emails'), url(r'^dump-studentbladet/$', dump_studentbladet, name='dump_studentbladet'), - # Used by BILL + # Used by BILL (?) url(r'^memberTypesForMember/(?Pusername|studynumber)/(?P[A-Za-z0-9]+)/$', member_types_for_member), - # Used by Generikey - url(r'^membersByMemberType/([A-Z]{2})/(\w+)?$', members_by_member_type), + # Used by BILL and Generikey + url(r'^membersByMemberType/([A-Z]{2})/(?:(\w+)/?)?$', members_by_member_type), ] diff --git a/teknologr/api/views.py b/teknologr/api/views.py index 4f189017..44be39d5 100644 --- a/teknologr/api/views.py +++ b/teknologr/api/views.py @@ -519,7 +519,7 @@ def multi_applicant_submissions(request): # JSON API:s -# Used by BILL +# Used by BILL (?) @api_view(['GET']) def member_types_for_member(request, mode, query): try: @@ -548,7 +548,7 @@ def member_types_for_member(request, mode, query): return Response(data, status=200) -# Used by GeneriKey +# Used by BILL and GeneriKey @api_view(['GET']) def members_by_member_type(request, membertype, field=None): member_pks = MemberType.objects.filter(type=membertype, end_date=None).values_list("member", flat=True) @@ -678,14 +678,11 @@ def dump_active(request): if membership.group.begin_date < now and membership.group.end_date > now: grouped_by_group[membership.group].append(membership.member) for group, members in grouped_by_group.items(): - content.append({ - 'position': str(group.grouptype), - 'member': '' - }) - content.extend([{ - 'position': '', - 'member': m.common_name - } for m in members]) + for m in members: + content.append({ + 'position': str(group.grouptype), + 'member': m.full_name, + }) return Response(content, status=200)