EDM-5631: terminate AAP Gateway session on flightctl-ui logout

proxy/auth/aap.go

@@ -111,7 +111,7 @@ func getAAPClient(authorizationUrl, tokenUrl string, tlsConfig *tls.Config, clie
 	return client, nil
 }
 
-func (a *AAPAuthHandler) Logout(token string, _ string) (string, error) {
+func (a *AAPAuthHandler) Logout(token string, postLogoutBase string) (string, error) {
 	data := url.Values{}
 	data.Set("client_id", a.clientId)
 	data.Set("token", token)
@@ -130,11 +130,55 @@ func (a *AAPAuthHandler) Logout(token string, _ string) (string, error) {
 
 	res, err := httpClient.Do(req)
 	if err != nil {
+		// Token revocation failed; do not issue a browser redirect. Preserve the
+		// original behavior of surfacing the error to the caller.
 		log.GetLogger().WithError(err).Warn("Failed to logout")
 		return "", err
 	}
-	defer res.Body.Close()
-	return "", nil
+	defer func() {
+		if cerr := res.Body.Close(); cerr != nil {
+			log.GetLogger().WithError(cerr).Debug("failed to close revocation response body")
+		}
+	}()
+
+	// A successful revocation returns a 2xx status. On any non-2xx response the token may
+	// still be valid, so surface an error and do not issue the browser redirect.
+	if res.StatusCode < 200 || res.StatusCode >= 300 {
+		revokeErr := fmt.Errorf("AAP token revocation failed with status %d", res.StatusCode)
+		log.GetLogger().WithError(revokeErr).Warn("Failed to logout")
+		return "", revokeErr
+	}
+
+	// The server-to-server token revocation above invalidates the OAuth token, but AAP
+	// also keeps a browser session cookie (awx_sessionid) that revocation alone does not
+	// clear. Return a redirect to AAP's session-termination endpoint so the browser's
+	// visit clears that cookie; the "next" parameter then sends the user back to RHEM's
+	// login page.
+	//
+	// AAP is built on Django/AWX. When AWX runs behind the AAP Gateway,
+	// LoggedLogoutView.dispatch() in awx/api/generics.py redirects logout requests to the
+	// Gateway's session-termination endpoint "/api/gateway/v1/logout/" (carrying a "next"
+	// query parameter), so we send the browser directly there to terminate the Gateway
+	// session.
+	//
+	// Security: both the redirect target and the "next" value are derived exclusively
+	// from configured values -- a.internalAuthURL (provider config) and postLogoutBase
+	// (resolved by the caller from an allow-listed origin or BASE_UI_URL). No
+	// user-supplied URL fragment is incorporated, which prevents open-redirect
+	// vulnerabilities.
+	logoutURL, err := url.Parse(a.internalAuthURL)
+	if err != nil {
+		log.GetLogger().WithError(err).Warn("failed to parse AAP internal auth URL for logout redirect")
+		return "", err
+	}
+	// JoinPath normalizes any duplicate or trailing slashes in the configured base URL and
+	// preserves the trailing slash the Gateway expects on the logout path.
+	logoutURL = logoutURL.JoinPath("api", "gateway", "v1", "logout/")
+	q := logoutURL.Query()
+	q.Set("next", postLogoutBase)
+	logoutURL.RawQuery = q.Encode()
+
+	return logoutURL.String(), nil
 }
 
 func (a *AAPAuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) (string, error) {

proxy/auth/aap_test.go

@@ -0,0 +1,219 @@
+package auth
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"testing"
+
+	"github.com/flightctl/flightctl-ui/log"
+)
+
+// TestMain initializes the shared logger the same way the proxy does at startup
+// (see app.go), so that error-path logging inside the handlers under test does
+// not dereference a nil logger.
+func TestMain(m *testing.M) {
+	log.InitLogs()
+	os.Exit(m.Run())
+}
+
+// revokeRecorder captures the token-revocation request the AAP handler sends so
+// tests can assert that the existing server-to-server revocation behavior is
+// preserved alongside the new redirect behavior.
+type revokeRecorder struct {
+	called bool
+	method string
+	path   string
+	form   url.Values
+}
+
+// newRevokeTestServer returns an httptest server that emulates AAP's OAuth token
+// revocation endpoint and records the request it receives. The server is closed
+// automatically when the test finishes.
+func newRevokeTestServer(t *testing.T, status int) (*httptest.Server, *revokeRecorder) {
+	t.Helper()
+	rec := &revokeRecorder{}
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		rec.called = true
+		rec.method = r.Method
+		rec.path = r.URL.Path
+		if err := r.ParseForm(); err != nil {
+			http.Error(w, "failed to parse form: "+err.Error(), http.StatusBadRequest)
+			return
+		}
+		rec.form = r.PostForm
+		w.WriteHeader(status)
+	}))
+	t.Cleanup(srv.Close)
+	return srv, rec
+}
+
+// TestAAPLogoutBuildsSessionTerminationURL verifies that after the OAuth token is
+// revoked, Logout returns a redirect to AAP's session-termination endpoint
+// ({internalAuthURL}/api/gateway/v1/logout/) carrying next={postLogoutBase}.
+func TestAAPLogoutBuildsSessionTerminationURL(t *testing.T) {
+	t.Parallel()
+
+	srv, rec := newRevokeTestServer(t, http.StatusOK)
+
+	handler := &AAPAuthHandler{
+		internalAuthURL: srv.URL,
+		clientId:        "test-client",
+	}
+
+	const postLogoutBase = "https://ui.example.com/ui"
+	logoutURL, err := handler.Logout("the-token", postLogoutBase)
+	if err != nil {
+		t.Fatalf("expected no error, got: %v", err)
+	}
+
+	expected := srv.URL + "/api/gateway/v1/logout/?next=" + url.QueryEscape(postLogoutBase)
+	if logoutURL != expected {
+		t.Fatalf("unexpected logout URL.\n  expected %q\n  got      %q", expected, logoutURL)
+	}
+
+	// Existing behavior must be preserved: the token is revoked server-to-server
+	// against {internalAuthURL}/o/revoke_token/ before the redirect is returned.
+	if !rec.called {
+		t.Fatal("expected token revocation request to be sent")
+	}
+	if rec.method != http.MethodPost {
+		t.Fatalf("expected revoke request method POST, got %q", rec.method)
+	}
+	if rec.path != "/o/revoke_token/" {
+		t.Fatalf("expected revoke path /o/revoke_token/, got %q", rec.path)
+	}
+	if got := rec.form.Get("token"); got != "the-token" {
+		t.Fatalf("expected revoke form token %q, got %q", "the-token", got)
+	}
+	if got := rec.form.Get("client_id"); got != "test-client" {
+		t.Fatalf("expected revoke form client_id %q, got %q", "test-client", got)
+	}
+}
+
+// TestAAPLogoutHandlesTrailingSlashInBaseURL verifies robust URL construction:
+// a configured internalAuthURL with a trailing slash must not produce a double
+// slash, and the trailing slash on /api/gateway/v1/logout/ must be preserved.
+func TestAAPLogoutHandlesTrailingSlashInBaseURL(t *testing.T) {
+	t.Parallel()
+
+	srv, _ := newRevokeTestServer(t, http.StatusOK)
+
+	handler := &AAPAuthHandler{
+		internalAuthURL: srv.URL + "/", // configured value with a trailing slash
+		clientId:        "test-client",
+	}
+
+	const postLogoutBase = "https://ui.example.com/ui"
+	logoutURL, err := handler.Logout("the-token", postLogoutBase)
+	if err != nil {
+		t.Fatalf("expected no error, got: %v", err)
+	}
+
+	expected := srv.URL + "/api/gateway/v1/logout/?next=" + url.QueryEscape(postLogoutBase)
+	if logoutURL != expected {
+		t.Fatalf("trailing slash in base URL not normalized.\n  expected %q\n  got      %q", expected, logoutURL)
+	}
+}
+
+// TestAAPLogoutOnlyUsesConfiguredURLs is an open-redirect prevention test. Even
+// when postLogoutBase points at a different host, the redirect target the browser
+// is sent to must stay locked to the configured AAP origin (a.internalAuthURL);
+// next must be exactly the configured base and must be the only query
+// parameter.
+func TestAAPLogoutOnlyUsesConfiguredURLs(t *testing.T) {
+	t.Parallel()
+
+	srv, _ := newRevokeTestServer(t, http.StatusOK)
+
+	handler := &AAPAuthHandler{
+		internalAuthURL: srv.URL,
+		clientId:        "test-client",
+	}
+
+	// postLogoutBase is a *different* host than the AAP provider. If the redirect
+	// target were ever taken from it, logout would become an open redirect.
+	const postLogoutBase = "https://ui.example.com/ui"
+	logoutURL, err := handler.Logout("the-token", postLogoutBase)
+	if err != nil {
+		t.Fatalf("expected no error, got: %v", err)
+	}
+
+	got, err := url.Parse(logoutURL)
+	if err != nil {
+		t.Fatalf("returned logout URL is not parseable: %v", err)
+	}
+	configured, err := url.Parse(srv.URL)
+	if err != nil {
+		t.Fatalf("failed to parse configured internalAuthURL: %v", err)
+	}
+
+	// Redirect target scheme+host is derived only from a.internalAuthURL, never
+	// from the (different-host) postLogoutBase.
+	if got.Scheme != configured.Scheme || got.Host != configured.Host {
+		t.Fatalf("redirect target must stay on the configured AAP origin %q, got scheme=%q host=%q",
+			srv.URL, got.Scheme, got.Host)
+	}
+	if got.Path != "/api/gateway/v1/logout/" {
+		t.Fatalf("expected session-termination path /api/gateway/v1/logout/, got %q", got.Path)
+	}
+	// next must equal the configured base exactly -- not modified or injected.
+	if rt := got.Query().Get("next"); rt != postLogoutBase {
+		t.Fatalf("expected next to equal the configured base %q, got %q", postLogoutBase, rt)
+	}
+	// The only query parameter is next; nothing else leaks into the target.
+	if n := len(got.Query()); n != 1 {
+		t.Fatalf("expected exactly one query parameter (next), got %d: %v", n, got.Query())
+	}
+}
+
+// TestAAPLogoutReturnsErrorOnRevocationFailure verifies the existing behavior is
+// preserved: if the token-revocation request fails, Logout returns the error and
+// does not issue a redirect.
+func TestAAPLogoutReturnsErrorOnRevocationFailure(t *testing.T) {
+	t.Parallel()
+
+	// Start a server, then close it so the revocation POST fails with a connection
+	// error, exercising the "revocation failed" path deterministically.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	revokeURL := srv.URL
+	srv.Close()
+
+	handler := &AAPAuthHandler{
+		internalAuthURL: revokeURL,
+		clientId:        "test-client",
+	}
+
+	logoutURL, err := handler.Logout("the-token", "https://ui.example.com/ui")
+	if err == nil {
+		t.Fatal("expected an error when token revocation fails, got nil")
+	}
+	if logoutURL != "" {
+		t.Fatalf("expected empty logout URL on revocation failure, got %q", logoutURL)
+	}
+}
+
+// TestAAPLogoutReturnsErrorOnNon2xxRevocation verifies that a non-2xx response
+// from the revocation endpoint is treated as a failure: Logout returns an error
+// and does not issue a session-termination redirect (the token may still be valid).
+func TestAAPLogoutReturnsErrorOnNon2xxRevocation(t *testing.T) {
+	t.Parallel()
+
+	srv, _ := newRevokeTestServer(t, http.StatusInternalServerError)
+
+	handler := &AAPAuthHandler{
+		internalAuthURL: srv.URL,
+		clientId:        "test-client",
+	}
+
+	logoutURL, err := handler.Logout("the-token", "https://ui.example.com/ui")
+	if err == nil {
+		t.Fatal("expected an error when revocation returns a non-2xx status, got nil")
+	}
+	if logoutURL != "" {
+		t.Fatalf("expected empty logout URL on non-2xx revocation, got %q", logoutURL)
+	}
+}