feat: multiple oauth providers#355
Conversation
WalkthroughReplaces hard-coded single-provider OAuth with runtime-configured multi-provider support (env/flags/files), surfaces provider display names through sessions and APIs, updates frontend for dynamic providers and an OAuth auto-redirect handover, and adds decoders, tests, migrations, and new icon assets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FE as Frontend (Login)
participant BE as Backend (Context API)
participant OB as OAuth Broker
Note over BE,OB: Runtime provider discovery (env/flags/files)
U->>FE: GET /login
FE->>BE: GET /api/context
BE-->>FE: {providers: [{id,name,oauth},...], oauthAutoRedirect}
alt oauthAutoRedirect enabled for provider P
FE->>OB: POST /api/oauth/start {providerId: P}
OB-->>FE: {authURL}
FE-->>U: Show handover UI (5s timer)
U->>OB: Navigate to authURL (auto or click)
else manual selection
U->>FE: Click provider button
FE->>OB: POST /api/oauth/start {providerId}
OB-->>FE: {authURL}
FE->>U: Redirect immediately
end
sequenceDiagram
autonumber
participant OP as OAuth Provider
participant OB as OAuth Broker
participant AS as Auth Service
participant DB as Database
participant MW as Context Middleware
participant FE as Frontend
OP->>OB: Callback (code,state)
OB->>AS: Verify code
AS->>DB: Insert session {oauth_name: service.GetName(), ...}
DB-->>AS: OK
AS-->>OB: Set-Cookie (session + OAuthName)
FE->>BE: Subsequent request with cookie
BE-->>FE: UserContext {oauthName, username, provider, ...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
==========================================
+ Coverage 22.61% 25.95% +3.33%
==========================================
Files 33 36 +3
Lines 2397 2524 +127
==========================================
+ Hits 542 655 +113
- Misses 1826 1833 +7
- Partials 29 36 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/context_controller.go (1)
71-97: Fix potential nil dereference: check GetContext error before accessing fieldsYou build the response using
context.*before checkingerr. Ifutils.GetContext(c)returns an error and a nil context, this will panic.Apply:
func (controller *ContextController) userContextHandler(c *gin.Context) { - context, err := utils.GetContext(c) - - userContext := UserContextResponse{ - Status: 200, - Message: "Success", - IsLoggedIn: context.IsLoggedIn, - Username: context.Username, - Name: context.Name, - Email: context.Email, - Provider: context.Provider, - OAuth: context.OAuth, - TotpPending: context.TotpPending, - OAuthName: context.OAuthName, - } - - if err != nil { - log.Debug().Err(err).Msg("No user context found in request") - userContext.Status = 401 - userContext.Message = "Unauthorized" - userContext.IsLoggedIn = false - c.JSON(200, userContext) - return - } - - c.JSON(200, userContext) + ctx, err := utils.GetContext(c) + if err != nil || ctx == nil { + log.Debug().Err(err).Msg("No user context found in request") + c.JSON(200, UserContextResponse{ + Status: 401, + Message: "Unauthorized", + IsLoggedIn: false, + }) + return + } + + userContext := UserContextResponse{ + Status: 200, + Message: "Success", + IsLoggedIn: ctx.IsLoggedIn, + Username: ctx.Username, + Name: ctx.Name, + Email: ctx.Email, + Provider: ctx.Provider, + OAuth: ctx.OAuth, + TotpPending: ctx.TotpPending, + OAuthName: ctx.OAuthName, + } + c.JSON(200, userContext) }internal/service/github_oauth_service.go (1)
52-56: Add HTTP client timeouts to prevent indefinite hangsOAuth and userinfo calls shouldn’t rely on a client without a timeout.
func (github *GithubOAuthService) Init() error { - httpClient := &http.Client{} + httpClient := &http.Client{Timeout: 10 * time.Second} ctx := context.Background() ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) verifier := oauth2.GenerateVerifier()
🧹 Nitpick comments (29)
internal/utils/decoders/label_decoder_test.go (1)
66-67: DeepEqual arg order (actual, expected).
Use actual first, expected second for clearer diffs: assert.DeepEqual(t, result, expected). This matches the pkg docs signature func DeepEqual(t, x, y ...). (pkg.go.dev)- assert.DeepEqual(t, expected, result) + assert.DeepEqual(t, result, expected)frontend/src/components/icons/tailscale.tsx (1)
7-7: Drop xmlSpace attribute (unused).
Reduces noise; not needed for inline SVG.- xmlSpace="preserve"frontend/src/components/icons/microsoft.tsx (1)
7-9: Default to 1em for inline sizing (optional).
Using em-based defaults tends to fit better inside buttons; callers can still override.- width="2em" - height="2em" + width="1em" + height="1em"frontend/src/pages/logout-page.tsx (1)
18-18: Guard against missing oauthName to avoid 'undefined' in UIIf backend omits oauthName for an OAuth session, the subtitle will render "undefined". Add a fallback to the provider id.
- values={{ - username: email, - provider: oauthName, - }} + values={{ + username: email, + provider: oauthName ?? provider, + }}Also applies to: 66-68
frontend/src/schemas/app-context-schema.ts (1)
3-7: Tighten provider schema (non-empty, safe id)Prevent empty strings and enforce a stable id format for mapping and icons.
-export const providerSchema = z.object({ - id: z.string(), - name: z.string(), - oauth: z.boolean(), -}); +export const providerSchema = z.object({ + id: z.string().min(1, "id required").regex(/^[a-z0-9-]+$/i, "use alphanumerics and hyphens"), + name: z.string().min(1, "name required"), + oauth: z.boolean(), +});internal/config/config.go (1)
169-171: Add explicit tags for decoders to reduce coupling to field namesMakes flags/env decoders resilient and self-documenting.
type Providers struct { - Providers map[string]OAuthServiceConfig + Providers map[string]OAuthServiceConfig `mapstructure:"providers" flag:"providers"` }internal/service/oauth_broker_service.go (3)
47-54: Fix log formatting: %T prints 'string', not the provider nameUse %s to print the provider key.
- if err != nil { - log.Error().Err(err).Msgf("Failed to initialize OAuth service: %T", name) - return err - } - log.Info().Msgf("Initialized OAuth service: %T", name) + if err != nil { + log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) + return err + } + log.Info().Msgf("Initialized OAuth service: %s", name)
3-9: Unify slices import to stdlibYou already use the stdlib "slices" elsewhere; prefer it here to drop x/exp.
-import ( +import ( "errors" "tinyauth/internal/config" "github.com/rs/zerolog/log" - "golang.org/x/exp/slices" + "slices" )Also applies to: 64-65
25-30: Pre-size services mapMinor allocation win when many providers are configured.
func NewOAuthBrokerService(configs map[string]config.OAuthServiceConfig) *OAuthBrokerService { return &OAuthBrokerService{ - services: make(map[string]OAuthService), + services: make(map[string]OAuthService, len(configs)), configs: configs, } }internal/utils/decoders/flags_decoder.go (1)
119-137: Clarify/validate provider id constraintsFlags like --providers-my-awesome-client-... aren’t supported; either validate and error early or document clearly.
Would you like me to add an explicit validation (e.g., provider id must match ^[a-z0-9-]+$ without extra dashes) and update the docs?
internal/controller/context_controller.go (1)
100-108: Preserve AppURL path; avoid rewriting to scheme://hostIf AppURL includes a path (e.g., behind a reverse-proxy), dropping it can break clients. Use the parsed URL as-is.
- AppURL: fmt.Sprintf("%s://%s", appUrl.Scheme, appUrl.Host), + AppURL: appUrl.String(),internal/controller/context_controller_test.go (1)
35-45: Add an assertion for oauthName propagationSince the API now exposes
oauthName, add a case asserting it’s present when set in context.var userContext = config.UserContext{ Username: "testuser", Name: "testuser", Email: "test@example.com", IsLoggedIn: true, OAuth: false, Provider: "username", TotpPending: false, OAuthGroups: "", TotpEnabled: false, + OAuthName: "Local", } func TestUserContextHandler(t *testing.T) { expectedRes := controller.UserContextResponse{ Status: 200, Message: "Success", IsLoggedIn: userContext.IsLoggedIn, Username: userContext.Username, Name: userContext.Name, Email: userContext.Email, Provider: userContext.Provider, OAuth: userContext.OAuth, TotpPending: userContext.TotpPending, + OAuthName: userContext.OAuthName, }Also applies to: 95-105
frontend/src/pages/continue-page.tsx (1)
50-53: Guard against null redirectUriObj in handleRedirectNon-null assertion can still crash at runtime in edge cases; add a quick guard.
const handleRedirect = () => { - setLoading(true); - window.location.assign(redirectUriObj!.toString()); + if (!redirectUriObj) return; + setLoading(true); + window.location.assign(redirectUriObj.toString()); };internal/controller/oauth_controller.go (1)
109-114: Improve CSRF mismatch loggingAvoid logging a nil error on pure mismatches; log clearer branches.
- if err != nil || state != csrfCookie { - log.Warn().Err(err).Msg("CSRF token mismatch or cookie missing") + if err != nil || state != csrfCookie { + if err != nil { + log.Warn().Err(err).Msg("CSRF cookie missing") + } else { + log.Warn().Msg("CSRF token mismatch") + } c.SetCookie(controller.config.CSRFCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true) c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL)) return }internal/service/google_oauth_service.go (1)
31-32: Expose provider display name — consider a safe fallback.If Name is unset in config, return a sensible default to keep UI stable.
func (google *GoogleOAuthService) GetName() string { - return google.name + if google.name != "" { + return google.name + } + return "Google" }Also applies to: 43-44, 117-119
internal/utils/decoders/flags_decoder_test.go (1)
11-60: Good positive test; add a precedence test (flags override env).Add a subtest mixing env+flags to assert flag values win.
// outside current test func TestProviders_FlagsOverrideEnv(t *testing.T) { env := []string{ "TINYAUTH_PROVIDERS_CLIENT1_CLIENT_ID=env-id", "TINYAUTH_PROVIDERS_CLIENT1_CLIENT_SECRET=env-secret", } args := []string{ "tinyauth", "--providers-client1-client-id=flag-id", } // use app_utils to exercise full path (env+flags merge) got, err := utils.GetOAuthProvidersConfig(env, args, "https://app.example.com") assert.NilError(t, err) assert.Equal(t, got["client1"].ClientID, "flag-id") assert.Equal(t, got["client1"].ClientSecret, "env-secret") }internal/utils/decoders/env_decoder_test.go (2)
37-54: Add cases for TokenURL and initialism/casing edge keys.To harden decoder compatibility, also assert mapping for:
- TOKEN_URL (expects TokenURL)
- USERINFO_URL (single word) vs USER_INFO_URL (two words) both landing on UserinfoURL
- INSECURE_SKIP_VERIFY=true
Apply this minimal augmentation:
test := map[string]string{ @@ - "PROVIDERS_CLIENT1_USER_INFO_URL": "client1-user-info-url", + "PROVIDERS_CLIENT1_USER_INFO_URL": "client1-user-info-url", + "PROVIDERS_CLIENT1_TOKEN_URL": "client1-token-url", @@ expected := config.Providers{ Providers: map[string]config.OAuthServiceConfig{ "client1": { @@ - UserinfoURL: "client1-user-info-url", + UserinfoURL: "client1-user-info-url", + TokenURL: "client1-token-url",
56-60: Consider adding a negative test for provider IDs with underscores.Currently, providers like PROVIDERS_MY_AWESOME_CLIENT_* are likely to mis-decode (see decoder comment). Add one test asserting proper handling or explicit error until decoder is fixed.
cmd/root.go (2)
55-55: UnknownFlags=true can hide genuine typos in user flags.This is useful to pass through dynamic --providers-* flags, but it will also swallow misspelled core flags. Consider validating and warning on unknown flags that don't start with the expected providers prefix.
Example approach: scan os.Args for --providers-* here and only tolerate those; fail others.
96-96: Help text still lists fixed providers.With dynamic providers, prefer “Auto redirect to the specified configured provider id.” to avoid misleading users.
Apply this tweak:
-{"oauth-auto-redirect", "none", "Auto redirect to the specified OAuth provider if configured. (available providers: github, google, generic)"}, +{"oauth-auto-redirect", "none", "Auto redirect to the specified configured OAuth provider id (e.g. google, github, client1)."},internal/utils/app_utils_test.go (1)
205-271: Solid happy-path tests; add a few more scenarios.Suggestions:
- Merge precedence: flags should override env for same provider key.
- Scopes parsing from flags (e.g., --providers-client1-scopes=a,b).
- Name and InsecureSkipVerify fields.
Apply a small extra case for precedence:
env := []string{"PROVIDERS_CLIENT1_CLIENT_ID=env-id", "PROVIDERS_CLIENT1_CLIENT_SECRET=env-secret"} args := []string{"/tinyauth/tinyauth", "--providers-client1-client-id=flag-id", "--providers-client1-client-secret=flag-secret"} @@ expected := map[string]config.OAuthServiceConfig{ "client1": {ClientID: "flag-id", ClientSecret: "flag-secret"}, }internal/utils/decoders/env_decoder.go (1)
119-137: Comment fix and initialism mapping caveat.
- The comment says “client_clientId” but code produces “…_client1_clientId”. Update comment.
- Be aware normalizeEnv yields userInfoUrl/tokenUrl; struct fields are UserinfoURL/TokenURL. Parser must be forgiving on URL initialism; otherwise consider a special-case to keep “URL” uppercased.
Suggested comment tweak:
-// normalizeEnv converts env vars from PROVIDERS_CLIENT1_CLIENT_ID to tinyauth_providers_client_clientId +// normalizeEnv converts env vars from PROVIDERS_CLIENT1_CLIENT_ID to tinyauth_providers_client1_clientIdIf needed, special-case URL:
- fkb += strings.ToUpper(string([]rune(s)[0])) + string([]rune(s)[1:]) + part := strings.ToUpper(string([]rune(s)[0])) + string([]rune(s)[1:]) + if strings.EqualFold(s, "url") { part = "URL" } + fkb += partfrontend/src/pages/login-page.tsx (3)
75-81: Clear the handover timer and reset state on OAuth error.Prevents stale timers and a latent “showRedirectButton=true” after failures.
Apply:
onError: () => { - setOauthAutoRedirectHandover(false); + setOauthAutoRedirectHandover(false); + if (redirectButtonTimer.current) { + clearTimeout(redirectButtonTimer.current); + redirectButtonTimer.current = null; + } + setShowRedirectButton(false); toast.error(t("loginOauthFailTitle"), { description: t("loginOauthFailSubtitle"), }); },
153-177: Expose manual redirect button state to a11y.Optional: add aria-busy to the CardFooter while waiting, and disable the button if oauthMutation is pending.
-<CardFooter className="flex flex-col items-stretch"> +<CardFooter className="flex flex-col items-stretch" aria-busy={oauthMutation.isPending}> <Button onClick={() => { window.location.replace(oauthMutation.data?.data.url); }} + disabled={oauthMutation.isPending} >
218-221: Use strict equality.Prefer === to avoid accidental coercion.
-{providers.length == 0 && ( +{providers.length === 0 && (internal/bootstrap/app_bootstrap.go (4)
49-55: Add error context when loading OAuth providers.Wrap the returned error for clearer diagnostics.
- oauthProviders, err := utils.GetOAuthProvidersConfig(os.Environ(), os.Args, app.Config.AppURL) - if err != nil { - return err - } + oauthProviders, err := utils.GetOAuthProvidersConfig(os.Environ(), os.Args, app.Config.AppURL) + if err != nil { + return fmt.Errorf("failed to load OAuth providers config from env/flags: %w", err) + }
143-147: DRY known-provider friendly names.This map duplicates knowledge also used in utils for “babysitting.” Consider centralizing friendly-name mapping (e.g., utils.FriendlyProviderName or a shared const map) to avoid divergence.
170-174: Username provider append looks good.Optional: if only LDAP is enabled (no local users), consider labeling “LDAP” for clarity; keep ID “username” for compatibility.
Does the frontend icon set handle a non-”Username” label without breaking layout?
147-167: Make provider order deterministic and trim/display-safe.Sort provider IDs case-insensitively, trim provider.Name, and preallocate the slice; no code was found that depends on map iteration order.
- configuredProviders := make([]controller.Provider, 0) - - for id, provider := range oauthProviders { - if id == "" { - continue - } - - if provider.Name == "" { - if name, ok := babysit[id]; ok { - provider.Name = name - } else { - provider.Name = utils.Capitalize(id) - } - } - - configuredProviders = append(configuredProviders, controller.Provider{ - Name: provider.Name, - ID: id, - OAuth: true, - }) - } + configuredProviders := make([]controller.Provider, 0, len(oauthProviders)) + + ids := make([]string, 0, len(oauthProviders)) + for id := range oauthProviders { + if id != "" { + ids = append(ids, id) + } + } + sort.Slice(ids, func(i, j int) bool { + return strings.ToLower(ids[i]) < strings.ToLower(ids[j]) + }) + + for _, id := range ids { + provider := oauthProviders[id] + name := strings.TrimSpace(provider.Name) + if name == "" { + if bName, ok := babysit[id]; ok { + name = bName + } else { + name = utils.Capitalize(id) + } + } + configuredProviders = append(configuredProviders, controller.Provider{ + Name: name, + ID: id, + OAuth: true, + }) + }Add missing import:
import ( "fmt" "net/url" "os" + "sort" "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
cmd/root.go(2 hunks)frontend/src/components/icons/microsoft.tsx(1 hunks)frontend/src/components/icons/oauth.tsx(1 hunks)frontend/src/components/icons/pocket-id.tsx(1 hunks)frontend/src/components/icons/tailscale.tsx(1 hunks)frontend/src/lib/i18n/locales/en-US.json(1 hunks)frontend/src/lib/i18n/locales/en.json(1 hunks)frontend/src/pages/continue-page.tsx(1 hunks)frontend/src/pages/login-page.tsx(6 hunks)frontend/src/pages/logout-page.tsx(2 hunks)frontend/src/schemas/app-context-schema.ts(1 hunks)frontend/src/schemas/user-context-schema.ts(1 hunks)internal/assets/migrations/000002_oauth_name.down.sql(1 hunks)internal/assets/migrations/000002_oauth_name.up.sql(1 hunks)internal/bootstrap/app_bootstrap.go(5 hunks)internal/config/config.go(5 hunks)internal/controller/context_controller.go(3 hunks)internal/controller/context_controller_test.go(2 hunks)internal/controller/oauth_controller.go(1 hunks)internal/middleware/context_middleware.go(1 hunks)internal/model/session_model.go(1 hunks)internal/service/auth_service.go(2 hunks)internal/service/generic_oauth_service.go(3 hunks)internal/service/github_oauth_service.go(3 hunks)internal/service/google_oauth_service.go(3 hunks)internal/service/oauth_broker_service.go(1 hunks)internal/utils/app_utils.go(2 hunks)internal/utils/app_utils_test.go(2 hunks)internal/utils/decoders/env_decoder.go(1 hunks)internal/utils/decoders/env_decoder_test.go(1 hunks)internal/utils/decoders/flags_decoder.go(1 hunks)internal/utils/decoders/flags_decoder_test.go(1 hunks)internal/utils/decoders/label_decoder_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
frontend/src/pages/continue-page.tsx (1)
frontend/src/pages/totp-page.tsx (2)
redirectTimer(53-55)redirectTimer(53-55)
internal/utils/decoders/flags_decoder_test.go (2)
internal/config/config.go (2)
Providers(169-171)OAuthServiceConfig(53-64)internal/utils/decoders/flags_decoder.go (1)
DecodeFlags(15-41)
internal/utils/app_utils.go (4)
internal/config/config.go (2)
OAuthServiceConfig(53-64)Providers(169-171)internal/utils/decoders/env_decoder.go (1)
DecodeEnv(15-41)internal/utils/decoders/flags_decoder.go (1)
DecodeFlags(15-41)internal/utils/security_utils.go (1)
GetSecret(13-28)
frontend/src/components/icons/pocket-id.tsx (4)
frontend/src/components/icons/generic.tsx (1)
GenericIcon(3-24)frontend/src/components/icons/github.tsx (1)
GithubIcon(3-18)frontend/src/components/icons/google.tsx (1)
GoogleIcon(3-30)frontend/src/components/ui/oauth-button.tsx (1)
Props(6-11)
internal/utils/decoders/env_decoder_test.go (2)
internal/config/config.go (2)
Providers(169-171)OAuthServiceConfig(53-64)internal/utils/decoders/env_decoder.go (1)
DecodeEnv(15-41)
frontend/src/components/icons/microsoft.tsx (3)
frontend/src/components/icons/google.tsx (1)
GoogleIcon(3-30)frontend/src/components/icons/github.tsx (1)
GithubIcon(3-18)frontend/src/components/icons/generic.tsx (1)
GenericIcon(3-24)
frontend/src/components/icons/oauth.tsx (2)
frontend/src/components/icons/generic.tsx (1)
GenericIcon(3-24)frontend/src/components/ui/oauth-button.tsx (1)
Props(6-11)
internal/controller/context_controller.go (1)
internal/config/config.go (1)
Providers(169-171)
frontend/src/lib/i18n/locales/en.json (1)
frontend/src/components/ui/oauth-button.tsx (1)
Props(6-11)
frontend/src/pages/logout-page.tsx (1)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)
internal/utils/decoders/flags_decoder.go (1)
internal/config/config.go (1)
Providers(169-171)
internal/utils/app_utils_test.go (2)
internal/config/config.go (1)
OAuthServiceConfig(53-64)internal/utils/app_utils.go (1)
GetOAuthProvidersConfig(137-198)
frontend/src/pages/login-page.tsx (9)
frontend/src/components/icons/tailscale.tsx (1)
TailscaleIcon(3-24)frontend/src/components/icons/microsoft.tsx (1)
MicrosoftIcon(3-18)frontend/src/components/icons/pocket-id.tsx (1)
PocketIDIcon(3-20)frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/components/ui/card.tsx (7)
Card(85-85)CardHeader(86-86)CardTitle(88-88)CardDescription(90-90)CardFooter(87-87)CardContent(91-91)CardFooter(74-82)frontend/src/components/ui/oauth-button.tsx (2)
OAuthButton(13-33)Props(6-11)frontend/src/components/icons/oauth.tsx (1)
OAuthIcon(3-24)frontend/src/pages/totp-page.tsx (1)
useUserContext(20-83)
internal/bootstrap/app_bootstrap.go (5)
internal/utils/app_utils.go (1)
GetOAuthProvidersConfig(137-198)internal/config/config.go (2)
Config(17-42)Providers(169-171)internal/service/oauth_broker_service.go (1)
NewOAuthBrokerService(25-30)internal/controller/context_controller.go (1)
Provider(37-41)internal/utils/string_utils.go (1)
Capitalize(7-12)
internal/controller/context_controller_test.go (1)
internal/controller/context_controller.go (1)
Provider(37-41)
internal/utils/decoders/env_decoder.go (1)
internal/config/config.go (1)
Providers(169-171)
frontend/src/components/icons/tailscale.tsx (3)
frontend/src/components/icons/github.tsx (1)
GithubIcon(3-18)frontend/src/components/icons/generic.tsx (1)
GenericIcon(3-24)frontend/src/components/icons/google.tsx (1)
GoogleIcon(3-30)
🔇 Additional comments (26)
internal/utils/decoders/label_decoder_test.go (1)
8-8: Switch to gotest.tools/assert looks good.
Consistent with the rest of the suite and improves failure messages.frontend/src/components/icons/oauth.tsx (1)
3-3: No action required — OAuthIcon rename verified.
oauth.tsx exports OAuthIcon and frontend/src/pages/login-page.tsx imports/uses OAuthIcon; ripgrep found no occurrences of GenericIcon, so no compatibility alias needed.internal/assets/migrations/000002_oauth_name.down.sql (1)
1-1: Down migration looks good.
Symmetric with the up migration.frontend/src/schemas/user-context-schema.ts (1)
11-11: Keep oauthName required — backend always returns oauthNameVerified: internal/controller/context_controller.go defines OAuthName string
json:"oauthName"(noomitempty) and the handler returns that struct on both success and unauthorized paths, so the JSON will always include oauthName (empty string if unset). Locations: internal/controller/context_controller.go, frontend/src/schemas/user-context-schema.ts, frontend/src/context/user-context.tsx.frontend/src/components/icons/pocket-id.tsx (1)
13-17: LGTM.
Black circle + white mark ensures visibility on light backgrounds; props are forwarded correctly.internal/model/session_model.go (1)
12-13: LGTM: schema matches migration and propagation pathField addition aligns with the migration and context exposure.
internal/controller/context_controller.go (1)
37-45: Provider type and config injection look goodThe new Provider struct and propagation via controller config cleanly model multi-provider state. JSON shape is sane and backwards changes are intentional.
internal/controller/context_controller_test.go (1)
15-26: LGTM: Tests updated for typed providersSwitching to []controller.Provider in both config and expectations matches the new public API.
frontend/src/pages/continue-page.tsx (1)
70-74: Confirm 5s reveal UXTimeout increased to 5s before showing the manual button. Confirm this aligns with the new auto-redirect handover spec; otherwise users may wait longer than necessary when redirects fail.
internal/controller/oauth_controller.go (1)
189-190: Good: Persisting OAuthName into the session cookieStoring
OAuthNamefrom the service ensures the UI can display a friendly provider name consistently.internal/service/github_oauth_service.go (1)
36-37: LGTM: Provider name plumbed and exposed via GetName()The added field and accessor align with the interface and are used by the controller to set OAuthName.
Also applies to: 173-175
internal/service/auth_service.go (1)
213-214: Approve — OAuthName propagation verified; migration & model present.Session persistence and cookie hydration include OAuthName; aligns with the new model and context usage. Found internal/model/session_model.go (OAuthName field) and internal/assets/migrations/000002_oauth_name.up.sql/.down.sql (adds/drops column and seeds existing rows). Ensure the migration has been applied in all target environments before rollout.
internal/service/generic_oauth_service.go (1)
25-26: LGTM on name wiring.Getter and constructor plumbing are consistent with the interface and other providers.
Also applies to: 42-43, 121-123
frontend/src/lib/i18n/locales/en.json (1)
17-19: Copy and tone read well.Keys are consistent with existing naming; punctuation and casing look good.
internal/middleware/context_middleware.go (1)
98-99: LGTM: OAuthName added to context.Matches session cookie shape; downstream UI can render provider display name.
internal/utils/app_utils.go (1)
9-11: No changes required — go.mod sets Go 1.24.0, which covers maps.Copy (>=1.21) and fmt.Appendf.go.mod: go 1.24.0; imports: internal/utils/app_utils.go ("maps"); fmt.Appendf usages: internal/service/google_oauth_service.go:62, internal/service/github_oauth_service.go:67, internal/service/generic_oauth_service.go:72.
frontend/src/lib/i18n/locales/en-US.json (1)
17-19: LGTM; matches base locale.Strings mirror en.json and support the new auto-redirect UI.
internal/utils/decoders/env_decoder_test.go (1)
11-36: Good baseline coverage for multi-provider env decoding.Covers two providers, scopes parsing, name, and booleans. Looks consistent with config.OAuthServiceConfig.
cmd/root.go (1)
96-96: Trusted proxies description improvement looks good.internal/utils/app_utils_test.go (1)
4-4: Import addition is appropriate.frontend/src/pages/login-page.tsx (2)
30-36: Icon map with sensible fallback.Good UX: known icons per id with OAuthIcon fallback.
114-129: Auto-redirect guard conditions look right.Checks: providers available, id exists, not logged in, and redirect_uri present. Nice.
internal/bootstrap/app_bootstrap.go (4)
6-6: LGTM on import.
123-123: Constructor change wired correctly.
177-177: Provider list debug log is helpful.No secrets included; safe at debug level.
218-218: Context config migration to Providers looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/assets/migrations/000002_oauth_name.up.sql (1)
1-8: Wrap in a transaction if your migrator doesn't do it.Prevents a half-applied state between ALTER and UPDATE.
If not auto-wrapped by your migration runner, enclose the statements in BEGIN/COMMIT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/icons/tailscale.tsx(1 hunks)internal/assets/migrations/000002_oauth_name.up.sql(1 hunks)internal/utils/app_utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/icons/tailscale.tsx
- internal/utils/app_utils.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/utils/decoders/flags_decoder.go (1)
24-29: More robust flag trimming.TrimPrefix("--") misses single-dash or accidental triple-dash variants. Prefer TrimLeft to strip any number of leading dashes.
- for k, v := range flags { - filtered[strings.TrimPrefix(k, "--")] = v - } + for k, v := range flags { + filtered[strings.TrimLeft(k, "-")] = v + }internal/utils/decoders/decoders.go (3)
34-45: Guard against empty provider IDs.If the key is like PROVIDERS__CLIENT_ID, camelClientName ends up empty, producing tinyauth.providers..clientId.
clientNameParts := strings.Split(strings.TrimPrefix(strings.TrimSuffix(cebabKey, sep+strings.ReplaceAll(suffix, "-", sep)), "providers"+sep), sep) + // Skip if no provider token + if len(clientNameParts) == 0 || clientNameParts[0] == "" { + continue + }
49-60: Nit: typos and readability.filedParts → fieldParts; cebabKey → lowerKey (or kebabKey). No behavior change, but avoids confusion.
- filedParts := strings.Split(suffix, "-") + fieldParts := strings.Split(suffix, "-") ... - cebabKey := strings.ToLower(k) + lowerKey := strings.ToLower(k)And update references accordingly.
13-21: Micro: avoid repeated string concatenations.Using strings.Builder (or pre-sizing slices and Join) will reduce allocations in tight loops.
internal/utils/decoders/decoders_test.go (1)
10-44: Solid coverage; add a couple of edge cases.Consider tests for:
- name and insecure-skip-verify keys.
- a provider with consecutive separators (e.g., PROVIDERS__X__CLIENT_ID) to verify the empty‑provider guard.
internal/config/config.go (2)
90-101: UserContext OAuthName: API surface check.If this is serialized to clients, confirm casing is acceptable (OAuthName vs oauthName) or add json tags for stability.
169-171: Providers wrapper type: optional mapstructure tag.If any non-paerser decoding path exists, add mapstructure:"providers" for symmetry.
type Providers struct { - Providers map[string]OAuthServiceConfig + Providers map[string]OAuthServiceConfig `mapstructure:"providers"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/config/config.go(5 hunks)internal/utils/decoders/decoders.go(1 hunks)internal/utils/decoders/decoders_test.go(1 hunks)internal/utils/decoders/env_decoder.go(1 hunks)internal/utils/decoders/flags_decoder.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/utils/decoders/decoders_test.go (1)
internal/utils/decoders/decoders.go (1)
NormalizeKeys(9-67)
internal/utils/decoders/flags_decoder.go (2)
internal/config/config.go (1)
Providers(169-171)internal/utils/decoders/decoders.go (1)
NormalizeKeys(9-67)
internal/utils/decoders/env_decoder.go (2)
internal/config/config.go (1)
Providers(169-171)internal/utils/decoders/decoders.go (1)
NormalizeKeys(9-67)
internal/utils/decoders/decoders.go (1)
internal/config/config.go (1)
OAuthServiceConfig(53-64)
🔇 Additional comments (4)
internal/utils/decoders/env_decoder.go (1)
9-20: LGTM: env decoding pipeline is clean and composable.NormalizeKeys + parser.Decode is the right shape for multi‑provider envs.
Please confirm we actually need both roots in Decode (tinyauth, tinyauth.providers). If only tinyauth is required, drop the second to reduce ambiguity.
internal/utils/decoders/flags_decoder.go (1)
10-22: LGTM: flags → normalized keys → parser.Decode flow.Consistent with the env decoder; easy to maintain.
internal/config/config.go (2)
53-64: Key tags look consistent; confirm UserinfoURL tag.user-info-url matches the tag schema; just confirm downstream expects “user-info-url”, not “userinfo-url”.
80-88: SessionCookie OAuthName addition: check persistence/migrations.Assuming migrations wire this field, looks good. Verify JSON exposure isn’t required here (no json tags).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/assets/migrations/000002_oauth_name.up.sql (1)
1-9: Wrap in a transaction if your migrator doesn’t.The prior concern stands: ADD COLUMN + UPDATE should be atomic if the runner doesn’t auto-wrap.
BEGIN; -- statements... COMMIT;
🧹 Nitpick comments (4)
internal/assets/migrations/000002_oauth_name.up.sql (4)
7-7: Prefer INITCAP with normalization over manual SUBSTR/UPPER.This yields better display names for values like 'GITHUB', 'azure_ad', or ' okta '. Example refactor:
- ELSE UPPER(SUBSTR("provider", 1, 1)) || SUBSTR("provider", 2) + ELSE INITCAP(REGEXP_REPLACE(TRIM("provider"), '[_\\-\\s]+', ' ', 'g'))
3-9: Guard against empty/whitespace-only providers.As written, provider='' would backfill oauth_name to ''. Trim and NULL-check to avoid junk values.
-WHERE "oauth_name" IS NULL AND "provider" IS NOT NULL; +WHERE "oauth_name" IS NULL + AND NULLIF(TRIM("provider"), '') IS NOT NULL;
4-6: Trim before case-insensitive comparisons.Covers stray whitespace from prior data.
- WHEN LOWER("provider") = 'github' THEN 'GitHub' - WHEN LOWER("provider") = 'google' THEN 'Google' + WHEN LOWER(TRIM("provider")) = 'github' THEN 'GitHub' + WHEN LOWER(TRIM("provider")) = 'google' THEN 'Google'
7-7: Postgres function form consistency.If you keep the manual capitalization path, prefer SUBSTRING(...) or verify SUBSTR two-arg support in your target PG version. Using INITCAP above sidesteps this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/assets/migrations/000002_oauth_name.up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/assets/migrations/000002_oauth_name.up.sql (1)
3-9: Good preservation of historical provider names.CASE mapping for GitHub/Google and leaving existing non-null oauth_name untouched looks correct.
1 participant
Fixes #62
Summary by CodeRabbit
New Features
Improvements
Chores