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

Secure API for listing users and groups #338

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e496e6a
WIP: secured apis for getting and creating users
ethanstrominger Jun 26, 2024
3765562
Complete tests and implentation for secure api
ethanstrominger Jun 28, 2024
69a53e5
Add comments
ethanstrominger Jun 30, 2024
056f5f4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 30, 2024
7317ea7
Adjust for pre-commit
ethanstrominger Jun 30, 2024
c484de3
Adjust for pre-commit
ethanstrominger Jun 30, 2024
2db02f8
Run through black
ethanstrominger Jun 30, 2024
2e235d2
Merge branch 'secured-apis' of https://github.com/hackforla/peopledep…
ethanstrominger Jun 30, 2024
16138a0
Format secret_permissions.py
ethanstrominger Jun 30, 2024
aecde3a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 30, 2024
08b7e26
Change self.assertEqual to assert
ethanstrominger Jul 1, 2024
a0c3370
Merge branch 'secured-apis' of https://github.com/hackforla/peopledep…
ethanstrominger Jul 1, 2024
b049409
Refactored to simplify
ethanstrominger Sep 5, 2024
28ff18b
Merge branch 'main' of https://github.com/hackforla/peopledepot into …
ethanstrominger Sep 11, 2024
4bb5e3c
Changed to use djangorestframework-api-key
ethanstrominger Sep 12, 2024
be8ecc8
Force port to be 5433
ethanstrominger Sep 12, 2024
8b30649
Fix spelling
ethanstrominger Sep 12, 2024
837b0f9
Spelling
ethanstrominger Sep 12, 2024
6e28f9b
Add to settings.py
ethanstrominger Sep 12, 2024
052f3c3
Add hadolint ignore=DL3008
ethanstrominger Sep 12, 2024
8089052
Revert .env.docker-example
ethanstrominger Sep 12, 2024
60c27b3
Removed unneeded files
ethanstrominger Sep 12, 2024
5e39d61
Revert changes to Dockerfile
ethanstrominger Sep 13, 2024
be33e78
Merge branch 'main' into secured-apis
ethanstrominger Sep 13, 2024
b43f003
Merge branch 'main' of https://github.com/hackforla/peopledepot into …
ethanstrominger Sep 20, 2024
842c33f
Remove unneeded code, rename secret to api_key, and update doc
ethanstrominger Sep 20, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/.env.docker-example
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
DEBUG=1
SECRET_KEY=foo
SECRET_API_SECRET=bar
ethanstrominger marked this conversation as resolved.
Show resolved Hide resolved
DJANGO_PORT=8000
DJANGO_ALLOWED_HOSTS="localhost 127.0.0.1 [::1]"
DJANGO_SUPERUSER_USERNAME=admin1111
Expand Down
59 changes: 59 additions & 0 deletions app/core/api/secret_permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import hashlib
import hmac
import time

from django.conf import settings
from rest_framework.exceptions import PermissionDenied
from rest_framework.permissions import BasePermission

from core.constants import message_invalid_signature


class _ApiFields:
def __init__(self, api_key: str, timestamp: str, signature: str):
self.api_key = api_key
self.timestamp = timestamp
self.signature = signature


def _get_request_api_fields(request) -> _ApiFields:
api_key = request.headers.get("X-API-Key", "")
timestamp = request.headers.get("X-API-Timestamp", "")
signature = request.headers.get("X-API-Signature", "")
return _ApiFields(api_key=api_key, timestamp=timestamp, signature=signature)


def _is_expected_signature(api_key: str, timestamp: str, signature: str) -> bool:
current_timestamp = int(time.time())
request_timestamp = int(timestamp)
if abs(current_timestamp - request_timestamp) > 10:
return False
ethanstrominger marked this conversation as resolved.
Show resolved Hide resolved

# Recreate the message and calculate the expected signature
message = f"{timestamp}{api_key}"
expected_signature = hmac.new(
settings.SECRET_API_KEY.encode("utf-8"), message.encode("utf-8"), hashlib.sha256
).hexdigest()

# Use constant-time comparison to prevent timing attacks
return hmac.compare_digest(signature, expected_signature)


def _has_expected_request_signature(request) -> bool:
api_fields = _get_request_api_fields(request)
return _is_expected_signature(
api_fields.api_key, api_fields.timestamp, api_fields.signature
)


class HasValidSignature(BasePermission):
def has_permission(self, request, __view__) -> bool:
if not _has_expected_request_signature(request):
raise PermissionDenied(message_invalid_signature)
return True

def has_object_permission(self, request, __view__, __obj__) -> bool:
if not _has_expected_request_signature(request):
raise PermissionDenied(message_invalid_signature)
return True
return _has_expected_request_signature(request)
20 changes: 20 additions & 0 deletions app/core/api/secret_views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# from rest_framework import serializers as rest_serializers
from rest_framework import viewsets

from core.api.secret_permissions import HasValidSignature
from core.api.serializers import UserSerializer
from core.models import User


class SecretUserViewSet(viewsets.ReadOnlyModelViewSet):
permission_classes = [HasValidSignature]
queryset = User.objects.all()
# when instantiated, get_serializer_context will be called
serializer_class = UserSerializer

# 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
23 changes: 22 additions & 1 deletion app/core/api/serializers.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.contrib.auth.models import Group
from rest_framework import serializers
from timezone_field.rest_framework import TimeZoneSerializerField

Expand Down Expand Up @@ -37,10 +38,23 @@ class Meta:
)


class GroupSerializer(serializers.ModelSerializer):
class Meta:
model = Group
fields = ("id", "name")


class UserSerializer(serializers.ModelSerializer):
"""Used to retrieve user info"""
"""
Serializer for User model.

Parameters:
- include_groups (bool): Flag to include user groups in the serialized output.
"""

time_zone = TimeZoneSerializerField(use_pytz=False)
# see get_groups method
groups = serializers.SerializerMethodField()

class Meta:
model = User
Expand All @@ -64,6 +78,7 @@ class Meta:
"phone",
"texting_ok",
"time_zone",
"groups",
)
read_only_fields = (
"uuid",
Expand All @@ -73,6 +88,12 @@ class Meta:
"email",
)

def get_groups(self, obj):
include_groups = self.context.get("include_groups", False)
if include_groups:
return GroupSerializer(obj.groups.all(), many=True).data
return None


class ProjectSerializer(serializers.ModelSerializer):
"""Used to retrieve project info"""
Expand Down
5 changes: 5 additions & 0 deletions app/core/api/urls.py
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 .secret_views import SecretUserViewSet
from .views import AffiliateViewSet
from .views import AffiliationViewSet
from .views import EventViewSet
Expand Down Expand Up @@ -34,6 +35,10 @@
router.register(
r"stack-element-types", StackElementTypeViewSet, basename="stack-element-type"
)
router.register(
r"secret-api/getusers", SecretUserViewSet, basename="secret-api-getusers"
)
router.register(r"sdgs", SdgViewSet, basename="sdg")
Copy link
Member

Choose a reason for hiding this comment

The 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",
Expand Down
1 change: 1 addition & 0 deletions app/core/constants.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
message_invalid_signature = "Invalid signature"
95 changes: 95 additions & 0 deletions app/core/tests/test_secret_api.py
ethanstrominger marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import hashlib
import hmac
import time

from django.contrib.auth.models import Group
from django.urls import reverse
from rest_framework import status
from rest_framework.test import APIClient
from rest_framework.test import APITestCase

from core.models import User
from peopledepot.settings import SECRET_API_KEY

secret_url = reverse("secret-api-getusers-list")


class SecretUserViewSetTests(APITestCase):
def setUp(self):
# Create a test user
self.client = APIClient()

def generate_signature(self, api_key, timestamp):
# Generate a valid signature
message = f"{timestamp}{api_key}"
return hmac.new(
SECRET_API_KEY.encode("utf-8"), message.encode(), hashlib.sha256
).hexdigest()

def test_access_with_invalid_signature(self):
response = self.client.get(
secret_url,
HTTP_X_API_Key=SECRET_API_KEY,
HTTP_X_API_Timestamp=str(int(time.time())),
HTTP_X_API_Signature="invalidsignature",
)
assert response.status_code == status.HTTP_403_FORBIDDEN

def test_access_with_invalid_timestamp(self):
invalid_timestamp = str(int(time.time()) - 20) # Invalid timestamp (too old)
ethanstrominger marked this conversation as resolved.
Show resolved Hide resolved
signature = self.generate_signature(SECRET_API_KEY, invalid_timestamp)
response = self.client.get(
secret_url,
HTTP_X_API_Key=SECRET_API_KEY,
HTTP_X_API_Timestamp=invalid_timestamp,
HTTP_X_API_Signature=signature,
)
assert response.status_code == status.HTTP_403_FORBIDDEN

def test_succeeds_with_valid_signature_without_authentication(self):
timestamp = str(int(time.time()))
signature = self.generate_signature(SECRET_API_KEY, timestamp)
response = self.client.get(
secret_url,
HTTP_X_API_Key=SECRET_API_KEY,
HTTP_X_API_Timestamp=timestamp,
HTTP_X_API_Signature=signature,
)
assert response.status_code == status.HTTP_200_OK
self.client.force_authenticate(user=None) # Log out the user

def test_succeeds_with_valid_signature_and_authentication(self):
user = User.objects.create_user(username="testuser", password="testpassword")
self.client.force_authenticate(user=user)
timestamp = str(int(time.time()))
signature = self.generate_signature(SECRET_API_KEY, timestamp)
response = self.client.get(
secret_url,
HTTP_X_API_Key=SECRET_API_KEY,
HTTP_X_API_Timestamp=timestamp,
HTTP_X_API_Signature=signature,
)
assert response.status_code == status.HTTP_200_OK
self.client.force_authenticate(user=None) # Log out the user

def test_list_users(self):
user = User.objects.create_user(username="testuser", password="testpassword")
group = Group.objects.create(name="testgroup")
group2 = Group.objects.create(name="testgroup2")
# third group created to confirm len(user.groups) does not include group3
Group.objects.create(name="testgroup3")
user.groups.add(group)
user.groups.add(group2)

timestamp = str(int(time.time()))
signature = self.generate_signature(SECRET_API_KEY, timestamp)
response = self.client.get(
secret_url,
HTTP_X_API_Key=SECRET_API_KEY,
HTTP_X_API_Timestamp=timestamp,
HTTP_X_API_Signature=signature,
)
assert response.status_code == status.HTTP_200_OK

group_count = len(response.data[0]["groups"])
assert group_count == 2
3 changes: 3 additions & 0 deletions app/peopledepot/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@
# See https://docs.djangoproject.com/en/4.0/howto/deployment/checklist/

# SECURITY WARNING: keep the secret key used in production secret!
# SECRET_KEY is used internally by Django to encrypt session data
# SECRET_API_KEY is used to authenticate a "secret" API endpoint
SECRET_KEY = os.environ.get("SECRET_KEY")
SECRET_API_KEY = os.environ.get("SECRET_API_KEY")

Copy link
Member

@fyliu fyliu Sep 23, 2024

Choose a reason for hiding this comment

The 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")
Expand Down
1 change: 1 addition & 0 deletions app/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ platformdirs==4.2.0
# via black
pluggy==1.4.0
# via pytest
pre-commit==3.7.1
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to install pre-commit inside the docker container?

psycopg2-binary==2.9.9
pycodestyle==2.11.1
# via flake8
Expand Down
5 changes: 0 additions & 5 deletions scripts/start-local.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ if [[ $PWD != *"app"* ]]; then
}
fi

SCRIPT_DIR="$(dirname "$0")"
"$SCRIPT_DIR"/loadenv.sh || {
echo "ERROR: loadenv.sh failed"
return 1
}
echo Admin user = "$DJANGO_SUPERUSER" email = "$DJANGO_SUPERUSER_EMAIL"
if [[ $1 != "" ]]; then
port=$1
Expand Down