Skip to content

Commit 8b76b8d

Browse files
fix: address CodeRabbit review feedback for OIDC support
- Add nil guards for OIDCOAuthConfig/OIDCProvider in both handlers to prevent nil pointer panic when OIDC is disabled - Move AllowSignup check from start handler to CreateUser path so existing OIDC users can still log in when signup is disabled - Require email_verified before authorizing or linking by email - Add UserInfo endpoint fallback when ID token lacks optional claims (email, name, email_verified) - Normalize BaseURL with TrimRight to prevent double-slash redirect URIs - Add 30s timeout context for OIDC discovery at startup - Enforce 'openid' scope is always present in configured scopes - Add ScopesString field for reliable env var binding (matching existing ServicesString/AllowedOriginsString pattern)
1 parent dc4c089 commit 8b76b8d

5 files changed

Lines changed: 65 additions & 4 deletions

File tree

api/v1/server/handlers/users/oidc_oauth_callback.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import (
2121

2222
// Note: we want all errors to redirect, otherwise the user will be greeted with raw JSON in the middle of the login flow.
2323
func (u *UserService) UserUpdateOidcOauthCallback(ctx echo.Context, _ gen.UserUpdateOidcOauthCallbackRequestObject) (gen.UserUpdateOidcOauthCallbackResponseObject, error) {
24+
if u.config.Auth.OIDCOAuthConfig == nil || u.config.Auth.OIDCProvider == nil {
25+
return nil, redirect.GetRedirectWithError(ctx, u.config.Logger, nil, "OIDC authentication is not configured.")
26+
}
27+
2428
isValid, _, err := authn.NewSessionHelpers(u.config.SessionStore).ValidateOAuthState(ctx, "oidc")
2529

2630
if err != nil || !isValid {
@@ -79,6 +83,10 @@ func (u *UserService) upsertOIDCUserFromToken(ctx context.Context, config *serve
7983
return nil, err
8084
}
8185

86+
if !claims.EmailVerified {
87+
return nil, fmt.Errorf("OIDC provider did not verify the email address")
88+
}
89+
8290
expiresAt := tok.Expiry
8391

8492
accessTokenEncrypted, err := config.Encryption.Encrypt([]byte(tok.AccessToken), "oidc_access_token")
@@ -118,6 +126,10 @@ func (u *UserService) upsertOIDCUserFromToken(ctx context.Context, config *serve
118126
return nil, fmt.Errorf("failed to update user: %s", err.Error())
119127
}
120128
case pgx.ErrNoRows:
129+
if !config.Runtime.AllowSignup {
130+
return nil, fmt.Errorf("user signup is disabled")
131+
}
132+
121133
user, err = u.config.V1.User().CreateUser(ctx, &v1.CreateUserOpts{
122134
Email: claims.Email,
123135
EmailVerified: v1.BoolPtr(claims.EmailVerified),
@@ -162,6 +174,29 @@ func getOIDCClaimsFromToken(ctx context.Context, config *server.ServerConfig, to
162174
return nil, fmt.Errorf("failed to parse ID token claims: %s", err.Error())
163175
}
164176

177+
// Fall back to UserInfo endpoint when the ID token is missing optional claims.
178+
// The OIDC spec does not require providers to include email/name in the ID token.
179+
if claims.Email == "" || claims.Name == "" || !claims.EmailVerified {
180+
userInfo, uiErr := config.Auth.OIDCProvider.UserInfo(ctx, oauth2.StaticTokenSource(tok))
181+
if uiErr == nil {
182+
uiClaims := &oidcClaims{}
183+
if err := userInfo.Claims(uiClaims); err == nil {
184+
if claims.Email == "" {
185+
claims.Email = uiClaims.Email
186+
}
187+
if claims.Name == "" {
188+
claims.Name = uiClaims.Name
189+
}
190+
if !claims.EmailVerified && uiClaims.EmailVerified {
191+
claims.EmailVerified = true
192+
}
193+
if claims.Sub == "" {
194+
claims.Sub = uiClaims.Sub
195+
}
196+
}
197+
}
198+
}
199+
165200
if claims.Email == "" {
166201
return nil, fmt.Errorf("OIDC provider did not return an email claim")
167202
}

api/v1/server/handlers/users/oidc_oauth_start.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ import (
1010

1111
// Note: we want all errors to redirect, otherwise the user will be greeted with raw JSON in the middle of the login flow.
1212
func (u *UserService) UserUpdateOidcOauthStart(ctx echo.Context, _ gen.UserUpdateOidcOauthStartRequestObject) (gen.UserUpdateOidcOauthStartResponseObject, error) {
13-
if !u.config.Runtime.AllowSignup {
14-
return nil, redirect.GetRedirectWithError(ctx, u.config.Logger, nil, "User signup is disabled.")
13+
if u.config.Auth.OIDCOAuthConfig == nil {
14+
return nil, redirect.GetRedirectWithError(ctx, u.config.Logger, nil, "OIDC authentication is not configured.")
1515
}
1616

1717
state, err := authn.NewSessionHelpers(u.config.SessionStore).SaveOAuthState(ctx, "oidc")

pkg/auth/oauth/configs.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package oauth
22

33
import (
4+
"strings"
5+
46
"golang.org/x/oauth2"
57
)
68

@@ -77,7 +79,7 @@ func NewOIDCClient(cfg *OIDCConfig) *oauth2.Config {
7779
AuthURL: cfg.AuthURL,
7880
TokenURL: cfg.TokenURL,
7981
},
80-
RedirectURL: cfg.BaseURL + "/api/v1/users/oidc/callback",
82+
RedirectURL: strings.TrimRight(cfg.BaseURL, "/") + "/api/v1/users/oidc/callback",
8183
Scopes: cfg.Scopes,
8284
}
8385
}

pkg/config/loader/loader.go

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,23 @@ func createControllerLayer(dc *database.Layer, cf *server.ServerConfigFile, vers
634634
return nil, nil, fmt.Errorf("oidc issuer url is required")
635635
}
636636

637-
oidcProvider, err := oidc.NewProvider(context.Background(), cf.Auth.OIDC.IssuerURL)
637+
// Ensure "openid" scope is always present, since we rely on the ID token.
638+
hasOpenID := false
639+
for _, s := range cf.Auth.OIDC.Scopes {
640+
if s == "openid" {
641+
hasOpenID = true
642+
break
643+
}
644+
}
645+
646+
if !hasOpenID {
647+
cf.Auth.OIDC.Scopes = append([]string{"openid"}, cf.Auth.OIDC.Scopes...)
648+
}
649+
650+
discoveryCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
651+
defer cancel()
652+
653+
oidcProvider, err := oidc.NewProvider(discoveryCtx, cf.Auth.OIDC.IssuerURL)
638654
if err != nil {
639655
return nil, nil, fmt.Errorf("could not create OIDC provider from issuer URL %s: %w", cf.Auth.OIDC.IssuerURL, err)
640656
}
@@ -804,6 +820,10 @@ func createControllerLayer(dc *database.Layer, cf *server.ServerConfigFile, vers
804820
cf.Runtime.AllowedOrigins = getStrArr(cf.Runtime.AllowedOriginsString)
805821
}
806822

823+
if cf.Auth.OIDC.ScopesString != "" {
824+
cf.Auth.OIDC.Scopes = getStrArr(cf.Auth.OIDC.ScopesString)
825+
}
826+
807827
if cf.Runtime.Monitoring.TLSRootCAFile == "" {
808828
cf.Runtime.Monitoring.TLSRootCAFile = cf.TLS.TLSRootCAFile
809829
}

pkg/config/server/server.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,9 @@ type ConfigFileAuthOIDC struct {
505505
ClientSecret string `mapstructure:"clientSecret" json:"clientSecret,omitempty"`
506506
IssuerURL string `mapstructure:"issuerURL" json:"issuerURL,omitempty"`
507507
Scopes []string `mapstructure:"scopes" json:"scopes,omitempty" default:"[\"openid\", \"profile\", \"email\"]"`
508+
// ScopesString is used to bind the SERVER_AUTH_OIDC_SCOPES env var, since
509+
// direct env-to-[]string binding is unreliable without a decode hook.
510+
ScopesString string `mapstructure:"scopesString" json:"scopesString,omitempty"`
508511
}
509512

510513
type ConfigFileAuthCookie struct {
@@ -858,6 +861,7 @@ func BindAllEnv(v *viper.Viper) {
858861
_ = v.BindEnv("auth.oidc.clientSecret", "SERVER_AUTH_OIDC_CLIENT_SECRET")
859862
_ = v.BindEnv("auth.oidc.issuerURL", "SERVER_AUTH_OIDC_ISSUER_URL")
860863
_ = v.BindEnv("auth.oidc.scopes", "SERVER_AUTH_OIDC_SCOPES")
864+
_ = v.BindEnv("auth.oidc.scopesString", "SERVER_AUTH_OIDC_SCOPES")
861865

862866
// task queue options
863867
// legacy options

0 commit comments

Comments
 (0)