Skip to content

Commit ead92c8

Browse files
EmyrkEmyrk
committed
chore: sane defaults and make gen
1 parent 53fc864 commit ead92c8

File tree

10 files changed

+109
-50
lines changed

10 files changed

+109
-50
lines changed

‎cli/server.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2771,6 +2771,8 @@ func parseExternalAuthProvidersFromEnv(prefix string, environ []string) ([]coder
27712771
provider.MCPToolAllowRegex = v.Value
27722772
case "MCP_TOOL_DENY_REGEX":
27732773
provider.MCPToolDenyRegex = v.Value
2774+
case "PKCE_METHODS":
2775+
provider.CodeChallengeMethodsSupported = strings.Split(v.Value, " ")
27742776
}
27752777
providers[providerNum] = provider
27762778
}

‎coderd/coderd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1298,7 +1298,7 @@ func New(options *Options) *API {
12981298
})
12991299
r.Route("/oidc/callback", func(r chi.Router) {
13001300
r.Use(
1301-
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCEMethods),
1301+
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCESupported()),
13021302
)
13031303
r.Get("/", api.userOIDC)
13041304
})

‎coderd/externalauth.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/coder/coder/v2/coderd/externalauth"
1717
"github.com/coder/coder/v2/coderd/httpapi"
1818
"github.com/coder/coder/v2/coderd/httpmw"
19+
"github.com/coder/coder/v2/coderd/util/slice"
1920
"github.com/coder/coder/v2/codersdk"
2021
)
2122

@@ -417,14 +418,15 @@ func ExternalAuthConfigs(auths []*externalauth.Config) []codersdk.ExternalAuthLi
417418

418419
func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvider {
419420
return codersdk.ExternalAuthLinkProvider{
420-
ID: cfg.ID,
421-
Type: cfg.Type,
422-
Device: cfg.DeviceAuth != nil,
423-
DisplayName: cfg.DisplayName,
424-
DisplayIcon: cfg.DisplayIcon,
425-
AllowRefresh: !cfg.NoRefresh,
426-
AllowValidate: cfg.ValidateURL != "",
427-
SupportsRevocation: cfg.RevokeURL != "",
421+
ID: cfg.ID,
422+
Type: cfg.Type,
423+
Device: cfg.DeviceAuth != nil,
424+
DisplayName: cfg.DisplayName,
425+
DisplayIcon: cfg.DisplayIcon,
426+
AllowRefresh: !cfg.NoRefresh,
427+
AllowValidate: cfg.ValidateURL != "",
428+
SupportsRevocation: cfg.RevokeURL != "",
429+
CodeChallengeMethodsSupported: slice.ToStrings(cfg.CodeChallengeMethodsSupported),
428430
}
429431
}
430432

‎coderd/externalauth/externalauth.go‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/coder/coder/v2/coderd/database"
2626
"github.com/coder/coder/v2/coderd/database/dbtime"
2727
"github.com/coder/coder/v2/coderd/promoauth"
28+
"github.com/coder/coder/v2/coderd/util/slice"
2829
"github.com/coder/coder/v2/codersdk"
2930
"github.com/coder/retry"
3031
)
@@ -724,24 +725,25 @@ func ConvertConfig(instrument *promoauth.Factory, entries []codersdk.ExternalAut
724725
}
725726

726727
cfg := &Config{
727-
InstrumentedOAuth2Config: instrumented,
728-
ID: entry.ID,
729-
ClientID: entry.ClientID,
730-
ClientSecret: entry.ClientSecret,
731-
Regex: regex,
732-
Type: entry.Type,
733-
NoRefresh: entry.NoRefresh,
734-
ValidateURL: entry.ValidateURL,
735-
RevokeURL: entry.RevokeURL,
736-
RevokeTimeout: tokenRevocationTimeout,
737-
AppInstallationsURL: entry.AppInstallationsURL,
738-
AppInstallURL: entry.AppInstallURL,
739-
DisplayName: entry.DisplayName,
740-
DisplayIcon: entry.DisplayIcon,
741-
ExtraTokenKeys: entry.ExtraTokenKeys,
742-
MCPURL: entry.MCPURL,
743-
MCPToolAllowRegex: mcpToolAllow,
744-
MCPToolDenyRegex: mcpToolDeny,
728+
InstrumentedOAuth2Config: instrumented,
729+
ID: entry.ID,
730+
ClientID: entry.ClientID,
731+
ClientSecret: entry.ClientSecret,
732+
Regex: regex,
733+
Type: entry.Type,
734+
NoRefresh: entry.NoRefresh,
735+
ValidateURL: entry.ValidateURL,
736+
RevokeURL: entry.RevokeURL,
737+
RevokeTimeout: tokenRevocationTimeout,
738+
AppInstallationsURL: entry.AppInstallationsURL,
739+
AppInstallURL: entry.AppInstallURL,
740+
DisplayName: entry.DisplayName,
741+
DisplayIcon: entry.DisplayIcon,
742+
ExtraTokenKeys: entry.ExtraTokenKeys,
743+
MCPURL: entry.MCPURL,
744+
MCPToolAllowRegex: mcpToolAllow,
745+
MCPToolDenyRegex: mcpToolDeny,
746+
CodeChallengeMethodsSupported: slice.StringEnums[promoauth.Oauth2PKCEChallengeMethod](entry.CodeChallengeMethodsSupported),
745747
}
746748

747749
if entry.DeviceFlow {
@@ -801,11 +803,8 @@ func applyDefaultsToConfig(config *codersdk.ExternalAuthConfig) {
801803
copyDefaultSettings(config, azureDevopsEntraDefaults(config))
802804
return
803805
default:
804-
// Defaults apply to any provider that doesn't have specific defaults.
805-
copyDefaultSettings(config, codersdk.ExternalAuthConfig{
806-
// PKCE should always be enabled by default.
807-
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)},
808-
})
806+
// Global defaults are specified at the end of the `copyDefaultSettings` function.
807+
copyDefaultSettings(config, codersdk.ExternalAuthConfig{})
809808
return
810809
}
811810
}
@@ -847,6 +846,9 @@ func copyDefaultSettings(config *codersdk.ExternalAuthConfig, defaults codersdk.
847846
if len(config.ExtraTokenKeys) == 0 {
848847
config.ExtraTokenKeys = defaults.ExtraTokenKeys
849848
}
849+
if config.CodeChallengeMethodsSupported == nil {
850+
config.CodeChallengeMethodsSupported = defaults.CodeChallengeMethodsSupported
851+
}
850852

851853
// Apply defaults if it's still empty...
852854
if config.ID == "" {
@@ -860,7 +862,7 @@ func copyDefaultSettings(config *codersdk.ExternalAuthConfig, defaults codersdk.
860862
config.DisplayIcon = "/emojis/1f511.png"
861863
}
862864
if config.CodeChallengeMethodsSupported == nil {
863-
config.CodeChallengeMethodsSupported = defaults.CodeChallengeMethodsSupported
865+
config.CodeChallengeMethodsSupported = []string{string(promoauth.PKCEChallengeMethodSha256)}
864866
}
865867
}
866868

‎coderd/externalauth/externalauth_internal_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,3 +196,45 @@ func Test_bitbucketServerConfigDefaults(t *testing.T) {
196196
})
197197
}
198198
}
199+
200+
func TestUntyped(t *testing.T) {
201+
t.Parallel()
202+
203+
tests := []struct {
204+
name string
205+
input codersdk.ExternalAuthConfig
206+
expected codersdk.ExternalAuthConfig
207+
}{
208+
{
209+
// Unknown Type uses S256 by default.
210+
name: "RandomValues",
211+
input: codersdk.ExternalAuthConfig{
212+
Type: "unknown",
213+
AuthURL: "https://auth.com/auth",
214+
ValidateURL: "https://validate.com/validate",
215+
TokenURL: "https://token.com/token",
216+
RevokeURL: "https://token.com/revoke",
217+
Regex: "random",
218+
},
219+
expected: codersdk.ExternalAuthConfig{
220+
ID: "unknown",
221+
Type: "unknown",
222+
DisplayName: "unknown",
223+
DisplayIcon: "/emojis/1f511.png",
224+
AuthURL: "https://auth.com/auth",
225+
ValidateURL: "https://validate.com/validate",
226+
TokenURL: "https://token.com/token",
227+
RevokeURL: "https://token.com/revoke",
228+
Regex: `random`,
229+
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)},
230+
},
231+
},
232+
}
233+
for _, c := range tests {
234+
t.Run(c.name, func(t *testing.T) {
235+
t.Parallel()
236+
applyDefaultsToConfig(&c.input)
237+
require.Equal(t, c.input, c.expected)
238+
})
239+
}
240+
}

‎coderd/httpmw/oauth2_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,15 @@ func TestOAuth2(t *testing.T) {
5050
t.Parallel()
5151
req := httptest.NewRequest("GET", "/", nil)
5252
res := httptest.NewRecorder()
53-
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
53+
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
5454
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
5555
})
5656
t.Run("RedirectWithoutCode", func(t *testing.T) {
5757
t.Parallel()
5858
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil)
5959
res := httptest.NewRecorder()
6060
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
61-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
61+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
6262
location := res.Header().Get("Location")
6363
if !assert.NotEmpty(t, location) {
6464
return
@@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) {
8282
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
8383
res := httptest.NewRecorder()
8484
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
85-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
85+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
8686
location := res.Header().Get("Location")
8787
if !assert.NotEmpty(t, location) {
8888
return
@@ -97,15 +97,15 @@ func TestOAuth2(t *testing.T) {
9797
req := httptest.NewRequest("GET", "/?code=something", nil)
9898
res := httptest.NewRecorder()
9999
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
100-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
100+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
101101
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
102102
})
103103
t.Run("NoStateCookie", func(t *testing.T) {
104104
t.Parallel()
105105
req := httptest.NewRequest("GET", "/?code=something&state=test", nil)
106106
res := httptest.NewRecorder()
107107
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
108-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
108+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
109109
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
110110
})
111111
t.Run("MismatchedState", func(t *testing.T) {
@@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) {
117117
})
118118
res := httptest.NewRecorder()
119119
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
120-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
120+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
121121
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
122122
})
123123
t.Run("ExchangeCodeAndState", func(t *testing.T) {
@@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) {
133133
})
134134
res := httptest.NewRecorder()
135135
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
136-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
136+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
137137
state := httpmw.OAuth2(r)
138138
require.Equal(t, "/dashboard", state.Redirect)
139139
})).ServeHTTP(res, req)
@@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) {
144144
res := httptest.NewRecorder()
145145
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar"))
146146
authOpts := map[string]string{"foo": "bar"}
147-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts)(nil).ServeHTTP(res, req)
147+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts, nil)(nil).ServeHTTP(res, req)
148148
location := res.Header().Get("Location")
149149
// Ideally we would also assert that the location contains the query params
150150
// we set in the auth URL but this would essentially be testing the oauth2 package.
@@ -160,7 +160,7 @@ func TestOAuth2(t *testing.T) {
160160
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{
161161
Secure: true,
162162
SameSite: "none",
163-
}, nil)(nil).ServeHTTP(res, req)
163+
}, nil, nil)(nil).ServeHTTP(res, req)
164164

165165
found := false
166166
for _, cookie := range res.Result().Cookies() {

‎coderd/userauth.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,6 +1175,14 @@ type OIDCConfig struct {
11751175
PKCEMethods []promoauth.Oauth2PKCEChallengeMethod
11761176
}
11771177

1178+
// PKCESupported is to prevent nil pointer dereference.
1179+
func (o *OIDCConfig) PKCESupported() []promoauth.Oauth2PKCEChallengeMethod {
1180+
if o == nil {
1181+
return nil
1182+
}
1183+
return o.PKCEMethods
1184+
}
1185+
11781186
// @Summary OpenID Connect Callback
11791187
// @ID openid-connect-callback
11801188
// @Security CoderSessionToken

‎codersdk/externalauth.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,15 @@ type ExternalAuthLink struct {
9494

9595
// ExternalAuthLinkProvider are the static details of a provider.
9696
type ExternalAuthLinkProvider struct {
97-
ID string `json:"id"`
98-
Type string `json:"type"`
99-
Device bool `json:"device"`
100-
DisplayName string `json:"display_name"`
101-
DisplayIcon string `json:"display_icon"`
102-
AllowRefresh bool `json:"allow_refresh"`
103-
AllowValidate bool `json:"allow_validate"`
104-
SupportsRevocation bool `json:"supports_revocation"`
97+
ID string `json:"id"`
98+
Type string `json:"type"`
99+
Device bool `json:"device"`
100+
DisplayName string `json:"display_name"`
101+
DisplayIcon string `json:"display_icon"`
102+
AllowRefresh bool `json:"allow_refresh"`
103+
AllowValidate bool `json:"allow_validate"`
104+
SupportsRevocation bool `json:"supports_revocation"`
105+
CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported"`
105106
}
106107

107108
type ExternalAuthAppInstallation struct {

‎site/src/api/typesGenerated.ts‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎site/src/testHelpers/entities.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4502,6 +4502,7 @@ export const MockGithubExternalProvider: TypesGen.ExternalAuthLinkProvider = {
45024502
allow_refresh: true,
45034503
allow_validate: true,
45044504
supports_revocation: false,
4505+
code_challenge_methods_supported: ["S256"],
45054506
};
45064507

45074508
export const MockGithubAuthLink: TypesGen.ExternalAuthLink = {

0 commit comments

Comments
 (0)