From 84edfcb541982b703048d4abec20477d4db9801f Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 26 Oct 2021 17:03:16 -0700 Subject: [PATCH] Refactor out a function, add tests for getting the wrong idp uid --- internal/oidc/token/token_handler.go | 24 +++++--- internal/oidc/token/token_handler_test.go | 70 ++++++++++++++++++++--- internal/upstreamldap/upstreamldap.go | 7 +-- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index d26b8642..302321b3 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -75,15 +75,10 @@ func NewHandler( func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, providerCache oidc.UpstreamIdentityProvidersLister) error { session := accessRequest.GetSession().(*psession.PinnipedSession) - extra := session.Fosite.Claims.Extra - if extra == nil { - return errorsx.WithStack(errMissingUpstreamSessionInternalError) + downstreamUsername, err := getDownstreamUsernameFromPinnipedSession(session) + if err != nil { + return err } - downstreamUsernameInterface := extra["username"] - if downstreamUsernameInterface == nil { - return errorsx.WithStack(errMissingUpstreamSessionInternalError) - } - downstreamUsername := downstreamUsernameInterface.(string) downstreamSubject := session.Fosite.Claims.Subject customSessionData := session.Custom @@ -225,3 +220,16 @@ func findLDAPProviderByNameAndValidateUID( return nil, "", errorsx.WithStack(errUpstreamRefreshError. WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) } + +func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) (string, error) { + extra := session.Fosite.Claims.Extra + if extra == nil { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + downstreamUsernameInterface := extra["username"] + if downstreamUsernameInterface == nil { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + downstreamUsername := downstreamUsernameInterface.(string) + return downstreamUsername, nil +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0cec17b6..f8a7ccd4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1644,10 +1644,10 @@ func TestRefreshGrant(t *testing.T) { }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - LDAP: nil, + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: nil, }, ), }, @@ -1705,9 +1705,9 @@ func TestRefreshGrant(t *testing.T) { }, { name: "upstream active directory refresh when the active directory session data does not contain dn", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ - Name: ldapUpstreamName, - ResourceUID: ldapUpstreamResourceUID, + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: activeDirectoryUpstreamResourceUID, URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ @@ -1716,7 +1716,7 @@ func TestRefreshGrant(t *testing.T) { ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ + ActiveDirectory: &psession.ActiveDirectorySessionData{ UserDN: "", }, }, @@ -1725,7 +1725,7 @@ func TestRefreshGrant(t *testing.T) { ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ + ActiveDirectory: &psession.ActiveDirectorySessionData{ UserDN: "", }, }, @@ -1922,6 +1922,58 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "when the ldap provider in the session storage is found but has the wrong resource UID during the refresh request", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: "the-wrong-uid", + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ldap-idp' of type 'ldap' from upstream session data has changed its resource UID since authentication." + } + `), + }, + }, + }, + { + name: "when the active directory provider in the session storage is found but has the wrong resource UID during the refresh request", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: "the-wrong-uid", + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyActiveDirectoryCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyActiveDirectoryCustomSessionData, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ad-idp' of type 'activedirectory' from upstream session data has changed its resource UID since authentication." + } + `), + }, + }, + }, } for _, test := range tests { test := test diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 33ca62f5..976b96b8 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -210,7 +210,7 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUs newUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, userDN) if err != nil { - return err // TODO test having no values or more than one maybe + return err } if newUsername != expectedUsername { return fmt.Errorf(`searching for user "%s" returned a different username than the previous value. expected: "%s", actual: "%s"`, @@ -220,15 +220,14 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUs newUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, userDN) if err != nil { - return err // TODO test + return err } newSubject := downstreamsession.DownstreamLDAPSubject(newUID, *p.GetURL()) if newSubject != expectedSubject { return fmt.Errorf(`searching for user "%s" produced a different subject than the previous value. expected: "%s", actual: "%s"`, userDN, expectedSubject, newSubject) } - // do nothing. if we got exactly one search result back then that means the user - // still exists. + // we checked that the user still exists and their information is the same, so just return. return nil }