-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Secure API for listing users and groups #338
Changes from all commits
e496e6a
3765562
69a53e5
056f5f4
7317ea7
c484de3
2db02f8
2e235d2
16138a0
aecde3a
08b7e26
a0c3370
b049409
28ff18b
4bb5e3c
be8ecc8
8b30649
837b0f9
6e28f9b
052f3c3
8089052
60c27b3
5e39d61
be33e78
b43f003
842c33f
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,61 @@ | ||
# from rest_framework import serializers as rest_serializers | ||
from drf_spectacular.utils import extend_schema | ||
from drf_spectacular.utils import extend_schema_view | ||
from rest_framework import viewsets | ||
from rest_framework.permissions import IsAuthenticatedOrReadOnly | ||
from rest_framework_api_key.permissions import HasAPIKey | ||
|
||
from core.api.serializers import PracticeAreaSerializer | ||
from core.api.serializers import UserSerializer | ||
from core.models import PracticeArea | ||
from core.models import User | ||
|
||
|
||
@extend_schema_view( | ||
list=extend_schema( | ||
description=""" | ||
Lists all users and the associated groups for the user | ||
|
||
Requires | ||
- API keys have been added using the API key screen in the People depot App. | ||
Check with People Depot admin. | ||
- Requires X-Api-Key be set when submitting request | ||
For curl, it would look like this: | ||
```http | ||
curl -L -X GET "localhost:8001/api/v1/apikey/getusers/" | ||
-H "X-Api-Key: *************" | ||
-H "Content-Type: application/json" | ||
``` | ||
|
||
|
||
For python, it would look like this: | ||
```bash | ||
import requests | ||
|
||
API_KEY = os.environ.get("API_KEY") | ||
BASE_URL = "http:8000" | ||
HEADERS = { | ||
'X-Api-Key': API_KEY, | ||
'Content-Type': 'application/json' | ||
} | ||
response = requests.get(f"{BASE_URL}/api/v1/apikey/getusers/", headers=HEADERS) | ||
``` | ||
""" | ||
) | ||
) | ||
class ApiKeyUserViewSet(viewsets.ReadOnlyModelViewSet): | ||
# HasAPIKey checks if value of X-API-KEY is in API keys table | ||
permission_classes = [IsAuthenticatedOrReadOnly, HasAPIKey] | ||
queryset = PracticeArea.objects.all() | ||
serializer_class = PracticeAreaSerializer # HasAPIKey checks against keys stored in | ||
queryset = User.objects.all() | ||
|
||
# when instantiated, get_serializer_context will be called | ||
serializer_class = UserSerializer | ||
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. Are you sure you should be returning all the I don't know if this PR is dependent on the user permission changes to somehow limit the exposed data fields, but the code right now will return all 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 probably should have been settled in the planning stage of this feature rather than during the PR review. To clarify further, the problem I see is this new API key authentication method allows the API client to view These are the fields it returns for
I'm thinking that KnowledgeBase doesn't really need or care about a user's So I think we need to consult the "permissions team" (@ExperimentsInHonesty @Neecolaa) to figure out/negotiate what data this should include based on what data KnowledgeBase requires to function. This seems to mean adding more/duplicate work to the "permissions" team (@ExperimentsInHonesty and @Neecolaa) to define exactly what tables and fields this new method can allow an API client to access. Would it be better to say the API client has the same permissions as one of the already-defined user permissions tier such as For reference: here's the Field Permissions sheet being worked on currently. |
||
|
||
# get_serializer_context called to set include_groups to True | ||
# to include groups in the response | ||
def get_serializer_context(self): | ||
context = super().get_serializer_context() | ||
context["include_groups"] = True | ||
return context |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
from django.urls import path | ||
from rest_framework import routers | ||
|
||
from .api_key_views import ApiKeyUserViewSet | ||
from .views import AffiliateViewSet | ||
from .views import AffiliationViewSet | ||
from .views import CheckTypeViewSet | ||
|
@@ -37,6 +38,8 @@ | |
router.register( | ||
r"stack-element-types", StackElementTypeViewSet, basename="stack-element-type" | ||
) | ||
router.register(r"apikey/getusers", ApiKeyUserViewSet, basename="apikey-getusers") | ||
router.register(r"sdgs", SdgViewSet, basename="sdg") | ||
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. Looks like a duplicate line got added here? |
||
router.register(r"sdgs", SdgViewSet, basename="sdg") | ||
router.register( | ||
r"affiliations", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import datetime | ||
|
||
from django.urls import reverse | ||
from rest_framework import status | ||
from rest_framework.test import APITestCase | ||
from rest_framework_api_key.models import APIKey | ||
|
||
api_key_url = reverse("apikey-getusers-list") | ||
|
||
|
||
class ApiKeyUserViewSetTests(APITestCase): | ||
def test_succeeds(self): | ||
_, api_key = APIKey.objects.create_key(name="test") | ||
response = self.client.get( | ||
api_key_url, | ||
HTTP_X_API_KEY=api_key, # Uppercase X-API-KEY | ||
Content_Type="application/json", | ||
) | ||
assert response.status_code == status.HTTP_200_OK | ||
|
||
def test_expired_fails(self): | ||
expired_date = datetime.datetime.now() - datetime.timedelta(days=2) | ||
_, api_key = APIKey.objects.create_key(name="test", expiry_date=expired_date) | ||
response = self.client.get( | ||
api_key_url, | ||
HTTP_X_API_KEY=api_key, # Uppercase X-API-KEY | ||
Content_Type="application/json", | ||
) | ||
assert response.status_code == status.HTTP_401_UNAUTHORIZED |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,13 +18,16 @@ | |
# Build paths inside the project like this: BASE_DIR / 'subdir'. | ||
BASE_DIR = Path(__file__).resolve().parent.parent | ||
|
||
# Used by djangorestframework-api-key | ||
API_KEY_CUSTOM_HEADER = "HTTP_X_API_KEY" | ||
|
||
# Quick-start development settings - unsuitable for production | ||
# See https://docs.djangoproject.com/en/4.0/howto/deployment/checklist/ | ||
|
||
# SECURITY WARNING: keep the secret key used in production secret! | ||
SECRET_KEY = os.environ.get("SECRET_KEY") | ||
|
||
|
||
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. Adding an extra blank line? Possibly a merging artifact? |
||
DJANGO_SUPERUSER_USERNAME = os.environ.get("DJANGO_SUPERUSER_USERNAME") | ||
DJANGO_SUPERUSER_EMAIL = os.environ.get("DJANGO_SUPERUSER_EMAIL") | ||
DJANGO_SUPERUSER_PASSWORD = os.environ.get("DJANGO_SUPERUSER_PASSWORD") | ||
|
@@ -70,6 +73,7 @@ | |
# 3rd party | ||
"django_extensions", | ||
"rest_framework", | ||
"rest_framework_api_key", | ||
"drf_spectacular", | ||
"phonenumber_field", | ||
"timezone_field", | ||
|
@@ -175,7 +179,10 @@ | |
] | ||
|
||
REST_FRAMEWORK = { | ||
"DEFAULT_PERMISSION_CLASSES": ("core.api.permissions.DenyAny",), | ||
"DEFAULT_PERMISSION_CLASSES": ( | ||
"core.api.permissions.DenyAny", | ||
"rest_framework_api_key.permissions.HasAPIKey", | ||
), | ||
"DEFAULT_AUTHENTICATION_CLASSES": ( | ||
"rest_framework_jwt.authentication.JSONWebTokenAuthentication", | ||
), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ djangorestframework==3.14.0 | |
# via | ||
# drf-jwt | ||
# drf-spectacular | ||
djangorestframework-api-key>=3.0 | ||
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're using docker-compose exec web uv pip compile requirements.in -o requirements.txt --no-header There's documentation on how to use it for upgrading dependencies. The command above is just missing the |
||
drf-jwt==1.19.2 | ||
drf-spectacular==0.27.1 | ||
flake8==7.0.0 | ||
|
@@ -59,6 +60,7 @@ platformdirs==4.2.0 | |
# via black | ||
pluggy==1.4.0 | ||
# via pytest | ||
pre-commit==3.7.1 | ||
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 do you want to install |
||
psycopg2-binary==2.9.9 | ||
pycodestyle==2.11.1 | ||
# via flake8 | ||
|
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.
These 2 lines don't seem to do anything and they're soon replaced by the next 2 code lines of
User
objects andUserSerializer
. Maybe you meant to remove them?