Extract username into a separate package and add OIDC tests

coder · kylecarbs · Aug 1, 2022 · Jul 29, 2022 · Jul 31, 2022 · Jul 31, 2022
commit 7eb897ab0f3b4716a61f7dbafa27536a6deef6a0
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -42,6 +42,7 @@
     "mattn",
     "mitchellh",
     "moby",
+    "namesgenerator",
     "nfpms",
     "nhooyr",
     "nolint",

diff --git a/cli/server.go b/cli/server.go
@@ -306,7 +306,6 @@ func server() *cobra.Command {
 						Endpoint:     oidcProvider.Endpoint(),
 						Scopes:       oidcScopes,
 					},
-					Provider: oidcProvider,
 					Verifier: oidcProvider.Verifier(&oidc.Config{
 						ClientID: oidcClientID,
 					}),

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -63,6 +63,7 @@ type Options struct {
 	Authorizer           rbac.Authorizer
 	AzureCertificates    x509.VerifyOptions
 	GithubOAuth2Config   *coderd.GithubOAuth2Config
+	OIDCConfig           *coderd.OIDCConfig
 	GoogleTokenValidator *idtoken.Validator
 	SSHKeygenAlgorithm   gitsshkey.Algorithm
 	APIRateLimit         int
@@ -189,6 +190,7 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer)
 		AWSCertificates:      options.AWSCertificates,
 		AzureCertificates:    options.AzureCertificates,
 		GithubOAuth2Config:   options.GithubOAuth2Config,
+		OIDCConfig:           options.OIDCConfig,
 		GoogleTokenValidator: options.GoogleTokenValidator,
 		SSHKeygenAlgorithm:   options.SSHKeygenAlgorithm,
 		TURNServer:           turnServer,

diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go
@@ -7,17 +7,16 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
-	"regexp"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 
+	"github.com/coder/coder/coderd/username"
 	"github.com/coder/coder/codersdk"
 )
 
 var (
-	validate      *validator.Validate
-	usernameRegex = regexp.MustCompile("^[a-zA-Z0-9]+(?:-[a-zA-Z0-9]+)*$")
+	validate *validator.Validate
 )
 
 // This init is used to create a validator and register validation-specific
@@ -39,13 +38,7 @@ func init() {
 		if !ok {
 			return false
 		}
-		if len(str) > 32 {
-			return false
-		}
-		if len(str) < 1 {
-			return false
-		}
-		return usernameRegex.MatchString(str)
+		return username.Valid(str)
 	})
 	if err != nil {
 		panic(err)

diff --git a/coderd/httpapi/httpapi_test.go b/coderd/httpapi/httpapi_test.go
@@ -81,71 +81,6 @@ func TestRead(t *testing.T) {
 	})
 }
 
-func TestReadUsername(t *testing.T) {
-	t.Parallel()
-	// Tests whether usernames are valid or not.
-	testCases := []struct {
-		Username string
-		Valid    bool
-	}{
-		{"1", true},
-		{"12", true},
-		{"123", true},
-		{"12345678901234567890", true},
-		{"123456789012345678901", true},
-		{"a", true},
-		{"a1", true},
-		{"a1b2", true},
-		{"a1b2c3d4e5f6g7h8i9j0", true},
-		{"a1b2c3d4e5f6g7h8i9j0k", true},
-		{"aa", true},
-		{"abc", true},
-		{"abcdefghijklmnopqrst", true},
-		{"abcdefghijklmnopqrstu", true},
-		{"wow-test", true},
-
-		{"", false},
-		{" ", false},
-		{" a", false},
-		{" a ", false},
-		{" 1", false},
-		{"1 ", false},
-		{" aa", false},
-		{"aa ", false},
-		{" 12", false},
-		{"12 ", false},
-		{" a1", false},
-		{"a1 ", false},
-		{" abcdefghijklmnopqrstu", false},
-		{"abcdefghijklmnopqrstu ", false},
-		{" 123456789012345678901", false},
-		{" a1b2c3d4e5f6g7h8i9j0k", false},
-		{"a1b2c3d4e5f6g7h8i9j0k ", false},
-		{"bananas_wow", false},
-		{"test--now", false},
-
-		{"123456789012345678901234567890123", false},
-		{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", false},
-		{"123456789012345678901234567890123123456789012345678901234567890123", false},
-	}
-	type toValidate struct {
-		Username string `json:"username" validate:"username"`
-	}
-	for _, testCase := range testCases {
-		testCase := testCase
-		t.Run(testCase.Username, func(t *testing.T) {
-			t.Parallel()
-			rw := httptest.NewRecorder()
-			data, err := json.Marshal(toValidate{testCase.Username})
-			require.NoError(t, err)
-			r := httptest.NewRequest("POST", "/", bytes.NewBuffer(data))
-
-			var validate toValidate
-			require.Equal(t, testCase.Valid, httpapi.Read(rw, r, &validate))
-		})
-	}
-}
-
 func WebsocketCloseMsg(t *testing.T) {
 	t.Parallel()
 

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -16,6 +16,7 @@ import (
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/httpapi"
 	"github.com/coder/coder/coderd/httpmw"
+	"github.com/coder/coder/coderd/username"
 	"github.com/coder/coder/codersdk"
 )
 
@@ -219,7 +220,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 type OIDCConfig struct {
 	httpmw.OAuth2Config
 
-	Provider *oidc.Provider
 	Verifier *oidc.IDTokenVerifier
 	// EmailDomain is an optional domain to require when authenticating.
 	EmailDomain  string
@@ -267,12 +267,30 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		})
 		return
 	}
+	if claims.Email == "" {
+		httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
+			Message: "No email found in OIDC payload!",
+		})
+		return
+	}
 	if !claims.Verified {
 		httpapi.Write(rw, http.StatusForbidden, codersdk.Response{
 			Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", claims.Email),
 		})
 		return
 	}
+	// The username is a required property in Coder. We make a best-effort
+	// attempt at using what the claims provide, but if that fails we will
+	// generate a random username.
+	if !username.Valid(claims.Username) {
+		// If no username is provided, we can default to use the email address.
+		// This will be converted in the from function below, so it's safe
+		// to keep the domain.
+		if claims.Username == "" {
+			claims.Username = claims.Email
+		}
+		claims.Username = username.From(claims.Username)
+	}
 	if api.OIDCConfig.EmailDomain != "" {
 		if !strings.HasSuffix(claims.Email, api.OIDCConfig.EmailDomain) {
 			httpapi.Write(rw, http.StatusForbidden, codersdk.Response{
@@ -319,6 +337,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 			Message: "Failed to get user by email.",
 			Detail:  err.Error(),
 		})
+		return
 	}
 
 	_, created := api.createAPIKey(rw, r, database.InsertAPIKeyParams{

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -2,11 +2,19 @@ package coderd_test
 
 import (
 	"context"
+	"crypto"
+	"crypto/rand"
+	"crypto/rsa"
+	"io"
 	"net/http"
 	"net/url"
 	"testing"
+	"time"
 
+	"github.com/coreos/go-oidc/v3/oidc"
+	"github.com/golang-jwt/jwt"
 	"github.com/google/go-github/v43/github"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
 	"golang.org/x/xerrors"
@@ -16,13 +24,18 @@ import (
 	"github.com/coder/coder/codersdk"
 )
 
-type oauth2Config struct{}
+type oauth2Config struct {
+	token *oauth2.Token
+}
 
 func (*oauth2Config) AuthCodeURL(state string, _ ...oauth2.AuthCodeOption) string {
 	return "/?state=" + url.QueryEscape(state)
 }
 
-func (*oauth2Config) Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
+func (o *oauth2Config) Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
+	if o.token != nil {
+		return o.token, nil
+	}
 	return &oauth2.Token{
 		AccessToken: "token",
 	}, nil
@@ -249,6 +262,117 @@ func TestUserOAuth2Github(t *testing.T) {
 	})
 }
 
+func TestUserOIDC(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		Name         string
+		Claims       jwt.MapClaims
+		AllowSignups bool
+		EmailDomain  string
+		Username     string
+		StatusCode   int
+	}{{
+		Name: "EmailNotVerified",
+		Claims: jwt.MapClaims{
+			"email": "kyle@kwc.io",
+		},
+		AllowSignups: true,
+		StatusCode:   http.StatusForbidden,
+	}, {
+		Name: "NotInRequiredEmailDomain",
+		Claims: jwt.MapClaims{
+			"email":          "kyle@kwc.io",
+			"email_verified": true,
+		},
+		AllowSignups: true,
+		EmailDomain:  "coder.com",
+		StatusCode:   http.StatusForbidden,
+	}, {
+		Name:         "EmptyClaims",
+		Claims:       jwt.MapClaims{},
+		AllowSignups: true,
+		StatusCode:   http.StatusBadRequest,
+	}, {
+		Name: "NoSignups",
+		Claims: jwt.MapClaims{
+			"email":          "kyle@kwc.io",
+			"email_verified": true,
+		},
+		StatusCode: http.StatusForbidden,
+	}, {
+		Name: "UsernameFromEmail",
+		Claims: jwt.MapClaims{
+			"email":          "kyle@kwc.io",
+			"email_verified": true,
+		},
+		Username:     "kyle",
+		AllowSignups: true,
+		StatusCode:   http.StatusTemporaryRedirect,
+	}, {
+		Name: "UsernameFromClaims",
+		Claims: jwt.MapClaims{
+			"email":              "kyle@kwc.io",
+			"email_verified":     true,
+			"preferred_username": "hotdog",
+		},
+		Username:     "hotdog",
+		AllowSignups: true,
+		StatusCode:   http.StatusTemporaryRedirect,
+	}} {
+		tc := tc
+		t.Run(tc.Name, func(t *testing.T) {
+			t.Parallel()
+			config := createOIDCConfig(t, tc.Claims)
+			config.AllowSignups = tc.AllowSignups
+			config.EmailDomain = tc.EmailDomain
+			client := coderdtest.New(t, &coderdtest.Options{
+				OIDCConfig: config,
+			})
+			resp := oidcCallback(t, client)
+			assert.Equal(t, tc.StatusCode, resp.StatusCode)
+
+			if tc.Username != "" {
+				client.SessionToken = resp.Cookies()[0].Value
+				user, err := client.User(context.Background(), "me")
+				require.NoError(t, err)
+				require.Equal(t, tc.Username, user.Username)
+			}
+		})
+	}
+}
+
+// createOIDCConfig generates a new OIDCConfig that returns a static token
+// with the claims provided.
+func createOIDCConfig(t *testing.T, claims jwt.MapClaims) *coderd.OIDCConfig {
+	t.Helper()
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err)
+
+	// https://datatracker.ietf.org/doc/html/rfc7519#section-4.1
+	claims["exp"] = time.Now().Add(time.Hour).UnixMilli()
+
+	signed, err := jwt.NewWithClaims(jwt.SigningMethodRS256, claims).SignedString(key)
+	require.NoError(t, err)
+
+	verifier := oidc.NewVerifier("", &oidc.StaticKeySet{
+		PublicKeys: []crypto.PublicKey{key.Public()},
+	}, &oidc.Config{
+		SkipClientIDCheck: true,
+	})
+
+	return &coderd.OIDCConfig{
+		OAuth2Config: &oauth2Config{
+			token: (&oauth2.Token{
+				AccessToken: "token",
+			}).WithExtra(map[string]interface{}{
+				"id_token": signed,
+			}),
+		},
+		Verifier: verifier,
+	}
+}
+
 func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
 	client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
 		return http.ErrUseLastResponse
@@ -269,3 +393,26 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
 	})
 	return res
 }
+
+func oidcCallback(t *testing.T, client *codersdk.Client) *http.Response {
+	t.Helper()
+	client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+		return http.ErrUseLastResponse
+	}
+	state := "somestate"
+	oauthURL, err := client.URL.Parse("/api/v2/users/oidc/callback?code=asd&state=" + state)
+	require.NoError(t, err)
+	req, err := http.NewRequest("GET", oauthURL.String(), nil)
+	require.NoError(t, err)
+	req.AddCookie(&http.Cookie{
+		Name:  "oauth_state",
+		Value: state,
+	})
+	res, err := client.HTTPClient.Do(req)
+	require.NoError(t, err)
+	defer res.Body.Close()
+	data, err := io.ReadAll(res.Body)
+	require.NoError(t, err)
+	t.Log(string(data))
+	return res
+}