From 0f3080ecb89b4e12534e16dd56122d600addd80e Mon Sep 17 00:00:00 2001
From: Karl Persson <kalle.persson@grafana.com>
Date: Sun, 12 May 2024 19:53:19 +0200
Subject: [PATCH] AuthN: Fix signout redirect url (#87631)

* Add missing return

* Use sign out redirect url from auth config if configured

* remove option from auth.jwt that is not used
---
 conf/defaults.ini                            |  1 -
 pkg/api/login.go                             |  1 +
 pkg/services/authn/authnimpl/service.go      | 13 +++++++------
 pkg/services/authn/authnimpl/service_test.go | 15 ++++++++++++++-
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/conf/defaults.ini b/conf/defaults.ini
index d51b97c4482..a3f53f4357a 100644
--- a/conf/defaults.ini
+++ b/conf/defaults.ini
@@ -875,7 +875,6 @@ auto_sign_up = false
 url_login = false
 allow_assign_grafana_admin = false
 skip_org_role_sync = false
-signout_redirect_url =
 
 #################################### Auth LDAP ###########################
 [auth.ldap]
diff --git a/pkg/api/login.go b/pkg/api/login.go
index dcea398d4a9..3dc203069c2 100644
--- a/pkg/api/login.go
+++ b/pkg/api/login.go
@@ -254,6 +254,7 @@ func (hs *HTTPServer) Logout(c *contextmodel.ReqContext) {
 	if err != nil {
 		hs.log.Error("Failed perform proper logout", "error", err)
 		c.Redirect(hs.Cfg.AppSubURL + "/login")
+		return
 	}
 
 	_, id := c.SignedInUser.GetNamespacedID()
diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go
index 38bbcca0f2f..006d8646766 100644
--- a/pkg/services/authn/authnimpl/service.go
+++ b/pkg/services/authn/authnimpl/service.go
@@ -17,7 +17,6 @@ import (
 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/infra/usagestats"
 	"github.com/grafana/grafana/pkg/services/auth"
-	"github.com/grafana/grafana/pkg/services/auth/identity"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/authn/clients"
 	"github.com/grafana/grafana/pkg/services/user"
@@ -265,15 +264,17 @@ func (s *Service) Logout(ctx context.Context, user authn.Requester, sessionToken
 	defer span.End()
 
 	redirect := &authn.Redirect{URL: s.cfg.AppSubURL + "/login"}
+	if s.cfg.SignoutRedirectUrl != "" {
+		redirect.URL = s.cfg.SignoutRedirectUrl
+	}
 
-	namespace, id := user.GetNamespacedID()
-	if namespace != authn.NamespaceUser {
+	if !user.GetID().IsNamespace(authn.NamespaceUser) {
 		return redirect, nil
 	}
 
-	userID, err := identity.IntIdentifier(namespace, id)
+	id, err := user.GetID().ParseInt()
 	if err != nil {
-		s.log.FromContext(ctx).Debug("Invalid user id", "id", userID, "err", err)
+		s.log.FromContext(ctx).Debug("Invalid user id", "id", id, "err", err)
 		return redirect, nil
 	}
 
@@ -301,7 +302,7 @@ func (s *Service) Logout(ctx context.Context, user authn.Requester, sessionToken
 	}
 
 Default:
-	if err = s.sessionService.RevokeToken(ctx, sessionToken, false); err != nil {
+	if err = s.sessionService.RevokeToken(ctx, sessionToken, false); err != nil && !errors.Is(err, auth.ErrUserTokenNotFound) {
 		return nil, err
 	}
 
diff --git a/pkg/services/authn/authnimpl/service_test.go b/pkg/services/authn/authnimpl/service_test.go
index 2b673c8c85d..51970ebb4b4 100644
--- a/pkg/services/authn/authnimpl/service_test.go
+++ b/pkg/services/authn/authnimpl/service_test.go
@@ -409,7 +409,8 @@ func TestService_Logout(t *testing.T) {
 		identity     *authn.Identity
 		sessionToken *usertoken.UserToken
 
-		client authn.Client
+		client             authn.Client
+		signoutRedirectURL string
 
 		expectedErr          error
 		expectedTokenRevoked bool
@@ -441,6 +442,14 @@ func TestService_Logout(t *testing.T) {
 			client:               &authntest.FakeClient{ExpectedName: "auth.client.azuread"},
 			expectedTokenRevoked: true,
 		},
+		{
+			desc:                 "should use signout redirect url if configured",
+			identity:             &authn.Identity{ID: authn.NewNamespaceID(authn.NamespaceUser, 1), AuthenticatedBy: "azuread"},
+			expectedRedirect:     &authn.Redirect{URL: "some-url"},
+			client:               &authntest.FakeClient{ExpectedName: "auth.client.azuread"},
+			signoutRedirectURL:   "some-url",
+			expectedTokenRevoked: true,
+		},
 		{
 			desc:             "should redirect to client specific url",
 			identity:         &authn.Identity{ID: authn.NewNamespaceID(authn.NamespaceUser, 1), AuthenticatedBy: "azuread"},
@@ -473,6 +482,10 @@ func TestService_Logout(t *testing.T) {
 						return nil
 					},
 				}
+
+				if tt.signoutRedirectURL != "" {
+					svc.cfg.SignoutRedirectUrl = tt.signoutRedirectURL
+				}
 			})
 
 			redirect, err := s.Logout(context.Background(), tt.identity, tt.sessionToken)