Use /callback (without IDP name) path for callback endpoint (part 1)

This is much nicer UX for an administrator installing a UpstreamOIDCProvider
CRD. They don't have to guess as hard at what the callback endpoint path should
be for their UpstreamOIDCProvider.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>
This commit is contained in:
Ryan Richard 2020-11-20 16:14:45 -05:00 committed by Andrew Keesler
parent 541019eb98
commit 72321fc106
No known key found for this signature in database
GPG Key ID: 27CE0444346F9413
4 changed files with 27 additions and 11 deletions

View File

@ -92,11 +92,18 @@ func NewHandler(
Endpoint: oauth2.Endpoint{ Endpoint: oauth2.Endpoint{
AuthURL: upstreamIDP.GetAuthorizationURL().String(), AuthURL: upstreamIDP.GetAuthorizationURL().String(),
}, },
RedirectURL: fmt.Sprintf("%s/callback/%s", downstreamIssuer, upstreamIDP.GetName()), RedirectURL: fmt.Sprintf("%s/callback", downstreamIssuer),
Scopes: upstreamIDP.GetScopes(), Scopes: upstreamIDP.GetScopes(),
} }
encodedStateParamValue, err := upstreamStateParam(authorizeRequester, nonceValue, csrfValue, pkceValue, upstreamStateEncoder) encodedStateParamValue, err := upstreamStateParam(
authorizeRequester,
upstreamIDP.GetName(),
nonceValue,
csrfValue,
pkceValue,
upstreamStateEncoder,
)
if err != nil { if err != nil {
plog.Error("authorize upstream state param error", err) plog.Error("authorize upstream state param error", err)
return err return err
@ -188,6 +195,7 @@ func generateValues(
func upstreamStateParam( func upstreamStateParam(
authorizeRequester fosite.AuthorizeRequester, authorizeRequester fosite.AuthorizeRequester,
upstreamName string,
nonceValue nonce.Nonce, nonceValue nonce.Nonce,
csrfValue csrftoken.CSRFToken, csrfValue csrftoken.CSRFToken,
pkceValue pkce.Code, pkceValue pkce.Code,
@ -195,6 +203,7 @@ func upstreamStateParam(
) (string, error) { ) (string, error) {
stateParamData := oidc.UpstreamStateParamData{ stateParamData := oidc.UpstreamStateParamData{
AuthParams: authorizeRequester.GetRequestForm().Encode(), AuthParams: authorizeRequester.GetRequestForm().Encode(),
UpstreamName: upstreamName,
Nonce: nonceValue, Nonce: nonceValue,
CSRFToken: csrfValue, CSRFToken: csrfValue,
PKCECode: pkceValue, PKCECode: pkceValue,

View File

@ -204,14 +204,19 @@ func TestAuthorizationEndpoint(t *testing.T) {
return pathWithQuery("/some/path", modifiedHappyGetRequestQueryMap(queryOverrides)) return pathWithQuery("/some/path", modifiedHappyGetRequestQueryMap(queryOverrides))
} }
expectedUpstreamStateParam := func(queryOverrides map[string]string, csrfValueOverride string) string { expectedUpstreamStateParam := func(queryOverrides map[string]string, csrfValueOverride, upstreamNameOverride string) string {
csrf := happyCSRF csrf := happyCSRF
if csrfValueOverride != "" { if csrfValueOverride != "" {
csrf = csrfValueOverride csrf = csrfValueOverride
} }
upstreamName := upstreamOIDCIdentityProvider.Name
if upstreamNameOverride != "" {
upstreamName = upstreamNameOverride
}
encoded, err := happyStateEncoder.Encode("s", encoded, err := happyStateEncoder.Encode("s",
oidctestutil.ExpectedUpstreamStateParamFormat{ oidctestutil.ExpectedUpstreamStateParamFormat{
P: encodeQuery(modifiedHappyGetRequestQueryMap(queryOverrides)), P: encodeQuery(modifiedHappyGetRequestQueryMap(queryOverrides)),
U: upstreamName,
N: happyNonce, N: happyNonce,
C: csrf, C: csrf,
K: happyPKCE, K: happyPKCE,
@ -232,7 +237,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
"nonce": happyNonce, "nonce": happyNonce,
"code_challenge": expectedUpstreamCodeChallenge, "code_challenge": expectedUpstreamCodeChallenge,
"code_challenge_method": "S256", "code_challenge_method": "S256",
"redirect_uri": downstreamIssuer + "/callback/some-idp", "redirect_uri": downstreamIssuer + "/callback",
}) })
} }
@ -281,7 +286,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantStatus: http.StatusFound, wantStatus: http.StatusFound,
wantContentType: "text/html; charset=utf-8", wantContentType: "text/html; charset=utf-8",
wantCSRFValueInCookieHeader: happyCSRF, wantCSRFValueInCookieHeader: happyCSRF,
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "")), wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")),
wantUpstreamStateParamInLocationHeader: true, wantUpstreamStateParamInLocationHeader: true,
wantBodyStringWithLocationInHref: true, wantBodyStringWithLocationInHref: true,
}, },
@ -299,7 +304,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue, csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue,
wantStatus: http.StatusFound, wantStatus: http.StatusFound,
wantContentType: "text/html; charset=utf-8", wantContentType: "text/html; charset=utf-8",
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, incomingCookieCSRFValue)), wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, incomingCookieCSRFValue, "")),
wantUpstreamStateParamInLocationHeader: true, wantUpstreamStateParamInLocationHeader: true,
wantBodyStringWithLocationInHref: true, wantBodyStringWithLocationInHref: true,
}, },
@ -320,7 +325,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantContentType: "", wantContentType: "",
wantBodyString: "", wantBodyString: "",
wantCSRFValueInCookieHeader: happyCSRF, wantCSRFValueInCookieHeader: happyCSRF,
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "")), wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")),
wantUpstreamStateParamInLocationHeader: true, wantUpstreamStateParamInLocationHeader: true,
}, },
{ {
@ -341,7 +346,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantCSRFValueInCookieHeader: happyCSRF, wantCSRFValueInCookieHeader: happyCSRF,
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{
"redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client
}, "")), }, "", "")),
wantUpstreamStateParamInLocationHeader: true, wantUpstreamStateParamInLocationHeader: true,
wantBodyStringWithLocationInHref: true, wantBodyStringWithLocationInHref: true,
}, },
@ -538,7 +543,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantContentType: "text/html; charset=utf-8", wantContentType: "text/html; charset=utf-8",
wantCSRFValueInCookieHeader: happyCSRF, wantCSRFValueInCookieHeader: happyCSRF,
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam( wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(
map[string]string{"prompt": "none login", "scope": "email"}, "", map[string]string{"prompt": "none login", "scope": "email"}, "", "",
)), )),
wantUpstreamStateParamInLocationHeader: true, wantUpstreamStateParamInLocationHeader: true,
wantBodyStringWithLocationInHref: true, wantBodyStringWithLocationInHref: true,
@ -787,11 +792,11 @@ func TestAuthorizationEndpoint(t *testing.T) {
"access_type": "offline", "access_type": "offline",
"scope": "other-scope1 other-scope2", "scope": "other-scope1 other-scope2",
"client_id": "some-other-client-id", "client_id": "some-other-client-id",
"state": expectedUpstreamStateParam(nil, ""), "state": expectedUpstreamStateParam(nil, "", newProviderSettings.Name),
"nonce": happyNonce, "nonce": happyNonce,
"code_challenge": expectedUpstreamCodeChallenge, "code_challenge": expectedUpstreamCodeChallenge,
"code_challenge_method": "S256", "code_challenge_method": "S256",
"redirect_uri": downstreamIssuer + "/callback/some-other-idp", "redirect_uri": downstreamIssuer + "/callback",
}, },
) )
test.wantBodyString = fmt.Sprintf(`<a href="%s">Found</a>.%s`, test.wantBodyString = fmt.Sprintf(`<a href="%s">Found</a>.%s`,

View File

@ -66,6 +66,7 @@ type Codec interface {
// the state param. // the state param.
type UpstreamStateParamData struct { type UpstreamStateParamData struct {
AuthParams string `json:"p"` AuthParams string `json:"p"`
UpstreamName string `json:"u"`
Nonce nonce.Nonce `json:"n"` Nonce nonce.Nonce `json:"n"`
CSRFToken csrftoken.CSRFToken `json:"c"` CSRFToken csrftoken.CSRFToken `json:"c"`
PKCECode pkce.Code `json:"k"` PKCECode pkce.Code `json:"k"`

View File

@ -112,6 +112,7 @@ func NewIDPListGetter(upstreamOIDCIdentityProviders ...*TestUpstreamOIDCIdentity
// assertions about the redirect URL in this test. // assertions about the redirect URL in this test.
type ExpectedUpstreamStateParamFormat struct { type ExpectedUpstreamStateParamFormat struct {
P string `json:"p"` P string `json:"p"`
U string `json:"u"`
N string `json:"n"` N string `json:"n"`
C string `json:"c"` C string `json:"c"`
K string `json:"k"` K string `json:"k"`