Skip to content

Commit 7eb897a

Browse files
kylecarbskylecarbs
committed
Extract username into a separate package and add OIDC tests
1 parent 50ee7ad commit 7eb897a

File tree

9 files changed

+322
-79
lines changed

9 files changed

+322
-79
lines changed

‎.vscode/settings.json

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
"mattn",
4343
"mitchellh",
4444
"moby",
45+
"namesgenerator",
4546
"nfpms",
4647
"nhooyr",
4748
"nolint",

‎cli/server.go

-1
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,6 @@ func server() *cobra.Command {
306306
Endpoint: oidcProvider.Endpoint(),
307307
Scopes: oidcScopes,
308308
},
309-
Provider: oidcProvider,
310309
Verifier: oidcProvider.Verifier(&oidc.Config{
311310
ClientID: oidcClientID,
312311
}),

‎coderd/coderdtest/coderdtest.go

+2
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ type Options struct {
6363
Authorizer rbac.Authorizer
6464
AzureCertificates x509.VerifyOptions
6565
GithubOAuth2Config *coderd.GithubOAuth2Config
66+
OIDCConfig *coderd.OIDCConfig
6667
GoogleTokenValidator *idtoken.Validator
6768
SSHKeygenAlgorithm gitsshkey.Algorithm
6869
APIRateLimit int
@@ -189,6 +190,7 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer)
189190
AWSCertificates: options.AWSCertificates,
190191
AzureCertificates: options.AzureCertificates,
191192
GithubOAuth2Config: options.GithubOAuth2Config,
193+
OIDCConfig: options.OIDCConfig,
192194
GoogleTokenValidator: options.GoogleTokenValidator,
193195
SSHKeygenAlgorithm: options.SSHKeygenAlgorithm,
194196
TURNServer: turnServer,

‎coderd/httpapi/httpapi.go

+3-10
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,16 @@ import (
77
"fmt"
88
"net/http"
99
"reflect"
10-
"regexp"
1110
"strings"
1211

1312
"github.com/go-playground/validator/v10"
1413

14+
"github.com/coder/coder/coderd/username"
1515
"github.com/coder/coder/codersdk"
1616
)
1717

1818
var (
19-
validate *validator.Validate
20-
usernameRegex = regexp.MustCompile("^[a-zA-Z0-9]+(?:-[a-zA-Z0-9]+)*$")
19+
validate *validator.Validate
2120
)
2221

2322
// This init is used to create a validator and register validation-specific
@@ -39,13 +38,7 @@ func init() {
3938
if !ok {
4039
return false
4140
}
42-
if len(str) > 32 {
43-
return false
44-
}
45-
if len(str) < 1 {
46-
return false
47-
}
48-
return usernameRegex.MatchString(str)
41+
return username.Valid(str)
4942
})
5043
if err != nil {
5144
panic(err)

‎coderd/httpapi/httpapi_test.go

-65
Original file line numberDiff line numberDiff line change
@@ -81,71 +81,6 @@ func TestRead(t *testing.T) {
8181
})
8282
}
8383

84-
func TestReadUsername(t *testing.T) {
85-
t.Parallel()
86-
// Tests whether usernames are valid or not.
87-
testCases := []struct {
88-
Username string
89-
Valid bool
90-
}{
91-
{"1", true},
92-
{"12", true},
93-
{"123", true},
94-
{"12345678901234567890", true},
95-
{"123456789012345678901", true},
96-
{"a", true},
97-
{"a1", true},
98-
{"a1b2", true},
99-
{"a1b2c3d4e5f6g7h8i9j0", true},
100-
{"a1b2c3d4e5f6g7h8i9j0k", true},
101-
{"aa", true},
102-
{"abc", true},
103-
{"abcdefghijklmnopqrst", true},
104-
{"abcdefghijklmnopqrstu", true},
105-
{"wow-test", true},
106-
107-
{"", false},
108-
{" ", false},
109-
{" a", false},
110-
{" a ", false},
111-
{" 1", false},
112-
{"1 ", false},
113-
{" aa", false},
114-
{"aa ", false},
115-
{" 12", false},
116-
{"12 ", false},
117-
{" a1", false},
118-
{"a1 ", false},
119-
{" abcdefghijklmnopqrstu", false},
120-
{"abcdefghijklmnopqrstu ", false},
121-
{" 123456789012345678901", false},
122-
{" a1b2c3d4e5f6g7h8i9j0k", false},
123-
{"a1b2c3d4e5f6g7h8i9j0k ", false},
124-
{"bananas_wow", false},
125-
{"test--now", false},
126-
127-
{"123456789012345678901234567890123", false},
128-
{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", false},
129-
{"123456789012345678901234567890123123456789012345678901234567890123", false},
130-
}
131-
type toValidate struct {
132-
Username string `json:"username" validate:"username"`
133-
}
134-
for _, testCase := range testCases {
135-
testCase := testCase
136-
t.Run(testCase.Username, func(t *testing.T) {
137-
t.Parallel()
138-
rw := httptest.NewRecorder()
139-
data, err := json.Marshal(toValidate{testCase.Username})
140-
require.NoError(t, err)
141-
r := httptest.NewRequest("POST", "/", bytes.NewBuffer(data))
142-
143-
var validate toValidate
144-
require.Equal(t, testCase.Valid, httpapi.Read(rw, r, &validate))
145-
})
146-
}
147-
}
148-
14984
func WebsocketCloseMsg(t *testing.T) {
15085
t.Parallel()
15186

‎coderd/userauth.go

+20-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/coder/coder/coderd/database"
1717
"github.com/coder/coder/coderd/httpapi"
1818
"github.com/coder/coder/coderd/httpmw"
19+
"github.com/coder/coder/coderd/username"
1920
"github.com/coder/coder/codersdk"
2021
)
2122

@@ -219,7 +220,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
219220
type OIDCConfig struct {
220221
httpmw.OAuth2Config
221222

222-
Provider *oidc.Provider
223223
Verifier *oidc.IDTokenVerifier
224224
// EmailDomain is an optional domain to require when authenticating.
225225
EmailDomain string
@@ -267,12 +267,30 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
267267
})
268268
return
269269
}
270+
if claims.Email == "" {
271+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
272+
Message: "No email found in OIDC payload!",
273+
})
274+
return
275+
}
270276
if !claims.Verified {
271277
httpapi.Write(rw, http.StatusForbidden, codersdk.Response{
272278
Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", claims.Email),
273279
})
274280
return
275281
}
282+
// The username is a required property in Coder. We make a best-effort
283+
// attempt at using what the claims provide, but if that fails we will
284+
// generate a random username.
285+
if !username.Valid(claims.Username) {
286+
// If no username is provided, we can default to use the email address.
287+
// This will be converted in the from function below, so it's safe
288+
// to keep the domain.
289+
if claims.Username == "" {
290+
claims.Username = claims.Email
291+
}
292+
claims.Username = username.From(claims.Username)
293+
}
276294
if api.OIDCConfig.EmailDomain != "" {
277295
if !strings.HasSuffix(claims.Email, api.OIDCConfig.EmailDomain) {
278296
httpapi.Write(rw, http.StatusForbidden, codersdk.Response{
@@ -319,6 +337,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
319337
Message: "Failed to get user by email.",
320338
Detail: err.Error(),
321339
})
340+
return
322341
}
323342

324343
_, created := api.createAPIKey(rw, r, database.InsertAPIKeyParams{

‎coderd/userauth_test.go

+149-2
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,19 @@ package coderd_test
22

33
import (
44
"context"
5+
"crypto"
6+
"crypto/rand"
7+
"crypto/rsa"
8+
"io"
59
"net/http"
610
"net/url"
711
"testing"
12+
"time"
813

14+
"github.com/coreos/go-oidc/v3/oidc"
15+
"github.com/golang-jwt/jwt"
916
"github.com/google/go-github/v43/github"
17+
"github.com/stretchr/testify/assert"
1018
"github.com/stretchr/testify/require"
1119
"golang.org/x/oauth2"
1220
"golang.org/x/xerrors"
@@ -16,13 +24,18 @@ import (
1624
"github.com/coder/coder/codersdk"
1725
)
1826

19-
type oauth2Config struct{}
27+
type oauth2Config struct {
28+
token *oauth2.Token
29+
}
2030

2131
func (*oauth2Config) AuthCodeURL(state string, _ ...oauth2.AuthCodeOption) string {
2232
return "/?state=" + url.QueryEscape(state)
2333
}
2434

25-
func (*oauth2Config) Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
35+
func (o *oauth2Config) Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
36+
if o.token != nil {
37+
return o.token, nil
38+
}
2639
return &oauth2.Token{
2740
AccessToken: "token",
2841
}, nil
@@ -249,6 +262,117 @@ func TestUserOAuth2Github(t *testing.T) {
249262
})
250263
}
251264

265+
func TestUserOIDC(t *testing.T) {
266+
t.Parallel()
267+
268+
for _, tc := range []struct {
269+
Name string
270+
Claims jwt.MapClaims
271+
AllowSignups bool
272+
EmailDomain string
273+
Username string
274+
StatusCode int
275+
}{{
276+
Name: "EmailNotVerified",
277+
Claims: jwt.MapClaims{
278+
"email": "kyle@kwc.io",
279+
},
280+
AllowSignups: true,
281+
StatusCode: http.StatusForbidden,
282+
}, {
283+
Name: "NotInRequiredEmailDomain",
284+
Claims: jwt.MapClaims{
285+
"email": "kyle@kwc.io",
286+
"email_verified": true,
287+
},
288+
AllowSignups: true,
289+
EmailDomain: "coder.com",
290+
StatusCode: http.StatusForbidden,
291+
}, {
292+
Name: "EmptyClaims",
293+
Claims: jwt.MapClaims{},
294+
AllowSignups: true,
295+
StatusCode: http.StatusBadRequest,
296+
}, {
297+
Name: "NoSignups",
298+
Claims: jwt.MapClaims{
299+
"email": "kyle@kwc.io",
300+
"email_verified": true,
301+
},
302+
StatusCode: http.StatusForbidden,
303+
}, {
304+
Name: "UsernameFromEmail",
305+
Claims: jwt.MapClaims{
306+
"email": "kyle@kwc.io",
307+
"email_verified": true,
308+
},
309+
Username: "kyle",
310+
AllowSignups: true,
311+
StatusCode: http.StatusTemporaryRedirect,
312+
}, {
313+
Name: "UsernameFromClaims",
314+
Claims: jwt.MapClaims{
315+
"email": "kyle@kwc.io",
316+
"email_verified": true,
317+
"preferred_username": "hotdog",
318+
},
319+
Username: "hotdog",
320+
AllowSignups: true,
321+
StatusCode: http.StatusTemporaryRedirect,
322+
}} {
323+
tc := tc
324+
t.Run(tc.Name, func(t *testing.T) {
325+
t.Parallel()
326+
config := createOIDCConfig(t, tc.Claims)
327+
config.AllowSignups = tc.AllowSignups
328+
config.EmailDomain = tc.EmailDomain
329+
client := coderdtest.New(t, &coderdtest.Options{
330+
OIDCConfig: config,
331+
})
332+
resp := oidcCallback(t, client)
333+
assert.Equal(t, tc.StatusCode, resp.StatusCode)
334+
335+
if tc.Username != "" {
336+
client.SessionToken = resp.Cookies()[0].Value
337+
user, err := client.User(context.Background(), "me")
338+
require.NoError(t, err)
339+
require.Equal(t, tc.Username, user.Username)
340+
}
341+
})
342+
}
343+
}
344+
345+
// createOIDCConfig generates a new OIDCConfig that returns a static token
346+
// with the claims provided.
347+
func createOIDCConfig(t *testing.T, claims jwt.MapClaims) *coderd.OIDCConfig {
348+
t.Helper()
349+
key, err := rsa.GenerateKey(rand.Reader, 2048)
350+
require.NoError(t, err)
351+
352+
// https://datatracker.ietf.org/doc/html/rfc7519#section-4.1
353+
claims["exp"] = time.Now().Add(time.Hour).UnixMilli()
354+
355+
signed, err := jwt.NewWithClaims(jwt.SigningMethodRS256, claims).SignedString(key)
356+
require.NoError(t, err)
357+
358+
verifier := oidc.NewVerifier("", &oidc.StaticKeySet{
359+
PublicKeys: []crypto.PublicKey{key.Public()},
360+
}, &oidc.Config{
361+
SkipClientIDCheck: true,
362+
})
363+
364+
return &coderd.OIDCConfig{
365+
OAuth2Config: &oauth2Config{
366+
token: (&oauth2.Token{
367+
AccessToken: "token",
368+
}).WithExtra(map[string]interface{}{
369+
"id_token": signed,
370+
}),
371+
},
372+
Verifier: verifier,
373+
}
374+
}
375+
252376
func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
253377
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
254378
return http.ErrUseLastResponse
@@ -269,3 +393,26 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
269393
})
270394
return res
271395
}
396+
397+
func oidcCallback(t *testing.T, client *codersdk.Client) *http.Response {
398+
t.Helper()
399+
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
400+
return http.ErrUseLastResponse
401+
}
402+
state := "somestate"
403+
oauthURL, err := client.URL.Parse("/api/v2/users/oidc/callback?code=asd&state=" + state)
404+
require.NoError(t, err)
405+
req, err := http.NewRequest("GET", oauthURL.String(), nil)
406+
require.NoError(t, err)
407+
req.AddCookie(&http.Cookie{
408+
Name: "oauth_state",
409+
Value: state,
410+
})
411+
res, err := client.HTTPClient.Do(req)
412+
require.NoError(t, err)
413+
defer res.Body.Close()
414+
data, err := io.ReadAll(res.Body)
415+
require.NoError(t, err)
416+
t.Log(string(data))
417+
return res
418+
}

0 commit comments

Comments
 (0)