auth: fix OIDC logging out

cmd/bridge/config/auth/authoptions.go

@@ -198,7 +198,6 @@ func (c *completedOptions) ApplyTo(
 	sessionConfig *session.CompletedOptions,
 ) error {
 	srv.InactivityTimeout = c.InactivityTimeoutSeconds
-	srv.LogoutRedirect = c.LogoutRedirectURL
 
 	var err error
 	srv.Authenticator, err = c.getAuthenticator(
@@ -234,7 +233,7 @@ func (c *completedOptions) getAuthenticator(
 		authLoginSuccessEndpoint = proxy.SingleJoiningSlash(baseURL.String(), server.AuthLoginSuccessEndpoint)
 		oidcClientSecret         = c.ClientSecret
 		// Abstraction leak required by NewAuthenticator. We only want the browser to send the auth token for paths starting with basePath/api.
-		cookiePath       = proxy.SingleJoiningSlash(baseURL.Path, "/api/")
+		cookiePath       = proxy.SingleJoiningSlash(baseURL.Path, "/")
 		refererPath      = baseURL.String()
 		useSecureCookies = baseURL.Scheme == "https"
 	)
@@ -281,6 +280,10 @@ func (c *completedOptions) getAuthenticator(
 		K8sConfig: k8sClientConfig,
 	}
 
+	if c.LogoutRedirectURL != nil {
+		oidcClientConfig.LogoutRedirectOverride = c.LogoutRedirectURL.String()
+	}
+
 	authenticator, err := auth.NewAuthenticator(context.Background(), oidcClientConfig)
 	if err != nil {
 		klog.Fatalf("Error initializing authenticator: %v", err)

cmd/bridge/main.go

@@ -24,7 +24,7 @@ import (
 	oscrypto "github.com/openshift/library-go/pkg/crypto"
 
 	"k8s.io/client-go/rest"
-	"k8s.io/klog"
+	klog "k8s.io/klog/v2"
 )
 
 const (

frontend/@types/console/index.d.ts

@@ -25,7 +25,6 @@ declare interface Window {
     customProductName: string;
     documentationBaseURL: string;
     kubeAPIServerURL: string;
-    kubeAdminLogoutURL: string;
     loadTestFactor: number;
     loginErrorURL: string;
     loginSuccessURL: string;

frontend/packages/console-telemetry-plugin/src/listeners/segment.ts

@@ -122,7 +122,7 @@ export const eventListener: TelemetryEventListener = async (
     case 'identify':
       {
         const { user, ...otherProperties } = properties;
-        const id = user?.metadata?.name;
+        const id = user?.metadata?.name; // FIXME: also wrong?
         if (id) {
           // Use SHA1 hash algorithm to anonymize the user
           const anonymousIdBuffer = await crypto.subtle.digest(

frontend/packages/gitops-plugin/src/components/utils/useDefaultSecret.tsx

@@ -7,7 +7,7 @@ import { userStateToProps } from '@console/internal/reducers/ui';
 
 const useDefaultSecret = () => {
   const { user } = useSelector(userStateToProps);
-  const userName = _.replace(user?.metadata?.name ?? '', /[^a-zA-Z0-9-]/g, '');
+  const userName = _.replace(user?.metadata?.name ?? '', /[^a-zA-Z0-9-]/g, ''); // FIXME: also wrong?
   const defaultSecret = userName
     ? [`pipelines-${userName}-github`, `${userName}-github-token`]
     : [];

frontend/packages/pipelines-plugin/src/components/pipelineruns/triggered-by/hooks.ts

@@ -18,7 +18,7 @@ const mergeAnnotationsWithResource = (annotations: AnnotationMap, resource: K8sR
 export const useUserAnnotationForManualStart = (): AnnotationMap => {
   const user = useSelector(getUser);
 
-  if (!user?.metadata?.name) {
+  if (!user?.metadata?.name) { // FIXME: this is wrong
     return {};
   }
 

frontend/public/components/masthead-toolbar.jsx

@@ -178,11 +178,7 @@ class MastheadToolbarContents_ extends React.Component {
     const { flags, user } = this.props;
     clearTimeout(this.userInactivityTimeout);
     this.userInactivityTimeout = setTimeout(() => {
-      if (flags[FLAGS.OPENSHIFT]) {
-        authSvc.logoutOpenShift(user?.metadata?.name === 'kube:admin');
-      } else {
-        authSvc.logout();
-      }
+        authSvc.logout('', user?.username === 'kube:admin');
     }, window.SERVER_FLAGS.inactivityTimeout * 1000);
   }
 
@@ -554,11 +550,7 @@ class MastheadToolbarContents_ extends React.Component {
     if (flags[FLAGS.AUTH_ENABLED]) {
       const logout = (e) => {
         e.preventDefault();
-        if (flags[FLAGS.OPENSHIFT]) {
-          authSvc.logoutOpenShift(this.state.isKubeAdmin);
-        } else {
-          authSvc.logout();
-        }
+        authSvc.logout('', this.state.isKubeAdmin);
       };
 
       if (requestTokenURL) {

frontend/public/module/auth.js

@@ -71,7 +71,6 @@ export const authSvc = {
       window.location.assign(redirect);
       return;
     }
-
     // If we're on the login error page, this means there was a problem with the last
     // authentication attempt. Show the error from the previous attempt instead of redirecting
     // login again. This is necessary because login may not be available (e.g. if the OAuth
@@ -82,31 +81,22 @@ export const authSvc = {
     }
   },
 
-  // Avoid logging out multiple times if concurrent requests return unauthorized.
-  logout: _.once((next) => {
+  logout: _.once((next, isKubeAdmin = false) => {
     setNext(next);
+    // FIXME: do isKubeAdminCheck here and send it to logoutRedirect that should decide
+    // where to redirect next
     clearLocalStorage(clearLocalStorageKeys);
     coFetch(logoutURL, { method: 'POST' })
       // eslint-disable-next-line no-console
       .catch((e) => console.error('Error logging out', e))
-      .then(() => authSvc.logoutRedirect(next));
-  }),
-
-  // Extra steps are needed if this is OpenShift to delete the user's access
-  // token and logout the kube:admin user.
-  logoutOpenShift: (isKubeAdmin = false) => {
-    clearLocalStorage(clearLocalStorageKeys);
-    coFetch('/api/openshift/delete-token', { method: 'POST' })
-      // eslint-disable-next-line no-console
-      .catch((e) => console.error('Error deleting token', e))
       .then(() => {
         if (isKubeAdmin) {
           authSvc.logoutKubeAdmin();
         } else {
-          authSvc.logoutRedirect();
+          authSvc.logoutRedirect(next);
         }
       });
-  },
+  }),
 
   // The kube:admin user has a special logout flow. The OAuth server has a
   // session cookie that must be cleared by POSTing to the kube:admin logout
@@ -129,7 +119,7 @@ export const authSvc = {
 
     document.body.appendChild(form);
     form.submit();
-  },
+},
 
   login: () => {
     // Ensure that we don't redirect to the current URL in a loop

pkg/auth/auth.go

@@ -88,6 +88,8 @@ type loginMethod interface {
 	DeleteCookie(http.ResponseWriter, *http.Request)
 	// logout deletes any cookies associated with the user, and writes a no-content response.
 	logout(http.ResponseWriter, *http.Request)
+	// LogoutRedirectURL returns the URL to redirect to after a logout.
+	LogoutRedirectURL() string
 
 	// Authenticate checks if there's an authenticated session connected to the
 	// request based on a cookie, and returns a user associated to the cookie
@@ -109,12 +111,13 @@ const (
 type Config struct {
 	AuthSource AuthSource
 
-	IssuerURL    string
-	IssuerCA     string
-	RedirectURL  string
-	ClientID     string
-	ClientSecret string
-	Scope        []string
+	IssuerURL              string
+	LogoutRedirectOverride string // overrides the OIDC provider's front-channel logout URL
+	IssuerCA               string
+	RedirectURL            string
+	ClientID               string
+	ClientSecret           string
+	Scope                  []string
 
 	// K8sCA is required for OpenShift OAuth metadata discovery. This is the CA
 	// used to talk to the master, which might be different than the issuer CA.
@@ -200,12 +203,13 @@ func NewAuthenticator(ctx context.Context, config *Config) (*Authenticator, erro
 	a := newUnstartedAuthenticator(c)
 
 	authConfig := &oidcConfig{
-		getClient:             a.clientFunc,
-		issuerURL:             c.IssuerURL,
-		clientID:              c.ClientID,
-		cookiePath:            c.CookiePath,
-		secureCookies:         c.SecureCookies,
-		constructOAuth2Config: a.oauth2ConfigConstructor,
+		getClient:              a.clientFunc,
+		issuerURL:              c.IssuerURL,
+		logoutRedirectOverride: c.LogoutRedirectOverride,
+		clientID:               c.ClientID,
+		cookiePath:             c.CookiePath,
+		secureCookies:          c.SecureCookies,
+		constructOAuth2Config:  a.oauth2ConfigConstructor,
 	}
 
 	var tokenHandler loginMethod

pkg/auth/auth_oidc.go

@@ -29,12 +29,13 @@ type oidcAuth struct {
 }
 
 type oidcConfig struct {
-	getClient             func() *http.Client
-	issuerURL             string
-	clientID              string
-	cookiePath            string
-	secureCookies         bool
-	constructOAuth2Config oauth2ConfigConstructor
+	getClient              func() *http.Client
+	issuerURL              string
+	logoutRedirectOverride string
+	clientID               string
+	cookiePath             string
+	secureCookies          bool
+	constructOAuth2Config  oauth2ConfigConstructor
 }
 
 func newOIDCAuth(ctx context.Context, sessionStore *sessions.CombinedSessionStore, c *oidcConfig) (*oidcAuth, error) {
@@ -150,6 +151,22 @@ func (o *oidcAuth) GetSpecialURLs() SpecialAuthURLs {
 	return SpecialAuthURLs{}
 }
 
+func (o *oidcAuth) LogoutRedirectURL() string {
+	if len(o.logoutRedirectOverride) > 0 {
+		return o.logoutRedirectOverride
+	}
+
+	sessionEndpoints := struct {
+		// Get the RP-initiated logout endpoint (https://openid.net/specs/openid-connect-rpinitiated-1_0.html)
+		EndSessionEndpoint string `json:"end_session_endpoint"`
+	}{}
+
+	provider := o.providerCache.GetItem()
+	provider.Claims(&sessionEndpoints)
+
+	return sessionEndpoints.EndSessionEndpoint
+}
+
 func (o *oidcAuth) oauth2Config() *oauth2.Config {
 	return o.oidcConfig.constructOAuth2Config(o.providerCache.GetItem().Endpoint())
 }

pkg/auth/auth_openshift.go

@@ -169,10 +169,15 @@ func (o *openShiftAuth) DeleteCookie(w http.ResponseWriter, r *http.Request) {
 }
 
 func (o *openShiftAuth) logout(w http.ResponseWriter, r *http.Request) {
+	// FIXME: delete access token
 	o.DeleteCookie(w, r)
 	w.WriteHeader(http.StatusNoContent)
 }
 
+func (o *openShiftAuth) LogoutRedirectURL() string {
+	return o.logoutRedirectOverride
+}
+
 func (o *openShiftAuth) Authenticate(_ http.ResponseWriter, r *http.Request) (*User, error) {
 	// TODO: This doesn't do any validation of the cookie with the assumption that the
 	// API server will reject tokens it doesn't recognize. If we want to keep some backend

pkg/auth/sessions/combined_sessions.go

@@ -160,21 +160,22 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req
 	defer cs.sessionLock.Unlock()
 
 	for _, cookie := range r.Cookies() {
+		cookie := cookie
 		if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) {
 			cookie.MaxAge = -1
 			http.SetCookie(w, cookie)
 		}
 	}
 
 	cookieSession := cs.getCookieSession(r)
-	if sessionToken, ok := cookieSession.sessionToken.Values["session-token"]; ok {
-		cs.serverStore.DeleteBySessionToken(sessionToken.(string))
-	}
-
 	if refreshToken, ok := cookieSession.refreshToken.Values["refresh-token"]; ok {
 		cs.serverStore.DeleteByRefreshToken(refreshToken.(string))
 	}
 
+	if sessionToken, ok := cookieSession.sessionToken.Values["session-token"]; ok {
+		cs.serverStore.DeleteBySessionToken(sessionToken.(string))
+	}
+
 	refreshTokenCookie, _ := cs.clientStore.Get(r, openshiftRefreshTokenCookieName)
 	if !refreshTokenCookie.IsNew {
 		// Get always returns a session, only timeout current sessions

pkg/auth/sessions/combined_sessions_test.go

@@ -575,7 +575,9 @@ func TestCombinedSessionStore_DeleteSession(t *testing.T) {
 
 			gotCookies := map[string]*http.Cookie{}
 			for _, c := range testWriter.Result().Cookies() {
-				gotCookies[c.Name] = c
+				if c.MaxAge == -1 {
+					gotCookies[c.Name] = c
+				}
 			}
 
 			for _, cookieName := range tt.wantCookieTimeouts {

pkg/server/server.go

@@ -64,7 +64,6 @@ const (
 	authLoginEndpoint                     = "/auth/login"
 	authLogoutEndpoint                    = "/auth/logout"
 	customLogoEndpoint                    = "/custom-logo"
-	deleteOpenshiftTokenEndpoint          = "/api/openshift/delete-token"
 	devfileEndpoint                       = "/api/devfile/"
 	devfileSamplesEndpoint                = "/api/devfile/samples/"
 	gitopsEndpoint                        = "/api/gitops/"
@@ -169,7 +168,6 @@ type Server struct {
 	KubeAPIServerURL                    string // JS global only. Not used for proxying.
 	KubeVersion                         string
 	LoadTestFactor                      int
-	LogoutRedirect                      *url.URL
 	MonitoringDashboardConfigMapLister  ResourceLister
 	NodeArchitectures                   []string
 	NodeOperatingSystems                []string
@@ -310,7 +308,6 @@ func (s *Server) HTTPHandler() (http.Handler, error) {
 		handleFunc(AuthLoginCallbackEndpoint, s.Authenticator.CallbackFunc(fn))
 		handle(requestTokenEndpoint, authHandler(s.handleClusterTokenURL))
 		// TODO: only add the following in case the auth type is openshift?
-		handleFunc(deleteOpenshiftTokenEndpoint, allowMethod(http.MethodPost, authHandlerWithUser(s.handleOpenShiftTokenDeletion)))
 	}
 
 	handleFunc("/api/", notFoundHandler)
@@ -697,7 +694,7 @@ func (s *Server) indexHandler(w http.ResponseWriter, r *http.Request) {
 		LoginURL:                  proxy.SingleJoiningSlash(s.BaseURL.String(), authLoginEndpoint),
 		LoginSuccessURL:           proxy.SingleJoiningSlash(s.BaseURL.String(), AuthLoginSuccessEndpoint),
 		LoginErrorURL:             proxy.SingleJoiningSlash(s.BaseURL.String(), AuthLoginErrorEndpoint),
-		LogoutURL:                 proxy.SingleJoiningSlash(s.BaseURL.String(), authLogoutEndpoint),
+		LogoutURL:                 authLogoutEndpoint,
 		KubeAPIServerURL:          s.KubeAPIServerURL,
 		Branding:                  s.Branding,
 		CustomProductName:         s.CustomProductName,
@@ -730,11 +727,9 @@ func (s *Server) indexHandler(w http.ResponseWriter, r *http.Request) {
 		K8sMode:                   s.K8sMode,
 	}
 
-	if s.LogoutRedirect != nil {
-		jsg.LogoutRedirect = s.LogoutRedirect.String()
-	}
-
 	if !s.authDisabled() {
+		jsg.LogoutRedirect = s.Authenticator.LogoutRedirectURL()
+
 		specialAuthURLs := s.Authenticator.GetSpecialURLs()
 		jsg.KubeAdminLogoutURL = specialAuthURLs.KubeAdminLogout
 	}