refactor(auth): nest internal packages under pkg/auth - #3338
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
PR SummaryLow Risk Overview Image builds for api and dashboard-api drop the extra Reviewed by Cursor Bugbot for commit d7660f3. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
LGTM — this is a mechanical package move (history-preserving) plus thin facade re-exports; no auth logic changed.
Extended reasoning...
Overview
The PR moves packages/auth/internal/* to packages/auth/pkg/auth/internal/* (renames only, import paths and otel tracer names updated to match), adds facade re-exports in pkg/auth/token.go for IdentityLookup, OIDCVerifier, ErrIdentityNotFound, and NewOIDCVerifier (all thin type aliases / one-line wrapper calls into the existing oidc package), and drops the now-redundant COPY ./auth/internal line from the api and dashboard-api Dockerfiles.
Security risks
None. No authentication or authorization logic changes — the OIDC verifier, JWT validation, team/ban/block checks, and caching behavior are untouched; only their package location and how they're re-exported changed. I confirmed no stale references to the old packages/auth/internal import path remain anywhere in the repo.
Level of scrutiny
Low. This is a textbook mechanical refactor: git mv-style renames plus facade re-exports that are all type aliases or single-line delegating functions. The Dockerfile change is a straightforward removal of a workaround line that's no longer needed now that internals live under pkg/.
Other factors
The PR description documents thorough testing (build/vet/test in the auth module, Docker build replication for both affected services, and simulation of an external consumer importing the facade). No outstanding review comments to address — the only timeline activity is automated summaries from Gemini/Cursor bots, no unresolved feedback.
3ebeb25 to
be22879
Compare
…facade External consumers (belt) previously imported pkg/auth/oidc directly (oidc.NewVerifier, oidc.IdentityLookup, oidc.ErrIdentityNotFound) and used auth.Verifier/auth.NewVerifier from the old facade. Both surfaces were removed in #3314, which breaks belt's daily infra sync at go mod tidy (module found, but does not contain package .../pkg/auth/oidc) and at compile time in argus-api. Expose the equivalents on the auth facade so all consumers use auth.*: - IdentityLookup, ErrIdentityNotFound - OIDCVerifier, NewOIDCVerifier (single issuer) - ProviderVerifier, NewProviderVerifier (multi-issuer, keeps the (nil, nil) no-provider semantics of the old auth.NewVerifier) Config/issuer types were already re-exported as JWTConfig/JWTIssuer.
Move packages/auth/internal/* to packages/auth/pkg/auth/internal/* so the implementation lives inside the consumer-facing pkg tree. Consumers keep importing the stable facade at .../packages/auth/pkg/auth. Go's internal visibility now scopes the implementation to pkg/auth alone (pkg/types and pkg/tests can no longer reach it), and image builds that COPY ./auth/pkg pick up the implementation for free — drop the separate ./auth/internal COPY that #3323 added to unbreak image builds after #3314.
be22879 to
57128f5
Compare
…facade (#3339) ## Summary Restores the auth surface external consumers lost in #3314, without touching the package layout. Belt imports this module and used `pkg/auth/oidc` (`oidc.NewVerifier`, `oidc.IdentityLookup`, `oidc.ErrIdentityNotFound`) plus the old facade's `auth.Verifier`/`auth.NewVerifier`. #3314 moved both behind Go `internal` packages, so belt's daily `sync-infra-repo` workflow fails at `go mod tidy`: > module …/packages/auth found, but does not contain package …/packages/auth/pkg/auth/oidc This PR re-exports the equivalents through the `auth.*` facade with identical signatures: - `auth.IdentityLookup`, `auth.ErrIdentityNotFound` - `auth.OIDCVerifier` / `auth.NewOIDCVerifier` (single issuer) - `auth.ProviderVerifier` / `auth.NewProviderVerifier` (multi-issuer; keeps the `(nil, nil)` no-provider semantics of the old `auth.NewVerifier`) Config/issuer types were already exposed as `auth.JWTConfig`/`auth.JWTIssuer`. Companion belt PR migrating its imports to the facade: e2b-dev/belt#1145. A follow-up PR (#3338) restructures the auth package layout separately. ## Testing - `go build ./… && go vet ./… && go test ./…` in `packages/auth` - `golangci-lint run ./packages/auth/…` — 0 issues - Belt compiled and tested against this commit (all 16 workspace modules build; `shared/pkg/auth`, `billing-server/internal/auth`, `argus-api/internal/handlers` green with `-race`) <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/e2b-dev/codesmith/infra/pr/3339"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787336953&installation_model_id=14389&pr_number=3339&repository=e2b-dev%2Finfra&return_to=https%3A%2F%2Fgithub.com%2Fe2b-dev%2Finfra%2Fpull%2F3339&signature=5551f98aca8e66952d8160dd7d79e3502f3e6cd0cc1c464e571035de4efef169"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>/codesmith</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer -->
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
…g tests (#3340) ## Summary `packages/dashboard-api/internal/provisioning` has not compiled on `main` since yesterday: #3327 removed read-replica support (and with it `authdb.Client.Read`), while #3328 — merged a few hours later — added provisioning tests calling `testDB.AuthDB.Read.GetDefaultTeamByUserID`. A semantic merge conflict CI didn't catch on either PR. Every `lint / golangci-lint (packages/dashboard-api)` and dashboard-api test job on PRs touching that module now fails with: > team_test.go:159:36: testDB.AuthDB.Read undefined (type *authdb.Client has no field or method Read) (e.g. https://github.com/e2b-dev/infra/actions/runs/29946947140/job/89014781860 on #3338). Fix: call the sqlc queries embedded on the client directly — the same style line 43 of the same file already uses. ## Testing - `go vet ./internal/provisioning/...` — passes (was typecheck-broken) - `golangci-lint run ./packages/dashboard-api/...` — 0 issues (was failing) - `go test ./internal/provisioning/ -count=1` against testcontainers — ok (11.9s) <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/e2b-dev/codesmith/infra/pr/3340"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787338978&installation_model_id=14389&pr_number=3340&repository=e2b-dev%2Finfra&return_to=https%3A%2F%2Fgithub.com%2Fe2b-dev%2Finfra%2Fpull%2F3340&signature=bd78b61c20b04a9ca1d584ba99a19910922124c2af5a4c2c7e58d346f0bfa436"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>/codesmith</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer -->
Summary
Stacked on #3339 (facade re-exports — the belt unblocker); this PR is the layout change only. Retarget to
mainafter #3339 merges.Moves
packages/auth/internal/*→packages/auth/pkg/auth/internal/*(history-preserving renames; import paths and otel tracer names updated). Consumers keep using the stable facade atgithub.laiyagushi.com/e2b-dev/infra/packages/auth/pkg/auth— nothing outside the auth module changes except two Dockerfile lines.Also removes the
COPY ./auth/internal ./auth/internallines from the api and dashboard-api Dockerfiles that #3323 added: with the implementation nested underpkg/auth, the existingCOPY ./auth/pkgcarries it, and the stale COPY would fail on a now-missing path.Motivation
#3314 placed implementation packages at
packages/auth/internal, outsidepkg/. The api/dashboard-api image builds copy the auth module selectively (COPY ./auth/pkg), so post-merge image builds broke and #3323 hot-fixed them with an extra COPY. This class of breakage is only detectable post-merge (PR CI builds from a full checkout; Dockerfiles build only inbuild-and-upload-images.yml), so every current and futurepkg-only copier must remember the extra line. Nesting internals underpkg/authmakes anypkg-only copy self-contained and deletes the failure mode structurally.It also tightens Go's internal boundary: only the
pkg/authfacade can reach the implementation now (previously any package underpackages/auth/, e.g.pkg/types/pkg/tests, could).Testing
go build ./… && go vet ./… && go test ./…inpackages/auth;golangci-lint run ./packages/auth/…— 0 issuesgo build ./packages/api/… ./packages/dashboard-api/…go.work,CGO_ENABLED=0 GOOS=linux): api and dashboard-api build OK — the exact scenario that broke after feat: add workspace admin API foundations #3314GOWORK=off, path replaces): facade import builds; direct import ofpkg/auth/internal/…is rejected by the compiler as intended