Skip to content

fix: standardize API token format across all creation paths #44

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.

Sign up for GitHub

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

Open
wicky-zipstack wants to merge 3 commits into
base: main
Choose a base branch
from
+70 −10
Open

fix: standardize API token format across all creation paths #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d66f9c8
fix: standardize API token format across all creation paths
wicky-zipstack Apr 7, 2026
68474fc
fix: address Greptile review — upsert pattern, audit logs, rename
wicky-zipstack Apr 7, 2026
446b7c4
fix: address Greptile round 2 — scope filter, audit action, token_hash
wicky-zipstack Apr 7, 2026
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
44 changes: 42 additions & 2 deletions backend/backend/core/routers/api_tokens/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import hashlib
import logging
from datetime import timedelta

Expand Down Expand Up @@ -161,10 +162,11 @@ def regenerate_api_key(request: Request, key_id: str) -> Response:
new_sig = generate_signature(new_key)

token.token = new_key
token.token_hash = hashlib.sha256(new_key.encode()).hexdigest()
token.signature = new_sig
token.is_disabled = False
token.expires_at = now() + timedelta(days=django_settings.API_KEY_EXPIRY_DAYS)
token.save(update_fields=["token", "signature", "is_disabled", "expires_at"])
token.save(update_fields=["token", "token_hash", "signature", "is_disabled", "expires_at"])

logger.info(f"API key regenerated: id={token.id}, label={token.label}, user={request.user.email}")
log_api_key_event(
Expand All @@ -182,8 +184,46 @@ def regenerate_api_key(request: Request, key_id: str) -> Response:
@api_view([HTTPMethods.POST])
@handle_http_request
def generate_token(request: Request) -> Response:
"""Legacy token generation endpoint."""
"""Legacy token generation endpoint.

Now creates a proper APIToken record with vtk_ prefix, label, and expiry
to maintain consistency with the api-keys/create endpoint.
Uses upsert: updates existing auto-generated token or creates a new one.
"""
api_key = generate_api_key()
sig = generate_signature(api_key)
new_expiry = now() + timedelta(days=django_settings.API_KEY_EXPIRY_DAYS)

# Upsert: update existing auto-generated token if present, else create
existing = APIToken.objects.filter(
user=request.user, label="Default"
).order_by("-created_at").first()

if existing:
existing.token = api_key
existing.token_hash = hashlib.sha256(api_key.encode()).hexdigest()
existing.signature = sig
existing.is_disabled = False
existing.expires_at = new_expiry
existing.save(update_fields=["token", "token_hash", "signature", "is_disabled", "expires_at"])
token = existing
audit_action = "regenerate"
else:
token = APIToken.objects.create(
user=request.user,
token=api_key,
signature=sig,
label="Default",
expires_at=new_expiry,
)
audit_action = "create"
Comment on lines +211 to +219
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 7, 2026

Choose a reason for hiding this comment

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

P1 MAX_KEYS_PER_USER limit not enforced on new-token creation path

The upsert logic correctly updates an existing "Default" token in-place, but when no "Default" token exists the else branch creates a brand-new APIToken record without first checking MAX_KEYS_PER_USER. A user who already holds the maximum number of non-Default keys can call this legacy endpoint to silently create an extra token that exceeds the per-user cap enforced by create_api_key.

create_api_key (lines 66–72 of the same file) shows the guard pattern:

max_keys = django_settings.MAX_KEYS_PER_USER
existing_count = APIToken.objects.filter(user=request.user).count()
if existing_count >= max_keys:
    return Response({"message": f"Maximum {max_keys} API keys allowed per user."}, status=status.HTTP_429_TOO_MANY_REQUESTS)

The same check should be added to generate_token before the else branch creates a new record.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: backend/backend/core/routers/api_tokens/views.py
Line: 211-219

Comment:
**`MAX_KEYS_PER_USER` limit not enforced on new-token creation path**

The upsert logic correctly updates an existing `"Default"` token in-place, but when no `"Default"` token exists the `else` branch creates a brand-new `APIToken` record without first checking `MAX_KEYS_PER_USER`. A user who already holds the maximum number of non-Default keys can call this legacy endpoint to silently create an extra token that exceeds the per-user cap enforced by `create_api_key`.

`create_api_key` (lines 66–72 of the same file) shows the guard pattern:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
existing_count = APIToken.objects.filter(user=request.user).count()
if existing_count >= max_keys:
    return Response({"message": f"Maximum {max_keys} API keys allowed per user."}, status=status.HTTP_429_TOO_MANY_REQUESTS)
```
The same check should be added to `generate_token` before the `else` branch creates a new record.

How can I resolve this? If you propose a fix, please make it concise.


logger.info(f"Legacy token generated: id={token.id}, user={request.user.email}")
log_api_key_event(
request, action=audit_action, key_id=token.id,
key_label="Default", key_masked=token.masked_token,
)

return Response({
"message": "Token generated successfully.",
"token": api_key,
Expand Down
10 changes: 7 additions & 3 deletions backend/backend/core/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import logging
import uuid
from datetime import timedelta
from typing import Any, Optional

from django.conf import settings as django_settings
from django.db import IntegrityError
from django.db import transaction
from django.utils.timezone import now

from backend.core.models.api_tokens import APIToken
from backend.core.models.organization_model import Organization
from backend.core.models.user_model import User
from backend.core.services.api_key_service import generate_api_key, generate_signature

Logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -87,11 +88,14 @@ def get_or_create_valid_token(self, user: User, organization: Organization):
token.delete()
token = None
if token is None:
api_key = generate_api_key()
token = APIToken.objects.create(
user=user,
organization=organization,
token=str(uuid.uuid4().hex),
expires_at=now() + timedelta(days=90),
token=api_key,
signature=generate_signature(api_key),
label="Default",
expires_at=now() + timedelta(days=django_settings.API_KEY_EXPIRY_DAYS),
)
logging.info(f"A new api token for user: {user} and tenant: {organization} is created")
except Exception as e:
Expand Down
26 changes: 21 additions & 5 deletions backend/backend/core/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
from backend.utils.tenant_context import get_current_tenant

from backend.core.models.api_tokens import APIToken
from backend.core.services.api_key_audit import log_api_key_event
from backend.core.services.api_key_service import generate_api_key, generate_signature
from django.conf import settings as django_settings
from django.utils.timezone import now
from datetime import timedelta

Expand Down Expand Up @@ -52,16 +55,29 @@ def update_user_profile(request: Request) -> Response:


def update_user_token(request, user):
new_token = request.data.get("token")
existing_token: APIToken = APIToken.objects.filter(user=user).first()
# token_value is sent back by the frontend — used only to detect "unchanged"
token_value = request.data.get("token")
existing_token: APIToken = APIToken.objects.filter(user=user, label="Default").first()

if new_token:
if existing_token and existing_token.token == new_token:
if token_value:
# Skip regeneration if the token hasn't changed
if existing_token and existing_token.token == token_value:
return
if existing_token:
existing_token.delete()

APIToken.objects.create(user=user, token=new_token, expires_at= now() + timedelta(days=90))
api_key = generate_api_key()
token = APIToken.objects.create(
user=user,
token=api_key,
signature=generate_signature(api_key),
label="Default",
expires_at=now() + timedelta(days=django_settings.API_KEY_EXPIRY_DAYS),
)
log_api_key_event(
request, action="create", key_id=token.id,
key_label="Default", key_masked=token.masked_token,
)
Comment on lines +62 to +80
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 7, 2026

Choose a reason for hiding this comment

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

P1 MAX_KEYS_PER_USER limit not enforced in update_user_token

Before this PR update_user_token stored the raw frontend string — no real DB row was inserted. Now it calls APIToken.objects.create(...) (line 70), creating a real row that counts against the per-user key cap. However, no MAX_KEYS_PER_USER guard is applied before the create call, unlike create_api_key. A user who has reached the maximum via the KeyManagement UI can get an extra token simply by saving their profile page.

Consider adding the same count check:

max_keys = django_settings.MAX_KEYS_PER_USER
if APIToken.objects.filter(user=user).count() >= max_keys:
    return  # or surface an appropriate error
Prompt To Fix With AI 
This is a comment left during a code review.
Path: backend/backend/core/views.py
Line: 62-80

Comment:
**`MAX_KEYS_PER_USER` limit not enforced in `update_user_token`**

Before this PR `update_user_token` stored the raw frontend string — no real DB row was inserted. Now it calls `APIToken.objects.create(...)` (line 70), creating a real row that counts against the per-user key cap. However, no `MAX_KEYS_PER_USER` guard is applied before the create call, unlike `create_api_key`. A user who has reached the maximum via the KeyManagement UI can get an extra token simply by saving their profile page.

Consider adding the same count check:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
if APIToken.objects.filter(user=user).count() >= max_keys:
    return  # or surface an appropriate error
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

@wicky-zipstack wicky-zipstack Apr 7, 2026

Choose a reason for hiding this comment

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

Acknowledged — low risk in practice since both paths only create label="Default" tokens (one per user via upsert). The only edge case is if the Default token was manually deleted and the user already has MAX_KEYS_PER_USER keys from KeyManagement. Will add the guard in a follow-up if needed.

else:
if existing_token:
existing_token.delete()
Expand Down
Loading