Skip to content

Commit a910050

Browse files
pzadrogapzadroga
committed
[#2473] more verbose logs
1 parent 880dad8 commit a910050

2 files changed

Lines changed: 116 additions & 11 deletions

File tree

‎backend/server/oidc/controller.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -380,12 +380,19 @@ func (ctl *Controller) callbackHandler(w http.ResponseWriter, r *http.Request) {
380380
ctx := r.Context()
381381
ctl.cleanupSessions(ctx)
382382

383+
var (
384+
ok bool
385+
authSession AuthSession
386+
sessionMap map[string]AuthSession
387+
)
383388
// Get cached data for received state.
384389
state := r.URL.Query().Get("state")
385-
sessionMap := ctl.getAuthSessionMap(ctx)
386-
authSession, ok := sessionMap[state]
390+
if len(state) > 0 {
391+
sessionMap = ctl.getAuthSessionMap(ctx)
392+
authSession, ok = sessionMap[state]
393+
}
387394
if !ok {
388-
log.Warn("OIDC callback endpoint received invalid or expired state")
395+
log.WithField("state", state).Warn("OIDC callback endpoint received invalid or expired state")
389396
http.Redirect(w, r, authErrorURLPath, http.StatusFound)
390397
return
391398
}
@@ -405,6 +412,11 @@ func (ctl *Controller) callbackHandler(w http.ResponseWriter, r *http.Request) {
405412

406413
// Do the exchange with token endpoint and verify the response.
407414
code := r.URL.Query().Get("code")
415+
if len(code) == 0 {
416+
log.Error("Code is missing in the OIDC authentication response")
417+
http.Redirect(w, r, authErrorURLPath, http.StatusFound)
418+
return
419+
}
408420
token, err := ctl.oauth2Config.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", codeVerifier))
409421
if err != nil {
410422
log.WithError(err).Error("Error while exchanging OIDC token")
@@ -425,7 +437,11 @@ func (ctl *Controller) callbackHandler(w http.ResponseWriter, r *http.Request) {
425437
}
426438

427439
if idToken.Nonce != expectedNonce {
428-
log.Error("Error while verifying OIDC token response - invalid nonce")
440+
logFields := log.Fields{
441+
"idTokenNonce": idToken.Nonce,
442+
"expectedNonce": expectedNonce,
443+
}
444+
log.WithFields(logFields).Error("Error while verifying OIDC token response - invalid nonce")
429445
http.Redirect(w, r, authErrorURLPath, http.StatusFound)
430446
return
431447
}

‎backend/server/oidc/controller_test.go‎

Lines changed: 96 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ func TestCallbackEndpointHandlesError(t *testing.T) {
539539
state := parsedURL.Query().Get("state")
540540

541541
// Send request to callback endpoint.
542-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state+"&error=123&error_description=testError", nil)
542+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state+"&error=123&error_description=testError&code=foobar", nil)
543543
w2 := httptest.NewRecorder()
544544
populateCookies(resp, req2)
545545
handler.ServeHTTP(w2, req2)
@@ -607,7 +607,7 @@ func TestCallbackEndpointHandlesTokenExchangeError(t *testing.T) {
607607
state := parsedURL.Query().Get("state")
608608

609609
// Send request to callback endpoint.
610-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
610+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
611611
w2 := httptest.NewRecorder()
612612
populateCookies(resp, req2)
613613
handler.ServeHTTP(w2, req2)
@@ -659,7 +659,7 @@ func TestCallbackEndpointHandlesTokenRespVerificationError(t *testing.T) {
659659
state := parsedURL.Query().Get("state")
660660

661661
// Send request to callback endpoint.
662-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
662+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
663663
w2 := httptest.NewRecorder()
664664
populateCookies(resp, req2)
665665
handler.ServeHTTP(w2, req2)
@@ -710,7 +710,7 @@ func TestCallbackEndpointHandlesWrongNonce(t *testing.T) {
710710
state := parsedURL.Query().Get("state")
711711

712712
// Send request to callback endpoint.
713-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
713+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
714714
w2 := httptest.NewRecorder()
715715
populateCookies(resp, req2)
716716
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w2, req2)
@@ -760,7 +760,7 @@ func TestCallbackEndpointHandlesUnauthorizedUser(t *testing.T) {
760760
state := parsedURL.Query().Get("state")
761761

762762
// Send request to callback endpoint.
763-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
763+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
764764
w2 := httptest.NewRecorder()
765765
populateCookies(resp, req2)
766766
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w2, req2)
@@ -820,7 +820,7 @@ func TestCallbackEndpointAuthorizesUser(t *testing.T) {
820820
state := parsedURL.Query().Get("state")
821821

822822
// Send request to callback endpoint.
823-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
823+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
824824
w2 := httptest.NewRecorder()
825825
populateCookies(resp, req2)
826826
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w2, req2)
@@ -887,7 +887,7 @@ func TestCallbackEndpointAuthorizesUserGroupMappingDisabled(t *testing.T) {
887887
state := parsedURL.Query().Get("state")
888888

889889
// Send request to callback endpoint.
890-
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil)
890+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar&state="+state, nil)
891891
w2 := httptest.NewRecorder()
892892
populateCookies(resp, req2)
893893
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w2, req2)
@@ -977,3 +977,92 @@ func TestExtractGroupsFromClaim(t *testing.T) {
977977
require.Contains(t, res, "groupC")
978978
})
979979
}
980+
981+
// Test that callback handler handles empty state.
982+
func TestCallbackEndpointHandlesEmptyState(t *testing.T) {
983+
// Arrange
984+
db, _, teardown := dbtest.SetupDatabaseTestCase(t)
985+
defer teardown()
986+
issuerURL, srvTeardown, err := oidctest.PrepareTestOIDCServer()
987+
require.NoError(t, err)
988+
defer srvTeardown()
989+
controller := NewController(Settings{IssuerURL: issuerURL, ClientID: "clientID"}, db)
990+
require.NotNil(t, controller)
991+
testSM, err := dbsession.NewSessionMgr(db)
992+
require.NoError(t, err)
993+
err = controller.Configure(url.URL{Scheme: "http", Path: "localhost"}, testSM)
994+
require.NoError(t, err)
995+
require.True(t, controller.configured)
996+
997+
nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
998+
// empty handler
999+
})
1000+
handler := controller.Middleware(nextHandler)
1001+
1002+
// Act
1003+
req := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?code=foobar", nil) // state is missing.
1004+
w := httptest.NewRecorder()
1005+
handler.ServeHTTP(w, req)
1006+
resp := w.Result()
1007+
resp.Body.Close()
1008+
1009+
// Assert
1010+
require.Equal(t, http.StatusFound, resp.StatusCode)
1011+
require.Greater(t, len(resp.Header), 2)
1012+
// Check auth_session cookie.
1013+
require.Contains(t, resp.Header, "Set-Cookie")
1014+
require.Contains(t, resp.Header.Get("Set-Cookie"), "auth_session")
1015+
// Check redirect Location header. It should redirect to login page showing brief error feedback message.
1016+
require.Contains(t, resp.Header, "Location")
1017+
require.Contains(t, resp.Header.Get("Location"), "/login/auth-err")
1018+
}
1019+
1020+
// Test that callback handler handles empty code.
1021+
func TestCallbackEndpointHandlesEmptyCode(t *testing.T) {
1022+
// Arrange
1023+
db, _, teardown := dbtest.SetupDatabaseTestCase(t)
1024+
defer teardown()
1025+
issuerURL, srvTeardown, err := oidctest.PrepareTestOIDCServer()
1026+
require.NoError(t, err)
1027+
defer srvTeardown()
1028+
controller := NewController(Settings{IssuerURL: issuerURL, ClientID: "clientID"}, db)
1029+
require.NotNil(t, controller)
1030+
testSM, err := dbsession.NewSessionMgr(db)
1031+
require.NoError(t, err)
1032+
err = controller.Configure(url.URL{Scheme: "http", Path: "localhost"}, testSM)
1033+
require.NoError(t, err)
1034+
require.True(t, controller.configured)
1035+
1036+
nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1037+
// empty handler
1038+
})
1039+
handler := nonceModifier(controller.Middleware(nextHandler), controller, "test-nonce")
1040+
1041+
// Act
1042+
// First send request to login endpoint to retrieve random state for the authentication.
1043+
req := httptest.NewRequest("GET", "http://localhost"+loginURLPath, nil)
1044+
w := httptest.NewRecorder()
1045+
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w, req)
1046+
resp := w.Result()
1047+
resp.Body.Close()
1048+
require.Equal(t, http.StatusFound, resp.StatusCode)
1049+
require.Contains(t, resp.Header, "Location")
1050+
redirectURL := resp.Header.Get("Location")
1051+
parsedURL, err := url.Parse(redirectURL)
1052+
require.NoError(t, err)
1053+
state := parsedURL.Query().Get("state")
1054+
1055+
// Send request to callback endpoint.
1056+
req2 := httptest.NewRequest("GET", "http://localhost"+callbackURLPath+"?state="+state, nil) // code is missing.
1057+
w2 := httptest.NewRecorder()
1058+
populateCookies(resp, req2)
1059+
controller.authSessionManager.LoadAndSave(handler).ServeHTTP(w2, req2)
1060+
resp2 := w2.Result()
1061+
resp2.Body.Close()
1062+
1063+
// Assert
1064+
require.Equal(t, http.StatusFound, resp2.StatusCode)
1065+
// Check redirect Location header. It should redirect to login page showing brief error feedback message.
1066+
require.Contains(t, resp2.Header, "Location")
1067+
require.Contains(t, resp2.Header.Get("Location"), "/login/auth-err")
1068+
}

0 commit comments

Comments
 (0)