fix: use UserInfo endpoint with OIDC (#5735)

kylecarbs · web-flow · commit bbc1a9a1d8d0 · 2023-01-16T16:06:39.000-06:00
This resolves a user issue surfaced in Discord: https://discord.com/channels/747933592273027093/1064566338875576361/1064566338875576361 Both methods of obtaining claims need to be used according to the OIDC specification.
diff --git a/cli/server.go b/cli/server.go
@@ -543,6 +543,7 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
 						Endpoint:     oidcProvider.Endpoint(),
 						Scopes:       cfg.OIDC.Scopes.Value,
 					},
+					Provider: oidcProvider,
 					Verifier: oidcProvider.Verifier(&oidc.Config{
 						ClientID: cfg.OIDC.ClientID.Value,
 					}),
diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -887,14 +887,31 @@ func (o *OIDCConfig) EncodeClaims(t *testing.T, claims jwt.MapClaims) string {
 	return base64.StdEncoding.EncodeToString([]byte(signed))
 }
 
-func (o *OIDCConfig) OIDCConfig() *coderd.OIDCConfig {
+func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *coderd.OIDCConfig {
+	// By default, the provider can be empty.
+	// This means it won't support any endpoints!
+	provider := &oidc.Provider{}
+	if userInfoClaims != nil {
+		resp, err := json.Marshal(userInfoClaims)
+		require.NoError(t, err)
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write(resp)
+		}))
+		t.Cleanup(srv.Close)
+		cfg := &oidc.ProviderConfig{
+			UserInfoURL: srv.URL,
+		}
+		provider = cfg.NewProvider(context.Background())
+	}
 	return &coderd.OIDCConfig{
 		OAuth2Config: o,
 		Verifier: oidc.NewVerifier(o.issuer, &oidc.StaticKeySet{
 			PublicKeys: []crypto.PublicKey{o.key.Public()},
 		}, &oidc.Config{
 			SkipClientIDCheck: true,
 		}),
+		Provider:      provider,
 		UsernameField: "preferred_username",
 	}
 }
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -204,6 +204,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 type OIDCConfig struct {
 	httpmw.OAuth2Config
 
+	Provider *oidc.Provider
 	Verifier *oidc.IDTokenVerifier
 	// EmailDomains are the domains to enforce when a user authenticates.
 	EmailDomain  []string
@@ -258,6 +259,35 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		})
 		return
 	}
+
+	// Not all claims are necessarily embedded in the `id_token`.
+	// In GitLab, the username is left empty and must be fetched in UserInfo.
+	//
+	// The OIDC specification says claims can be in either place, so we fetch
+	// user info and merge the two claim sets to be sure we have all of
+	// the correct data.
+	userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
+	if err == nil {
+		userInfoClaims := map[string]interface{}{}
+		err = userInfo.Claims(&userInfoClaims)
+		if err != nil {
+			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+				Message: "Failed to unmarshal user info claims.",
+				Detail:  err.Error(),
+			})
+			return
+		}
+		for k, v := range userInfoClaims {
+			claims[k] = v
+		}
+	} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
+		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Failed to obtain user information claims.",
+			Detail:  "The OIDC provider returned no claims as part of the `id_token`. The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
+		})
+		return
+	}
+
 	usernameRaw, ok := claims[api.OIDCConfig.UsernameField]
 	var username string
 	if ok {
diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -480,7 +480,8 @@ func TestUserOIDC(t *testing.T) {
 
 	for _, tc := range []struct {
 		Name                string
-		Claims              jwt.MapClaims
+		IDTokenClaims       jwt.MapClaims
+		UserInfoClaims      jwt.MapClaims
 		AllowSignups        bool
 		EmailDomain         []string
 		Username            string
@@ -489,31 +490,31 @@ func TestUserOIDC(t *testing.T) {
 		IgnoreEmailVerified bool
 	}{{
 		Name: "EmailOnly",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email": "kyle@kwc.io",
 		},
 		AllowSignups: true,
 		StatusCode:   http.StatusTemporaryRedirect,
 		Username:     "kyle",
 	}, {
 		Name: "EmailNotVerified",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": false,
 		},
 		AllowSignups: true,
 		StatusCode:   http.StatusForbidden,
 	}, {
 		Name: "EmailNotAString",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          3.14159,
 			"email_verified": false,
 		},
 		AllowSignups: true,
 		StatusCode:   http.StatusBadRequest,
 	}, {
 		Name: "EmailNotVerifiedIgnored",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": false,
 		},
@@ -523,7 +524,7 @@ func TestUserOIDC(t *testing.T) {
 		IgnoreEmailVerified: true,
 	}, {
 		Name: "NotInRequiredEmailDomain",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": true,
 		},
@@ -534,7 +535,7 @@ func TestUserOIDC(t *testing.T) {
 		StatusCode: http.StatusForbidden,
 	}, {
 		Name: "EmailDomainCaseInsensitive",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@KWC.io",
 			"email_verified": true,
 		},
@@ -544,20 +545,20 @@ func TestUserOIDC(t *testing.T) {
 		},
 		StatusCode: http.StatusTemporaryRedirect,
 	}, {
-		Name:         "EmptyClaims",
-		Claims:       jwt.MapClaims{},
-		AllowSignups: true,
-		StatusCode:   http.StatusBadRequest,
+		Name:          "EmptyClaims",
+		IDTokenClaims: jwt.MapClaims{},
+		AllowSignups:  true,
+		StatusCode:    http.StatusBadRequest,
 	}, {
 		Name: "NoSignups",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": true,
 		},
 		StatusCode: http.StatusForbidden,
 	}, {
 		Name: "UsernameFromEmail",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": true,
 		},
@@ -566,7 +567,7 @@ func TestUserOIDC(t *testing.T) {
 		StatusCode:   http.StatusTemporaryRedirect,
 	}, {
 		Name: "UsernameFromClaims",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":              "kyle@kwc.io",
 			"email_verified":     true,
 			"preferred_username": "hotdog",
@@ -578,7 +579,7 @@ func TestUserOIDC(t *testing.T) {
 		// Services like Okta return the email as the username:
 		// https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present
 		Name: "UsernameAsEmail",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"email":              "kyle@kwc.io",
 			"email_verified":     true,
 			"preferred_username": "kyle@kwc.io",
@@ -589,21 +590,35 @@ func TestUserOIDC(t *testing.T) {
 	}, {
 		// See: https://github.com/coder/coder/issues/4472
 		Name: "UsernameIsEmail",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
 			"preferred_username": "kyle@kwc.io",
 		},
 		Username:     "kyle",
 		AllowSignups: true,
 		StatusCode:   http.StatusTemporaryRedirect,
 	}, {
 		Name: "WithPicture",
-		Claims: jwt.MapClaims{
+		IDTokenClaims: jwt.MapClaims{
+			"email":              "kyle@kwc.io",
+			"email_verified":     true,
+			"preferred_username": "kyle",
+			"picture":            "/example.png",
+		},
+		Username:     "kyle",
+		AllowSignups: true,
+		AvatarURL:    "/example.png",
+		StatusCode:   http.StatusTemporaryRedirect,
+	}, {
+		Name: "WithUserInfoClaims",
+		IDTokenClaims: jwt.MapClaims{
 			"email":          "kyle@kwc.io",
 			"email_verified": true,
-			"username":       "kyle",
-			"picture":        "/example.png",
 		},
-		Username:     "kyle",
+		UserInfoClaims: jwt.MapClaims{
+			"preferred_username": "potato",
+			"picture":            "/example.png",
+		},
+		Username:     "potato",
 		AllowSignups: true,
 		AvatarURL:    "/example.png",
 		StatusCode:   http.StatusTemporaryRedirect,
@@ -613,15 +628,15 @@ func TestUserOIDC(t *testing.T) {
 			t.Parallel()
 			conf := coderdtest.NewOIDCConfig(t, "")
 
-			config := conf.OIDCConfig()
+			config := conf.OIDCConfig(t, tc.UserInfoClaims)
 			config.AllowSignups = tc.AllowSignups
 			config.EmailDomain = tc.EmailDomain
 			config.IgnoreEmailVerified = tc.IgnoreEmailVerified
 
 			client := coderdtest.New(t, &coderdtest.Options{
 				OIDCConfig: config,
 			})
-			resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.Claims))
+			resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.IDTokenClaims))
 			assert.Equal(t, tc.StatusCode, resp.StatusCode)
 
 			ctx, _ := testutil.Context(t)
@@ -647,7 +662,7 @@ func TestUserOIDC(t *testing.T) {
 
 		conf := coderdtest.NewOIDCConfig(t, "")
 
-		config := conf.OIDCConfig()
+		config := conf.OIDCConfig(t, nil)
 		config.AllowSignups = true
 
 		client := coderdtest.New(t, &coderdtest.Options{
@@ -705,6 +720,7 @@ func TestUserOIDC(t *testing.T) {
 		verifier := oidc.NewVerifier("", &oidc.StaticKeySet{
 			PublicKeys: []crypto.PublicKey{},
 		}, &oidc.Config{})
+		provider := &oidc.Provider{}
 
 		client := coderdtest.New(t, &coderdtest.Options{
 			OIDCConfig: &coderd.OIDCConfig{
@@ -715,6 +731,7 @@ func TestUserOIDC(t *testing.T) {
 						"id_token": "invalid",
 					}),
 				},
+				Provider: provider,
 				Verifier: verifier,
 			},
 		})
