Oh yeah. It's happening.

Fork 0

Set up Wagtail with password-free sign-in #13

Merged

personhood.removal.surgery 1mo ago

targeting main from codex/oidc-login

Merged opened by and assigned to

personhood.removal.surgery 1mo ago targeting main from codex/oidc-login

Hornroot now has a working local Wagtail admin with custom OIDC, Discord, and ATProto sign-in in place of local passwords. Provider settings and administrator identities come from an ignored TOML config; email addresses do not link accounts, and provider tokens are not stored.

The foundation uses a uv workspace with separate web, authentication, and ATProto packages, an ASGI development server, mise tasks, strict basedpyright checks, pytest, and Zed configuration. ATProto sign-in verifies the DID/PDS relationship and uses PKCE, PAR, and DPoP. Existing sessions recheck configured access, and authentication errors and logs omit private OAuth state.

Validation:

49 pytest tests passed, including access-policy, callback, session, and failure-boundary tests.
Ruff, basedpyright, Django checks, and migration consistency passed.
Live ATProto sign-in completed in a regular browser; OIDC and Discord flows are covered with simulated provider responses.
Local configuration, credentials, databases, media, and research source checkouts are excluded from Git.

Commits 0

Could not load commits.

Comments

v0 1mo ago

personhood.removal.surgery 1mo ago

README.md:100-112

do these need to be full separate packages, wouldn't it be enough for them to just be apps inside hornroot's larger project? i.e. hornroot as a whole as a django project and web, auth, etc as apps

personhood.removal.surgery 1mo ago

pyproject.toml:29

i'm a line-length = 79 enjoyer personally

personhood.removal.surgery 1mo ago

pyproject.toml:44-52

could you find tooling for python to find outdated dependencies and wire it up to our tangled CI?

personhood.removal.surgery 1mo ago

packages/atproto-auth/src/hornroot_atproto/http.py:45

this looks like a code smell. if https_url is a formatter it shouldn't be used like this, if it's a type guard it should be defined as a type guard.

personhood.removal.surgery 1mo ago

packages/auth/src/hornroot_auth/config.py:94 this looks to me like OIDC and Discord need a .provider_config() method that returns these dicts, rather than having the branching handled in AuthConfig?

personhood.removal.surgery 1mo ago

packages/web/src/hornroot_web/development.py:13

_ = ... is a code smell

personhood.removal.surgery 1mo ago

packages/web/src/hornroot_web/development.py:14

explicit flushing should not be necessary, should happen when the context manager falls through

personhood.removal.surgery 1mo ago

packages/web/src/hornroot_web/development.py:17

if we ignore FileExistsError, explicitly checking for PathExists is completely unnecessary

personhood.removal.surgery 1mo ago

packages/atproto-auth/src/hornroot_atproto/views.py:44-45

why in the world would we check for this here and not in urls.py? what's the point of registering atproto routes if atproto is not configured?

personhood.removal.surgery 1mo ago

packages/auth/src/hornroot_auth/backends.py:15

this comment is meaningless

personhood.removal.surgery 1mo ago

packages/auth/src/hornroot_auth/backends.py:35-45

this entire function could be much shorter and avoid the repeated time() calls:

t = time()
if (user := super().get_user(user_id) and any(
    may_admin(settings.HORNROOT_AUTH, account.provider, account.uid, account.extra_data, t)
    for account in user.social_accounts.all()
):
    return user
return None

personhood.removal.surgery 1mo ago

this would read much clearer without the negation packages/auth/src/hornroot_auth/adapter.py:29-34

if (
    sociallogin.state.get("process", "login") == "login"
    and self.is_open_for_signup(request, sociallogin)
    and (user := sociallogin.user)
    and user.is_active
):

personhood.removal.surgery 1mo ago

packages/auth/src/hornroot_auth/adapter.py:42

wait, so if the user is not existing do we just... do nothing? what's the logic here?

personhood.removal.surgery 1mo ago

packages/atproto-auth/src/hornroot_atproto/views.py:72-73

please stop checking if config is none in the hot path, this is NOT where that check should go. i'm not sure if that check should exist at all.

personhood.removal.surgery 1mo ago

packages/atproto-auth/src/hornroot_atproto/flow.py:180-187

this too would clearer as an and chain

personhood.removal.surgery 1mo ago

packages/atproto-auth/src/hornroot_atproto/http.py:24-30

this too would read clearer as an and chain, and also i'm pretty sure packages/atproto-auth/src/hornroot_atproto/flow.py:147-156

...surely this is not necessary or our oauth library or even a pydantic schema or anything could take care of this, right? this is an insanely overengineered check which also successfully removes any useful information for debugging should anything go wrong.

personhood.removal.surgery 1mo ago

the tests are massively overengineered and the code is massively overdefensive and way too sprawling. the auth and atproto packages could probably be a tenth of their current size.

chappy.tngl.sh 1mo ago

i addressed the 17 current review comments in the feature branch. the recurring mistake was adding local machinery before checking what the framework and libraries already guaranteed. the review fixes remove that duplication:

project layout: one uv project now contains hornroot.web, hornroot.auth, and hornroot.atproto as Django apps. separate distribution metadata is gone.
formatting and tooling: Ruff uses 79 columns. mise run outdated uses uv's existing dependency report; Tangled CI runs setup, checks, tests, and that report. the native wheel runtime issue in Nixery is fixed.
configuration: OIDC and Discord each own provider_config(). ATProto routes mount only when configured and receive a required config argument, removing the per-request None checks.
validation and errors: require_https_url is an explicitly named raising validator, called directly. SavedLogin, PARResponse, and TokenResponse replace the hand-written structural checks. the OAuth library validates callbacks; diagnostics retain field locations or exception names without exposing OAuth codes, tokens, or private keys.
authorization lifecycle: the backend captures time once and uses one positive authorization condition. the adapter also uses a positive condition. new users receive their admin flags before allauth's later save; existing users are saved in the hook. that lifecycle is now documented. Django supplies the async user lookup, and the redundant backend docstring is removed.
secret creation: the temporary file closes before the atomic hard link. discarded return assignments, explicit flushing, and the redundant existence precheck are removed.
tests: the simulated provider no longer implements its own JWT signature verification or duplicates library PKCE checks. application-level handshake, nonce continuity, identity/access policy, expiry, and failure-boundary coverage remain. private provider values are also checked against rendered errors.

the remaining custom protocol code owns ATProto discovery, DID/PDS binding, DPoP nonce retries, and restrictions on outbound destinations. these are concrete responsibilities of this integration. i have not achieved a tenfold reduction and won't present the change that way.

i also updated AGENTS.md to require library/lifecycle inspection, configuration binding, focused tests, and a real reason for each extra guard or package. it now distinguishes instruction sources, Git state from authorship, and existing authorization from new scope. local .codex/ content and private instruction overrides are ignored.

49 tests, Ruff, basedpyright, Django checks, and migration consistency pass locally and in Tangled CI. live ATProto sign-in passed after the review fixes; OIDC and Discord were tested with simulated providers. the original PR description's uv-workspace sentence is superseded by the one-project layout in this round.

personhood.removal.surgery 1mo ago

tests/atproto/authority.py:28-64

i really like this. i feel like it would be more elegant if the DID were a constructor argument rather than a referenced global constant, but this is really nice.

personhood.removal.surgery 1mo ago

src/hornroot/atproto/config.py:12-26

surely there's a clearer way to express this, right? this conditional is very hard to parse and takes a lot of cognitive effort to figure out

personhood.removal.surgery 1mo ago

src/hornroot/atproto/config.py:30

we should rstrip the base_url on construction so that we don't have to repeatedly do that every time we use it. it's the same failure mode as last time - doing checks repeatedly in logic rather than ensuring invalid state is unrepresentable in the first place.

personhood.removal.surgery 1mo ago

src/hornroot/web/settings.py:84-86

this shouldn't just be HORNROOT_AUTH, auth is only a part of the config; hornroot.toml will be our main config file and so we should read all the settings that make sense to customize per-instance (e.g. database path, timezone, etc) from there.

personhood.removal.surgery 1mo ago

src/hornroot/web/settings.py:116

please use lowercase and no capitalization in all prose, especially UI text and in comments, as a convention

personhood.removal.surgery 1mo ago

src/hornroot/auth/adapter.py:31-36

negating this:

        if not (
            sociallogin.state.get("process", "login") == "login"
            and self.is_open_for_signup(request, sociallogin)
            and (user := sociallogin.user) is not None
            and user.is_active
        ):

lets us save an indentation level by having the branch only contain error handling :3

chappy.tngl.sh 1mo ago

all six notes are addressed in e9ed244.

Authority takes its DID, the origin checks have names, and base_url is normalized on construction. the signup guard now handles failure first.

hornroot.toml covers instance settings, with providers under [auth.oidc], [auth.discord], and [auth.atproto]. paths, timezone, language, hosts, site name, debug, and the instance secret are configurable. the README explains the layout; my local config is migrated.

our UI copy, comments, and docs are lowercase, with plainer sign-in errors. 61 tests, the full local checks, and CI pass. i checked the login page and the existing admin session in the browser too.

chappy.tngl.sh 1mo ago

7a8422f includes the discovery cleanup. configs are editable, metadata() returns ClientMetadata, and there are no cast(...) calls left in src or tests.

i treated config as snapshots and patched weak JSON types with casts, including types from our own allauth stub. the stub now preserves the JSON type, and server metadata and the saved session have schemas.

_strings is gone. issuer selection uses case [str() as issuer], and claimed-handle lookup stays inside the if handle short-circuit. handle selection scans aliases directly; PDS selection uses a mapping pattern and preserves the first matching entry. request helpers no longer repeat the transport's URL checks. HTTPS origin checks and browser-redirect validation remain, following the OAuth requirements.

86 tests and all local checks pass. i added the config, typing, and short-circuit lessons to AGENTS.md.

chappy.tngl.sh 1mo ago

ae20d59 gives the DID document and its services Pydantic schemas at the discovery boundary. _claimed_handle takes list[str]; _personal_data_server takes DIDDocument. both helpers now do only domain selection, with no casts or JSON shape checks. malformed records fail during parsing, and the claimed-handle lookup still runs only when a handle was supplied.

ATProto config, client metadata, protected-resource metadata, and server records also use Pydantic. config stays editable, and the existing metadata and pending-login JSON formats are preserved. f87a0fb parses stored OIDC claims through a schema while retaining the current client, expiry, and role checks.

all 104 tests and local checks pass. the configured account's live DID/PDS/authorization discovery also passed; i did not repeat the full sign-in flow. startup validation keeps credential-bearing configuration out of error output. i added the boundary-schema lesson to AGENTS.md.

chappy.tngl.sh 1mo ago

c854760 removes ResolvedHandle, accepts Iterable[str] for alias selection, and simplifies callback-state consumption. validation errors now list each affected top-level field once, without response values.

59e1a82 adds the shared Config.provider_config() and typed ProviderConfig return values. serialization happens at the allauth settings boundary. the adapter and backend use the requested control flow, and issuer validation uses pattern matching.

may_admin now takes OIDCClaims | None. stored JSON is parsed before the policy call; role grants contain organization IDs and string domains, following Zitadel's claim format. malformed grant values are rejected.

all 116 tests and local checks pass. both push and PR CI are green on 59e1a82. live handle resolution/discovery and the login error page passed too; no full OAuth sign-in was repeated.

chappy.tngl.sh 1mo ago

75fc211 assigns the DPoP nonce once before branching and keeps the authorization URL as httpx.URL until Django needs the redirect header.

JSON_DOCUMENT and the generic JsonObject schema are gone. HTTP now decodes JSON into object; the provider models validate their records. handle resolution and nonce errors use direct pattern matching at their response boundaries.

i kept require_https_url: the ATProto spec requires HTTPS origins for discovery, and the browser authorization redirect bypasses our HTTP transport. the validator now takes an existing URL directly and returns nothing. DNS pinning and bounded response reads remain.

all 121 tests and local checks pass, as does live handle/DID/PDS/authorization discovery. the push CI logs also show the checks and 121 tests passing on this commit; its final status is still running, and the PR run is queued. no full OAuth sign-in was repeated.

chappy.tngl.sh 1mo ago

both push and PR CI are now green on 75fc211.

chappy.tngl.sh 1mo ago

cbb24b7 completes the test pass. provider responses are editable fixture data, shared async setup lives in conftest.py, and one flow test owns the PKCE/key/nonce assertions. the view tests now use the production URL configuration. duplicate discovery checks and repeated library type cases are gone.

the added cases cover independent identity bindings, expiry boundaries, bounded nonce retries, DNS failure and mixed public/private answers, response-size limits, account revocation, and privacy. assertions use actual private values and independent expected URLs. the backend test also checks admission after an earlier denied account; startup tests exercise the ASGI entry point and login_redirects with providers enabled and disabled.

i preserved the cleanup structure and fixed the regressions the tests exposed: the SDK type needs Pydantic's arbitrary_types_allowed, sessions need JSON data, the alias generator must not overwrite the requested handle, Wagtail's catch-all must come last, disabled providers must return False, validation diagnostics must hide input credentials, and HTTPS origins must reject explicit port zero too.

local checks pass with 157 tests and 100% statement/branch coverage of hornroot, including startup subprocesses (615 statements, 104 branches). PR CI is also green on this exact commit. provider tests use simulated servers.

personhood.removal.surgery 1mo ago

よさげ！👍 お疲れ様♡

Versions