From 28aaa8f5117d30d6da3cc23b0b1e5da6d3711999 Mon Sep 17 00:00:00 2001
From: Maksim Nabokikh <maksim.nabokikh@flant.com>
Date: Fri, 14 Apr 2023 00:15:17 +0400
Subject: [PATCH] fix: Do not skip approval screen by default (#2897)

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
---
 server/handlers.go      |  2 +-
 server/handlers_test.go | 58 +++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/server/handlers.go b/server/handlers.go
index f25d52f531..08004c6d0e 100755
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -523,7 +523,7 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth
 		authReq.ConnectorID, claims.Username, claims.PreferredUsername, email, claims.Groups)
 
 	// we can skip the redirect to /approval and go ahead and send code if it's not required
-	if s.skipApproval || !authReq.ForceApprovalPrompt {
+	if s.skipApproval && !authReq.ForceApprovalPrompt {
 		return "", true, nil
 	}
 
diff --git a/server/handlers_test.go b/server/handlers_test.go
index d49e72415c..b9340c9bd5 100644
--- a/server/handlers_test.go
+++ b/server/handlers_test.go
@@ -10,7 +10,6 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"path"
-	"strings"
 	"testing"
 	"time"
 
@@ -339,6 +338,7 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 		name         string
 		skipApproval bool
 		authReq      storage.AuthRequest
+		expectedRes  string
 	}{
 		{
 			name:         "Force approval",
@@ -351,6 +351,7 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: true,
 			},
+			expectedRes: "/approval",
 		},
 		{
 			name:         "Skip approval by server config",
@@ -363,9 +364,10 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: true,
 			},
+			expectedRes: "/approval",
 		},
 		{
-			name:         "Skip approval by auth request",
+			name:         "No skip",
 			skipApproval: false,
 			authReq: storage.AuthRequest{
 				ID:                  authReqID,
@@ -375,6 +377,20 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: false,
 			},
+			expectedRes: "/approval",
+		},
+		{
+			name:         "Skip approval",
+			skipApproval: true,
+			authReq: storage.AuthRequest{
+				ID:                  authReqID,
+				ConnectorID:         connID,
+				RedirectURI:         "cb",
+				Expiry:              expiry,
+				ResponseTypes:       resTypes,
+				ForceApprovalPrompt: false,
+			},
+			expectedRes: "/auth/mockPw/cb",
 		},
 	}
 
@@ -410,15 +426,11 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 		require.Equal(t, 303, rr.Code)
 
 		resp := rr.Result()
-		cbPath := strings.Split(resp.Header.Get("Location"), "?")[0]
 
-		if tc.skipApproval || !tc.authReq.ForceApprovalPrompt {
-			require.Equal(t, "/auth/mockPw/cb", cbPath)
-		} else {
-			require.Equal(t, "/approval", cbPath)
-		}
+		defer resp.Body.Close()
 
-		resp.Body.Close()
+		cb, _ := url.Parse(resp.Header.Get("Location"))
+		require.Equal(t, tc.expectedRes, cb.Path)
 	}
 }
 
@@ -435,6 +447,7 @@ func TestHandleConnectorCallbackWithSkipApproval(t *testing.T) {
 		name         string
 		skipApproval bool
 		authReq      storage.AuthRequest
+		expectedRes  string
 	}{
 		{
 			name:         "Force approval",
@@ -447,6 +460,7 @@ func TestHandleConnectorCallbackWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: true,
 			},
+			expectedRes: "/approval",
 		},
 		{
 			name:         "Skip approval by server config",
@@ -459,6 +473,7 @@ func TestHandleConnectorCallbackWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: true,
 			},
+			expectedRes: "/approval",
 		},
 		{
 			name:         "Skip approval by auth request",
@@ -471,6 +486,20 @@ func TestHandleConnectorCallbackWithSkipApproval(t *testing.T) {
 				ResponseTypes:       resTypes,
 				ForceApprovalPrompt: false,
 			},
+			expectedRes: "/approval",
+		},
+		{
+			name:         "Skip approval",
+			skipApproval: true,
+			authReq: storage.AuthRequest{
+				ID:                  authReqID,
+				ConnectorID:         connID,
+				RedirectURI:         "cb",
+				Expiry:              expiry,
+				ResponseTypes:       resTypes,
+				ForceApprovalPrompt: false,
+			},
+			expectedRes: "/callback/cb",
 		},
 	}
 
@@ -492,14 +521,9 @@ func TestHandleConnectorCallbackWithSkipApproval(t *testing.T) {
 		require.Equal(t, 303, rr.Code)
 
 		resp := rr.Result()
-		cbPath := strings.Split(resp.Header.Get("Location"), "?")[0]
-
-		if tc.skipApproval || !tc.authReq.ForceApprovalPrompt {
-			require.Equal(t, "/callback/cb", cbPath)
-		} else {
-			require.Equal(t, "/approval", cbPath)
-		}
+		defer resp.Body.Close()
 
-		resp.Body.Close()
+		cb, _ := url.Parse(resp.Header.Get("Location"))
+		require.Equal(t, tc.expectedRes, cb.Path)
 	}
 }