Skip to content

Commit 37aa4c0

Browse files
EmyrkEmyrk
committed
PR feedback
1 parent c6b3b24 commit 37aa4c0

File tree

4 files changed

+18
-11
lines changed

4 files changed

+18
-11
lines changed

‎coderd/coderd.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"github.com/coder/coder/v2/coderd/oauth2provider"
2525
"github.com/coder/coder/v2/coderd/pproflabel"
2626
"github.com/coder/coder/v2/coderd/prebuilds"
27-
"github.com/coder/coder/v2/coderd/promoauth"
2827
"github.com/coder/coder/v2/coderd/usage"
2928
"github.com/coder/coder/v2/coderd/wsbuilder"
3029

@@ -1291,7 +1290,7 @@ func New(options *Options) *API {
12911290
r.Route("/github", func(r chi.Router) {
12921291
r.Use(
12931292
// Github supports PKCE S256
1294-
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256}),
1293+
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, options.GithubOAuth2Config.PKCESupported()),
12951294
)
12961295
r.Get("/callback", api.userOAuth2Github)
12971296
})

‎coderd/coderdtest/oidctest/idp.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ type FakeIDP struct {
169169
// clientID to be used by coderd
170170
clientID string
171171
clientSecret string
172-
pkce bool // TODO: Implement for refresh token flow as well
172+
pkce bool // TODO(Emyrk): Implement for refresh token flow as well
173173
// externalProviderID is optional to match the provider in coderd for
174174
// redirectURLs.
175175
externalProviderID string
@@ -1054,13 +1054,17 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
10541054
if f.pkce {
10551055
challenge, ok := f.codeToChallengeMap.Load(code)
10561056
if !ok {
1057-
httpError(rw, http.StatusBadRequest, xerrors.New("code challenge not found for code"))
1057+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: challenge not found for code"))
10581058
return
10591059
}
10601060
codeVerifier := values.Get("code_verifier")
1061+
if codeVerifier == "" {
1062+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: missing code_verifier"))
1063+
return
1064+
}
10611065
expecter := oauth2.S256ChallengeFromVerifier(codeVerifier)
10621066
if challenge != expecter {
1063-
httpError(rw, http.StatusBadRequest, xerrors.New("invalid code verifier"))
1067+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: invalid code verifier"))
10641068
return
10651069
}
10661070
}

‎coderd/externalauth/externalauth.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -895,7 +895,7 @@ func bitbucketServerDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exter
895895
DisplayName: "Bitbucket Server",
896896
Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"},
897897
DisplayIcon: "/icon/bitbucket.svg",
898-
// TODO: PKCE support? Test 'S256' as the string value
898+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
899899
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
900900
}
901901
// Bitbucket servers will have some base url, e.g. https://bitbucket.coder.com.
@@ -975,7 +975,7 @@ func jfrogArtifactoryDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exte
975975
DisplayName: "JFrog Artifactory",
976976
Scopes: []string{"applied-permissions/user"},
977977
DisplayIcon: "/icon/jfrog.svg",
978-
// TODO: PKCE support? Test 'S256' as the string value
978+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
979979
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
980980
}
981981
// Artifactory servers will have some base url, e.g. https://jfrog.coder.com.
@@ -1047,7 +1047,7 @@ func azureDevopsEntraDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exte
10471047
DisplayName: "Azure DevOps (Entra)",
10481048
DisplayIcon: "/icon/azure-devops.svg",
10491049
Regex: `^(https?://)?dev\.azure\.com(/.*)?$`,
1050-
// TODO: PKCE support? Test 'S256' as the string value
1050+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
10511051
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
10521052
}
10531053
// The tenant ID is required for urls and is in the auth url.
@@ -1087,7 +1087,7 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External
10871087
DisplayIcon: "/icon/azure-devops.svg",
10881088
Regex: `^(https?://)?dev\.azure\.com(/.*)?$`,
10891089
Scopes: []string{"vso.code_write"},
1090-
// TODO: PKCE support? Test 'S256' as the string value
1090+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
10911091
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
10921092
},
10931093
codersdk.EnhancedExternalAuthProviderBitBucketCloud: {
@@ -1098,7 +1098,7 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External
10981098
DisplayIcon: "/icon/bitbucket.svg",
10991099
Regex: `^(https?://)?bitbucket\.org(/.*)?$`,
11001100
Scopes: []string{"account", "repository:write"},
1101-
// TODO: PKCE support? Test 'S256' as the string value
1101+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
11021102
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
11031103
},
11041104
codersdk.EnhancedExternalAuthProviderSlack: {
@@ -1109,7 +1109,7 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External
11091109
DisplayIcon: "/icon/slack.svg",
11101110
// See: https://api.slack.com/authentication/oauth-v2#exchanging
11111111
ExtraTokenKeys: []string{"authed_user"},
1112-
// TODO: PKCE support? Test 'S256' as the string value
1112+
// TODO: Investigate if 'S256' is accepted and PKCE is supported
11131113
CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)},
11141114
},
11151115
}

‎coderd/userauth.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,10 @@ type GithubOAuth2Config struct {
770770
DefaultProviderConfigured bool
771771
}
772772

773+
func (*GithubOAuth2Config) PKCESupported() []promoauth.Oauth2PKCEChallengeMethod {
774+
return []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256}
775+
}
776+
773777
func (c *GithubOAuth2Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
774778
if !c.DeviceFlowEnabled {
775779
return c.OAuth2Config.Exchange(ctx, code, opts...)

0 commit comments

Comments
 (0)