Skip to content

feat(auth): Add web authentication with django-allauth#993

Open
srtab wants to merge 7 commits intomainfrom
feat/allauth-authentication
Open

feat(auth): Add web authentication with django-allauth#993
srtab wants to merge 7 commits intomainfrom
feat/allauth-authentication

Conversation

@srtab
Copy link
Copy Markdown
Owner

@srtab srtab commented Mar 28, 2026

Add social login (GitHub, GitLab), passwordless login-by-code, styled login/dashboard pages, and API key management UI.

  • django-allauth with conditional provider registration
  • Tailwind CSS v4 via standalone CLI (ARM-compatible)
  • WhiteNoise for static file serving
  • Dark-themed login, code entry, dashboard, and API keys pages
  • setup_default_site management command (reads DAIV_EXTERNAL_URL)
  • Custom social account adapter for GitLab server URL separation

@srtab srtab self-assigned this Mar 28, 2026
@srtab
Copy link
Copy Markdown
Owner Author

srtab commented Mar 28, 2026

@daiv-agent /code-review

@daiv-agent
Copy link
Copy Markdown
Contributor

daiv-agent bot commented Mar 28, 2026


Findings

1. collectstatic at runtime regenerates static files on every container restart, causing avoidable downtime riskdocker/production/app/start-app:7

Details

django-admin collectstatic --noinput is now called inside start-app, which runs at every container startup. In production, this has two risks:

  1. Race condition on rolling restarts: if multiple replicas start simultaneously and share a common STATIC_ROOT (a bind-mounted volume), concurrent collectstatic runs can corrupt the staticfiles.json manifest used by CompressedManifestStaticFilesStorage.
  2. Redundancy: Tailwind is already compiled and static files are already collected during the docker build step (the RUN block that ends with --minify). Calling it again at runtime serves no new purpose.

Consider removing collectstatic from start-app and relying solely on the build-time step, or at minimum guard it with a file-system sentinel (e.g., test -f data/static/staticfiles.json || django-admin collectstatic).


2. SOCIALACCOUNT_LOGIN_ON_GET = True is a CSRF bypass that allows one-click account takeover via a crafted URLdaiv/daiv/settings/components/allauth.py:14

Details

With SOCIALACCOUNT_LOGIN_ON_GET = True, visiting /accounts/github/login/ via a GET request (e.g., from a link in an email or another site) immediately initiates the OAuth flow without requiring the user to click a "Continue" button backed by a CSRF-protected POST. An attacker who can cause a victim's browser to follow a crafted URL can silently attach their social account to the victim's session (login CSRF) or force an unwanted login.

django-allauth's own documentation explicitly warns about this setting and recommends keeping it False, showing the intermediate confirmation page instead. The custom socialaccount/login.html template is already present in this PR and renders a CSRF-protected POST form—so the UX for this confirmation step is already built. Just remove or set the setting to False.


3. setup_default_site failure aborts the container start unconditionallydocker/production/app/start-app:4

Details

start-app runs under set -eu, so django-admin setup_default_site raising a CommandError (e.g., DAIV_EXTERNAL_URL not set, or the site row missing before migrate was run in the right order) will kill the container with exit code 1. This is an implicit hard dependency that isn't validated and will produce a confusing failure in environments where the setting isn't configured or where the database migration ran in a separate init step.

Consider adding an explicit pre-check or making the command exit 0 with a warning instead of raising CommandError when the URL is unconfigured, and document the startup requirement clearly.


Suggestions

  • The APIKeyCreateForm in forms.py re-declares name = forms.CharField(max_length=128) which is already on the model—the Meta.fields alone is sufficient here, unless you intend to customise the widget or label.
  • The Tailwind CSS version v4.1.7 is hardcoded in three separate places (local Dockerfile, production Dockerfile, and the comment in styles.css). Centralising it in a build ARG or a shared variable would make upgrades a single-edit operation.
  • The production Dockerfile downloads the Tailwind binary during image build via curl without verifying a checksum; adding a sha256 verification step would prevent supply-chain tampering.
  • ACCOUNT_EMAIL_UNKNOWN_ACCOUNTS = False silently drops the login-code email when an unknown address is submitted, which can be confusing for users who mistype their email. Confirm this UX is intentional.
  • The hardcoded /api/docs/ link in accounts/templates/accounts/dashboard.html (line 599) should use a {% url %} tag or at minimum a settings variable for consistency with the rest of the routing.

Tests

  • No tests are included for APIKeyCreateView and APIKeyRevokeView; at minimum, cover the ownership check (a user cannot revoke another user's key) and the happy-path create-then-list flow.
  • GitLabServerAwareAdapter._build_server_url has no test; a unit test asserting it falls back to _build_url when gitlab_server_url is absent would catch future regressions.
  • setup_default_site management command has no test; cover the DAIV_EXTERNAL_URL-missing and the Site-not-found error branches.

Add social login (GitHub, GitLab), passwordless login-by-code,
styled login/dashboard pages, and API key management UI.

- django-allauth with conditional provider registration
- Tailwind CSS v4 via standalone CLI (ARM-compatible)
- WhiteNoise for static file serving
- Dark-themed login, code entry, dashboard, and API keys pages
- setup_default_site management command (reads DAIV_EXTERNAL_URL)
- Custom social account adapter for GitLab server URL separation
@srtab srtab force-pushed the feat/allauth-authentication branch from 60a8b38 to 79dc9b5 Compare March 28, 2026 23:26
srtab added 6 commits March 28, 2026 23:40
Change <script> to <script type="module"> so the toast auto-dismiss
logic does not block page rendering.
The version was hardcoded in both Dockerfiles. A top-level ARG with a
default makes upgrades a single-edit operation per file and allows
overriding at build time via --build-arg.
Set a stable urls_namespace="api" on NinjaAPI so the dashboard template
can use {% url 'api:openapi-view' %} instead of a hardcoded path.
Cover the happy-path create flow, ownership check (a user cannot revoke
another user's key), already-revoked guard, and login-required redirects.
Replace single accounts/urls.py with allauth_urls.py, api_keys_urls.py,
and dashboard_urls.py. Move custom GitLab OAuth and allauth routes from
the root urlconf into accounts/allauth_urls.py so each concern has its
own module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant