Repository: Azure/ARO-RP
Pull request #1556 (2021-06-21 00:32:30)

Admin Portal 2.0 -- RESTify the existing API
### Which issue this PR addresses:

Fixes https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/10191797

### What this PR does / why we need it:

The existing REST API does redirects when not authorised, which means that it's difficult to make a more front-end based interface over these APIs.

It does not change the portal UI.

### Test plan for issue:

Existing tests + manual testing of the portal.

### Is there any documentation that needs to be updated for this PR?

N/A



Patch:
diff --git a/pkg/portal/middleware/aad.go b/pkg/portal/middleware/aad.go
index 78bd8887d..0b05cff6e 100644
--- a/pkg/portal/middleware/aad.go
+++ b/pkg/portal/middleware/aad.go
@@ -28,12 +28,11 @@ import (
 )
 
 const (
-	SessionName            = "session"
-	SessionKeyExpires      = "expires"
-	sessionKeyRedirectPath = "redirect_path"
-	sessionKeyState        = "state"
-	SessionKeyUsername     = "user_name"
-	SessionKeyGroups       = "groups"
+	SessionName        = "session"
+	SessionKeyExpires  = "expires"
+	sessionKeyState    = "state"
+	SessionKeyUsername = "user_name"
+	SessionKeyGroups   = "groups"
 )
 
 func init() {
@@ -43,8 +42,9 @@ func init() {
 // AAD is responsible for ensuring that we have a valid login session with AAD.
 type AAD interface {
 	AAD(http.Handler) http.Handler
+	CheckAuthentication(http.Handler) http.Handler
+	Login(http.ResponseWriter, *http.Request)
 	Logout(string) http.Handler
-	Redirect(http.Handler) http.Handler
 }
 
 type oauther interface {
@@ -124,6 +124,8 @@ func NewAAD(log *logrus.Entry,
 	a.store.Options.SameSite = http.SameSiteLaxMode
 
 	unauthenticatedRouter.NewRoute().Methods(http.MethodGet).Path("/callback").Handler(Log(env, audit, baseAccessLog)(http.HandlerFunc(a.callback)))
+	unauthenticatedRouter.NewRoute().Methods(http.MethodGet).Path("/api/login").Handler(Log(env, audit, baseAccessLog)(http.HandlerFunc(a.Login)))
+	unauthenticatedRouter.NewRoute().Methods(http.MethodPost).Path("/api/logout").Handler(Log(env, audit, baseAccessLog)(a.Logout("/")))
 
 	return a, nil
 }
@@ -154,21 +156,28 @@ func (a *aad) AAD(h http.Handler) http.Handler {
 	})
 }
 
-// Redirect is the late stage (post logging) handler which redirects to AAD if
-// there is no valid user.
-func (a *aad) Redirect(h http.Handler) http.Handler {
+// CheckAuthentication is the handler which prevents access to requests without
+// valid authentication.
+func (a *aad) CheckAuthentication(h http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
-
-		if ctx.Value(ContextKeyUsername) != nil {
-			h.ServeHTTP(w, r)
+		if ctx.Value(ContextKeyUsername) == nil {
+			if r.URL != nil && r.URL.Path == "/" {
+				http.Redirect(w, r, "/api/login", http.StatusTemporaryRedirect)
+				return
+			}
+			http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
 			return
 		}
-
-		a.redirect(w, r)
+		h.ServeHTTP(w, r)
 	})
 }
 
+// Login will redirect the user to a login page.
+func (a *aad) Login(w http.ResponseWriter, r *http.Request) {
+	a.redirect(w, r)
+}
+
 func (a *aad) Logout(url string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		session, err := a.store.Get(r, SessionName)
@@ -196,16 +205,10 @@ func (a *aad) redirect(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	path := r.URL.Path
-	if path == "/callback" {
-		path = "/"
-	}
-
 	state := uuid.Must(uuid.NewV4()).String()
 
 	session.Values = map[interface{}]interface{}{
-		sessionKeyRedirectPath: path,
-		sessionKeyState:        state,
+		sessionKeyState: state,
 	}
 
 	err = session.Save(r, w)
@@ -289,13 +292,6 @@ func (a *aad) callback(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
 	}
 
-	redirectPath, ok := session.Values[sessionKeyRedirectPath].(string)
-	if !ok {
-		a.internalServerError(w, errors.New("redirect_path not found"))
-		return
-	}
-
-	delete(session.Values, sessionKeyRedirectPath)
 	session.Values[SessionKeyUsername] = claims.PreferredUsername
 	session.Values[SessionKeyGroups] = groupsIntersect
 	session.Values[SessionKeyExpires] = a.now().Add(a.sessionTimeout)
@@ -306,7 +302,7 @@ func (a *aad) callback(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.Redirect(w, r, redirectPath, http.StatusTemporaryRedirect)
+	http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
 }
 
 // clientAssertion adds a JWT client assertion according to
diff --git a/pkg/portal/portal.go b/pkg/portal/portal.go
index 99e8dfa31..c9bc471a1 100644
--- a/pkg/portal/portal.go
+++ b/pkg/portal/portal.go
@@ -10,6 +10,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
+	"fmt"
 	"html/template"
 	"log"
 	"net"
@@ -38,13 +39,16 @@ type Runnable interface {
 }
 
 type portal struct {
-	env           env.Core
-	audit         *logrus.Entry
-	log           *logrus.Entry
-	baseAccessLog *logrus.Entry
-	l             net.Listener
-	sshl          net.Listener
-	verifier      oidc.Verifier
+	env                 env.Core
+	audit               *logrus.Entry
+	log                 *logrus.Entry
+	baseAccessLog       *logrus.Entry
+	l                   net.Listener
+	sshl                net.Listener
+	verifier            oidc.Verifier
+	baseRouter          *mux.Router
+	authenticatedRouter *mux.Router
+	publicRouter        *mux.Router
 
 	hostname     string
 	servingKey   *rsa.PrivateKey
@@ -88,6 +92,7 @@ func NewPortal(env env.Core,
 	dbOpenShiftClusters database.OpenShiftClusters,
 	dbPortal database.Portal,
 	dialer proxy.Dialer) Runnable {
+
 	return &portal{
 		env:           env,
 		audit:         audit,
@@ -116,7 +121,14 @@ func NewPortal(env env.Core,
 	}
 }
 
-func (p *portal) Run(ctx context.Context) error {
+func (p *portal) setupRouter() error {
+	if p.baseRouter != nil {
+		return fmt.Errorf("can't setup twice")
+	}
+
+	r := mux.NewRouter()
+	r.Use(middleware.Panic(p.log))
+
 	asset, err := Asset("index.html")
 	if err != nil {
 		return err
@@ -127,6 +139,51 @@ func (p *portal) Run(ctx context.Context) error {
 		return err
 	}
 
+	unauthenticatedRouter := r.NewRoute().Subrouter()
+	p.unauthenticatedRoutes(unauthenticatedRouter)
+
+	allGroups := append([]string{}, p.groupIDs...)
+	allGroups = append(allGroups, p.elevatedGroupIDs...)
+
+	p.aad, err = middleware.NewAAD(p.log, p.audit, p.env, p.baseAccessLog, p.hostname, p.sessionKey, p.clientID, p.clientKey, p.clientCerts, allGroups, unauthenticatedRouter, p.verifier)
+	if err != nil {
+		return err
+	}
+
+	aadAuthenticatedRouter := r.NewRoute().Subrouter()
+	aadAuthenticatedRouter.Use(p.aad.AAD)
+	aadAuthenticatedRouter.Use(middleware.Log(p.env, p.audit, p.baseAccessLog))
+	aadAuthenticatedRouter.Use(p.aad.CheckAuthentication)
+	aadAuthenticatedRouter.Use(csrf.Protect(p.sessionKey, csrf.SameSite(csrf.SameSiteStrictMode), csrf.MaxAge(0)))
+
+	p.aadAuthenticatedRoutes(aadAuthenticatedRouter)
+
+	p.baseRouter = r
+	p.publicRouter = unauthenticatedRouter
+	p.authenticatedRouter = aadAuthenticatedRouter
+
+	return nil
+}
+
+func (p *portal) setupServices() error {
+	ssh, err := ssh.New(p.env, p.log, p.baseAccessLog, p.sshl, p.sshKey, p.elevatedGroupIDs, p.dbOpenShiftClusters, p.dbPortal, p.dialer, p.authenticatedRouter)
+	if err != nil {
+		return err
+	}
+
+	err = ssh.Run()
+	if err != nil {
+		return err
+	}
+
+	kubeconfig.New(p.log, p.audit, p.env, p.baseAccessLog, p.servingCerts[0], p.elevatedGroupIDs, p.dbOpenShiftClusters, p.dbPortal, p.dialer, p.authenticatedRouter, p.publicRouter)
+
+	prometheus.New(p.log, p.dbOpenShiftClusters, p.dialer, p.authenticatedRouter)
+
+	return nil
+}
+
+func (p *portal) Run(ctx context.Context) error {
 	config := &tls.Config{
 		Certificates: []tls.Certificate{
 			{
@@ -155,44 +212,19 @@ func (p *portal) Run(ctx context.Context) error {
 		config.Certificates[0].Certificate = append(config.Certificates[0].Certificate, cert.Raw)
 	}
 
-	r := mux.NewRouter()
-	r.Use(middleware.Panic(p.log))
-
-	unauthenticatedRouter := r.NewRoute().Subrouter()
-	p.unauthenticatedRoutes(unauthenticatedRouter)
-
-	allGroups := append([]string{}, p.groupIDs...)
-	allGroups = append(allGroups, p.elevatedGroupIDs...)
-
-	p.aad, err = middleware.NewAAD(p.log, p.audit, p.env, p.baseAccessLog, p.hostname, p.sessionKey, p.clientID, p.clientKey, p.clientCerts, allGroups, unauthenticatedRouter, p.verifier)
-	if err != nil {
-		return err
-	}
-
-	aadAuthenticatedRouter := r.NewRoute().Subrouter()
-	aadAuthenticatedRouter.Use(p.aad.AAD)
-	aadAuthenticatedRouter.Use(middleware.Log(p.env, p.audit, p.baseAccessLog))
-	aadAuthenticatedRouter.Use(p.aad.Redirect)
-	aadAuthenticatedRouter.Use(csrf.Protect(p.sessionKey, csrf.SameSite(csrf.SameSiteStrictMode), csrf.MaxAge(0)))
-
-	p.aadAuthenticatedRoutes(aadAuthenticatedRouter)
-
-	ssh, err := ssh.New(p.env, p.log, p.baseAccessLog, p.sshl, p.sshKey, p.elevatedGroupIDs, p.dbOpenShiftClusters, p.dbPortal, p.dialer, aadAuthenticatedRouter)
-	if err != nil {
-		return err
-	}
-
-	err = ssh.Run()
-	if err != nil {
-		return err
+	if p.baseRouter == nil {
+		err := p.setupRouter()
+		if err != nil {
+			return err
+		}
+		err = p.setupServices()
+		if err != nil {
+			return err
+		}
 	}
 
-	kubeconfig.New(p.log, p.audit, p.env, p.baseAccessLog, p.servingCerts[0], p.elevatedGroupIDs, p.dbOpenShiftClusters, p.dbPortal, p.dialer, aadAuthenticatedRouter, unauthenticatedRouter)
-
-	prometheus.New(p.log, p.dbOpenShiftClusters, p.dialer, aadAuthenticatedRouter)
-
 	s := &http.Server{
-		Handler:     frontendmiddleware.Lowercase(r),
+		Handler:     frontendmiddleware.Lowercase(p.baseRouter),
 		ReadTimeout: 10 * time.Second,
 		IdleTimeout: 2 * time.Minute,
 		ErrorLog:    log.New(p.log.Writer(), "", 0),
@@ -220,7 +252,6 @@ func (p *portal) aadAuthenticatedRoutes(r *mux.Router) {
 	r.NewRoute().Methods(http.MethodGet).Path("/").HandlerFunc(p.index)
 
 	r.NewRoute().Methods(http.MethodGet).Path("/api/clusters").HandlerFunc(p.clusters)
-	r.NewRoute().Methods(http.MethodPost).Path("/api/logout").Handler(p.aad.Logout("/"))
 }
 
 func (p *portal) serve(path string) func(w http.ResponseWriter, r *http.Request) {


Test patch:
diff --git a/pkg/portal/portal_test.go b/pkg/portal/cluster_test.go
similarity index 65%
rename from pkg/portal/portal_test.go
rename to pkg/portal/cluster_test.go
index 93fd0f7d4..473a90b00 100644
--- a/pkg/portal/portal_test.go
+++ b/pkg/portal/cluster_test.go
@@ -7,9 +7,10 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
-	"reflect"
 	"testing"
 
+	"github.com/go-test/deep"
+
 	"github.com/Azure/ARO-RP/pkg/api"
 	testdatabase "github.com/Azure/ARO-RP/test/database"
 )
@@ -21,6 +22,7 @@ func TestClusters(t *testing.T) {
 		WithOpenShiftClusters(dbOpenShiftClusters)
 
 	fixture.AddOpenShiftClusterDocuments(&api.OpenShiftClusterDocument{
+		ID:  "00000000-0000-0000-0000-000000000000",
 		Key: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourcegroupname/providers/microsoft.redhatopenshift/openshiftclusters/succeeded",
 		OpenShiftCluster: &api.OpenShiftCluster{
 			ID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroupName/providers/microsoft.redhatopenshift/openshiftclusters/succeeded",
@@ -29,6 +31,7 @@ func TestClusters(t *testing.T) {
 			},
 		},
 	}, &api.OpenShiftClusterDocument{
+		ID:  "00000000-0000-0000-0000-000000000001",
 		Key: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourcegroupname/providers/microsoft.redhatopenshift/openshiftclusters/creating",
 		OpenShiftCluster: &api.OpenShiftCluster{
 			ID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroupName/providers/microsoft.redhatopenshift/openshiftclusters/creating",
@@ -36,7 +39,18 @@ func TestClusters(t *testing.T) {
 				ProvisioningState: api.ProvisioningStateCreating,
 			},
 		},
-	})
+	},
+		&api.OpenShiftClusterDocument{
+			ID:  "00000000-0000-0000-0000-000000000002",
+			Key: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourcegroupname/providers/microsoft.redhatopenshift/openshiftclusters/failedcreate",
+			OpenShiftCluster: &api.OpenShiftCluster{
+				ID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroupName/providers/microsoft.redhatopenshift/openshiftclusters/failedcreate",
+				Properties: api.OpenShiftClusterProperties{
+					ProvisioningState:       api.ProvisioningStateFailed,
+					FailedProvisioningState: api.ProvisioningStateCreating,
+				},
+			},
+		})
 
 	err := fixture.Create()
 	if err != nil {
@@ -61,7 +75,11 @@ func TestClusters(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if !reflect.DeepEqual(r, []string{"/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroupName/providers/microsoft.redhatopenshift/openshiftclusters/succeeded"}) {
-		t.Error(r)
+	expected := []string{
+		"/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroupName/providers/microsoft.redhatopenshift/openshiftclusters/succeeded",
+	}
+
+	for _, l := range deep.Equal(expected, r) {
+		t.Error(l)
 	}
 }
diff --git a/pkg/portal/middleware/aad_test.go b/pkg/portal/middleware/aad_test.go
index 01c4ac2dd..a62212e20 100644
--- a/pkg/portal/middleware/aad_test.go
+++ b/pkg/portal/middleware/aad_test.go
@@ -19,6 +19,7 @@ import (
 
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/form3tech-oss/jwt-go"
+	"github.com/go-test/deep"
 	"github.com/gofrs/uuid"
 	"github.com/golang/mock/gomock"
 	"github.com/gorilla/mux"
@@ -193,7 +194,7 @@ func TestAAD(t *testing.T) {
 	}
 }
 
-func TestRedirect(t *testing.T) {
+func TestCheckAuthentication(t *testing.T) {
 	for _, tt := range []struct {
 		name              string
 		request           func(*aad) (*http.Request, error)
@@ -205,18 +206,18 @@ func TestRedirect(t *testing.T) {
 			request: func(a *aad) (*http.Request, error) {
 				ctx := context.Background()
 				ctx = context.WithValue(ctx, ContextKeyUsername, "user")
-				return http.NewRequestWithContext(ctx, http.MethodGet, "/", nil)
+				return http.NewRequestWithContext(ctx, http.MethodGet, "/api/info", nil)
 			},
-			wantStatusCode:    http.StatusOK,
 			wantAuthenticated: true,
+			wantStatusCode:    http.StatusOK,
 		},
 		{
 			name: "not authenticated",
 			request: func(a *aad) (*http.Request, error) {
 				ctx := context.Background()
-				return http.NewRequestWithContext(ctx, http.MethodGet, "/", nil)
+				return http.NewRequestWithContext(ctx, http.MethodGet, "/api/info", nil)
 			},
-			wantStatusCode: http.StatusTemporaryRedirect,
+			wantStatusCode: http.StatusForbidden,
 		},
 		{
 			name: "not authenticated",
@@ -224,7 +225,7 @@ func TestRedirect(t *testing.T) {
 				ctx := context.Background()
 				return http.NewRequestWithContext(ctx, http.MethodGet, "/callback", nil)
 			},
-			wantStatusCode: http.StatusTemporaryRedirect,
+			wantStatusCode: http.StatusForbidden,
 		},
 		{
 			name: "invalid cookie",
@@ -235,7 +236,7 @@ func TestRedirect(t *testing.T) {
 					},
 				}, nil
 			},
-			wantStatusCode: http.StatusInternalServerError,
+			wantStatusCode: http.StatusForbidden,
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -254,7 +255,7 @@ func TestRedirect(t *testing.T) {
 			}
 
 			var authenticated bool
-			h := a.Redirect(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			h := a.CheckAuthentication(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				_, authenticated = r.Context().Value(ContextKeyUsername).(string)
 			}))
 
@@ -268,7 +269,7 @@ func TestRedirect(t *testing.T) {
 			h.ServeHTTP(w, r)
 
 			if w.Code != tt.wantStatusCode {
-				t.Error(w.Code)
+				t.Error(w.Code, tt.wantStatusCode)
 			}
 
 			if authenticated != tt.wantAuthenticated {
@@ -278,33 +279,83 @@ func TestRedirect(t *testing.T) {
 			if tt.wantStatusCode == http.StatusInternalServerError {
 				return
 			}
+		})
+	}
+}
 
-			if !tt.wantAuthenticated {
-				if !strings.HasPrefix(w.Header().Get("Location"), "https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=&redirect_uri=https%3A%2F%2F%2Fcallback&response_type=code&scope=openid+profile&state=") {
-					t.Error(w.Header().Get("Location"))
-				}
+func TestLogin(t *testing.T) {
+	for _, tt := range []struct {
+		name           string
+		request        func(*aad) (*http.Request, error)
+		wantStatusCode int
+	}{
+		{
+			name: "authenticated",
+			request: func(a *aad) (*http.Request, error) {
+				ctx := context.Background()
+				ctx = context.WithValue(ctx, ContextKeyUsername, "user")
+				return http.NewRequestWithContext(ctx, http.MethodGet, "/login", nil)
+			},
+			wantStatusCode: http.StatusTemporaryRedirect,
+		},
+		{
+			name: "not authenticated",
+			request: func(a *aad) (*http.Request, error) {
+				ctx := context.Background()
+				return http.NewRequestWithContext(ctx, http.MethodGet, "/login", nil)
+			},
+			wantStatusCode: http.StatusTemporaryRedirect,
+		},
+		{
+			name: "invalid cookie",
+			request: func(a *aad) (*http.Request, error) {
+				return &http.Request{
+					Header: http.Header{
+						"Cookie": []string{"session=xxx"},
+					},
+				}, nil
+			},
+			wantStatusCode: http.StatusInternalServerError,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+			env := mock_env.NewMockInterface(controller)
+			env.EXPECT().IsLocalDevelopmentMode().AnyTimes().Return(false)
+			env.EXPECT().TenantID().AnyTimes().Return("")
 
-				var m map[interface{}]interface{}
-				cookies := w.Result().Cookies()
-				err = securecookie.DecodeMulti(SessionName, cookies[len(cookies)-1].Value, &m, a.(*aad).store.Codecs...)
-				if err != nil {
-					t.Fatal(err)
-				}
+			_, audit := testlog.NewAudit()
+			_, baseLog := testlog.New()
+			_, baseAccessLog := testlog.New()
+			a, err := NewAAD(baseLog, audit, env, baseAccessLog, "", make([]byte, 32), "", nil, nil, nil, mux.NewRouter(), nil)
+			if err != nil {
+				t.Fatal(err)
+			}
 
-				if len(m) != 2 {
-					t.Error(len(m))
-				}
+			h := http.HandlerFunc(a.Login)
 
-				if redirectPath, ok := m[sessionKeyRedirectPath].(string); !ok ||
-					redirectPath != "/" {
-					t.Error(m[sessionKeyRedirectPath])
-				}
+			r, err := tt.request(a.(*aad))
+			if err != nil {
+				t.Fatal(err)
+			}
 
-				if state, ok := m[sessionKeyState].(string); !ok ||
-					uuid.FromStringOrNil(state) == uuid.Nil {
-					t.Error(m[sessionKeyState])
-				}
+			w := httptest.NewRecorder()
+
+			h.ServeHTTP(w, r)
+
+			if w.Code != tt.wantStatusCode {
+				t.Error(w.Code, tt.wantStatusCode)
 			}
+
+			if tt.wantStatusCode == http.StatusInternalServerError {
+				return
+			}
+
+			if !strings.HasPrefix(w.Header().Get("Location"), "https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=&redirect_uri=https%3A%2F%2F%2Fcallback&response_type=code&scope=openid+profile&state=") {
+				t.Error(w.Header().Get("Location"))
+			}
+
 		})
 	}
 }
@@ -439,8 +490,7 @@ func TestCallback(t *testing.T) {
 				uuid := uuid.Must(uuid.NewV4()).String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -491,8 +541,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -517,8 +566,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -545,8 +593,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -574,8 +621,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -601,8 +647,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -633,8 +678,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -665,8 +709,7 @@ func TestCallback(t *testing.T) {
 				uuid := u.String()
 
 				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState:        uuid,
-					sessionKeyRedirectPath: "/",
+					sessionKeyState: uuid,
 				}, a.store.Codecs...)
 				if err != nil {
 					return nil, err
@@ -690,37 +733,6 @@ func TestCallback(t *testing.T) {
 			verifier:      &oidc.NoopVerifier{},
 			wantForbidden: true,
 		},
-		{
-			name: "fail - missing redirect_path",
-			request: func(a *aad) (*http.Request, error) {
-				u, _ := uuid.NewV4()
-				uuid := u.String()
-
-				cookie, err := securecookie.EncodeMulti(SessionName, map[interface{}]interface{}{
-					sessionKeyState: uuid,
-				}, a.store.Codecs...)
-				if err != nil {
-					return nil, err
-				}
-
-				return &http.Request{
-					URL: &url.URL{},
-					Header: http.Header{
-						"Cookie": []string{SessionName + "=" + cookie},
-					},
-					Form: url.Values{
-						"state": []string{uuid},
-					},
-				}, nil
-			},
-			oauther: &noopOauther{
-				tokenMap: map[string]interface{}{
-					"id_token": string(idToken),
-				},
-			},
-			verifier:  &oidc.NoopVerifier{},
-			wantError: "Internal Server Error\n",
-		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			controller := gomock.NewController(t)
@@ -760,7 +772,8 @@ func TestCallback(t *testing.T) {
 				return
 			}
 
-			var m map[interface{}]interface{}
+			type cookie map[interface{}]interface{}
+			var m cookie
 			cookies := w.Result().Cookies()
 			err = securecookie.DecodeMulti(SessionName, cookies[len(cookies)-1].Value, &m, a.(*aad).store.Codecs...)
 			if err != nil {
@@ -777,23 +790,12 @@ func TestCallback(t *testing.T) {
 					t.Error(w.Header().Get("Location"))
 				}
 
-				if len(m) != 3 {
-					t.Error(len(m))
-				}
-
-				if expires, ok := m[SessionKeyExpires].(time.Time); !ok ||
-					expires != time.Unix(3600, 0) {
-					t.Error(m[SessionKeyExpires])
-				}
-
-				if sessionGroups, ok := m[SessionKeyGroups].([]string); !ok ||
-					!reflect.DeepEqual(sessionGroups, groups) {
-					t.Error(m[SessionKeyGroups])
-				}
-
-				if sessionUsername, ok := m[SessionKeyUsername].(string); !ok ||
-					sessionUsername != username {
-					t.Error(m[SessionKeyUsername])
+				for _, l := range deep.Equal(m, cookie{
+					SessionKeyExpires:  time.Unix(3600, 0),
+					SessionKeyGroups:   groups,
+					SessionKeyUsername: username,
+				}) {
+					t.Error(l)
 				}
 
 			case tt.wantForbidden:
@@ -805,15 +807,9 @@ func TestCallback(t *testing.T) {
 					t.Error(w.Header().Get("Location"))
 				}
 
-				if len(m) != 1 {
-					t.Error(len(m))
-				}
-
-				if redirectPath, ok := m[sessionKeyRedirectPath].(string); !ok ||
-					redirectPath != "/" {
-					t.Error(m[sessionKeyRedirectPath])
+				for _, l := range deep.Equal(m, cookie{}) {
+					t.Error(l)
 				}
-
 			default:
 				if w.Code != http.StatusTemporaryRedirect {
 					t.Error(w.Code)
@@ -822,20 +818,7 @@ func TestCallback(t *testing.T) {
 				if w.Header().Get("Location") != "/authcodeurl" {
 					t.Error(w.Header().Get("Location"))
 				}
-
-				if len(m) != 2 {
-					t.Error(len(m))
-				}
-
-				if redirectPath, ok := m[sessionKeyRedirectPath].(string); !ok ||
-					redirectPath != "" {
-					t.Error(m[sessionKeyRedirectPath])
-				}
-
-				if state, ok := m[sessionKeyState].(string); !ok ||
-					uuid.FromStringOrNil(state) == uuid.Nil {
-					t.Error(m[sessionKeyState])
-				}
+				return
 			}
 		})
 	}
diff --git a/pkg/portal/security_test.go b/pkg/portal/security_test.go
index b50a9d747..f93fffc1b 100644
--- a/pkg/portal/security_test.go
+++ b/pkg/portal/security_test.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/golang/mock/gomock"
+	"github.com/golangci/golangci-lint/pkg/sliceutil"
 	"github.com/gorilla/securecookie"
 	"github.com/gorilla/sessions"
 	"github.com/sirupsen/logrus"
@@ -109,7 +110,9 @@ func TestSecurity(t *testing.T) {
 			request: func() (*http.Request, error) {
 				return http.NewRequest(http.MethodGet, "https://server/", nil)
 			},
-			wantAuditOperation: "GET /",
+			unauthenticatedWantStatusCode: 307,
+			authenticatedWantStatusCode:   200,
+			wantAuditOperation:            "GET /",
 			wantAuditTargetResources: []audit.TargetResource{
 				{
 					TargetResourceType: "",
@@ -148,8 +151,9 @@ func TestSecurity(t *testing.T) {
 			request: func() (*http.Request, error) {
 				return http.NewRequest(http.MethodPost, "https://server/api/logout", nil)
 			},
-			authenticatedWantStatusCode: http.StatusSeeOther,
-			wantAuditOperation:          "POST /api/logout",
+			unauthenticatedWantStatusCode: http.StatusSeeOther,
+			authenticatedWantStatusCode:   http.StatusSeeOther,
+			wantAuditOperation:            "POST /api/logout",
 			wantAuditTargetResources: []audit.TargetResource{
 				{
 					TargetResourceType: "",
@@ -162,8 +166,9 @@ func TestSecurity(t *testing.T) {
 			request: func() (*http.Request, error) {
 				return http.NewRequest(http.MethodGet, "https://server/callback", nil)
 			},
-			authenticatedWantStatusCode: http.StatusTemporaryRedirect,
-			wantAuditOperation:          "GET /callback",
+			unauthenticatedWantStatusCode: http.StatusTemporaryRedirect,
+			authenticatedWantStatusCode:   http.StatusTemporaryRedirect,
+			wantAuditOperation:            "GET /callback",
 			wantAuditTargetResources: []audit.TargetResource{
 				{
 					TargetResourceType: "",
@@ -310,12 +315,18 @@ func TestSecurity(t *testing.T) {
 					if tt2.authenticated {
 						tt2.wantStatusCode = http.StatusOK
 					} else {
-						tt2.wantStatusCode = http.StatusTemporaryRedirect
+						tt2.wantStatusCode = http.StatusForbidden
 					}
 				}
 
 				if resp.StatusCode != tt2.wantStatusCode {
-					t.Error(resp.StatusCode)
+					t.Error(resp.StatusCode, tt2.wantStatusCode)
+					body := make([]byte, 0)
+					_, err := resp.Body.Read(body)
+					if err != nil {
+						t.Fatal(err)
+					}
+					t.Error(body)
 				}
 
 				if tt.checkResponse != nil {
@@ -347,15 +358,24 @@ func TestSecurity(t *testing.T) {
 					}
 				}
 
-				payload := auditPayloadFixture()
-				payload.OperationName = tt.wantAuditOperation
-				payload.TargetResources = tt.wantAuditTargetResources
-				payload.Result.ResultDescription = fmt.Sprintf("Status code: %d", tt2.wantStatusCode)
-				if tt2.authenticated && tt.name != "/callback" && tt.name != "/healthz/ready" {
-					payload.CallerIdentities[0].CallerIdentityValue = "username"
-				}
+				if tt.wantAuditOperation != "" {
+					payload := auditPayloadFixture()
+					payload.OperationName = tt.wantAuditOperation
+					payload.TargetResources = tt.wantAuditTargetResources
+					payload.Result.ResultDescription = fmt.Sprintf("Status code: %d", tt2.wantStatusCode)
 
-				testlog.AssertAuditPayloads(t, auditHook, []*audit.Payload{payload})
+					if tt2.wantStatusCode == http.StatusForbidden {
+						payload.Result.ResultType = audit.ResultTypeFail
+					}
+
+					if tt2.authenticated && !sliceutil.Contains([]string{
+						"/callback", "/healthz/ready", "/api/login", "/api/logout"}, tt.name) {
+						payload.CallerIdentities[0].CallerIdentityValue = "username"
+					}
+					testlog.AssertAuditPayloads(t, auditHook, []*audit.Payload{payload})
+				} else {
+					testlog.AssertAuditPayloads(t, auditHook, []*audit.Payload{})
+				}
 			})
 		}
 	}