Avoid polling token endpoint after login request cancelled

internal/brokers/broker.go

@@ -142,26 +142,14 @@ func (b Broker) SelectAuthenticationMode(ctx context.Context, sessionID, authent
 func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationData string) (access string, data string, err error) {
 	sessionID = b.parseSessionID(sessionID)
 
-	// monitor ctx in goroutine to call cancel
-	done := make(chan struct{})
-	go func() {
-		access, data, err = b.brokerer.IsAuthenticated(ctx, sessionID, authenticationData)
-		close(done)
-	}()
-
-	select {
-	case <-done:
-		if err != nil {
-			return "", "", err
-		}
-	case <-ctx.Done():
-		b.cancelIsAuthenticated(ctx, sessionID)
-		<-done
+	access, data, err = b.brokerer.IsAuthenticated(ctx, sessionID, authenticationData)
+	if err != nil {
+		return "", "", err
 	}
 
 	// Validate access authentication.
 	if !slices.Contains(auth.Replies, access) {
-		return "", "", fmt.Errorf("invalid access authentication key: %v", access)
+		return "", "", fmt.Errorf("invalid authentication reply: %v", access)
 	}
 
 	if data == "" {
@@ -223,14 +211,6 @@ func (b Broker) endSession(ctx context.Context, sessionID string) (err error) {
 	return b.brokerer.EndSession(ctx, sessionID)
 }
 
-// cancelIsAuthenticated calls the broker corresponding method.
-// If the session does not have a pending IsAuthenticated call, this is a no-op.
-//
-// Even though this is a public method, it should only be interacted with through IsAuthenticated and ctx cancellation.
-func (b Broker) cancelIsAuthenticated(ctx context.Context, sessionID string) {
-	b.brokerer.CancelIsAuthenticated(ctx, sessionID)
-}
-
 // UserPreCheck calls the broker corresponding method.
 func (b Broker) UserPreCheck(ctx context.Context, username string) (userinfo string, err error) {
 	log.Debugf(context.TODO(), "Pre-checking user %q", username)

internal/brokers/broker_test.go

@@ -213,7 +213,7 @@ func TestIsAuthenticated(t *testing.T) {
 		cancelFirstCall bool
 	}{
 		"Successfully_authenticate":                                        {sessionID: "success"},
-		"Successfully_authenticate_after_cancelling_first_call":            {sessionID: "ia_second_call", secondCall: true},
+		"Successfully_authenticate_after_cancelling_first_call":            {sessionID: "ia_second_call", secondCall: true, cancelFirstCall: true},
 		"Denies_authentication_when_broker_times_out":                      {sessionID: "ia_timeout"},
 		"Adds_default_groups_even_if_broker_did_not_set_them":              {sessionID: "ia_info_empty_groups"},
 		"No_error_when_auth.Next_and_no_data":                              {sessionID: "ia_next"},
@@ -236,7 +236,7 @@ func TestIsAuthenticated(t *testing.T) {
 		"Error_when_broker_returns_data_on_auth.Cancelled":                    {sessionID: "ia_cancelled_with_data"},
 		"Error_when_broker_returns_no_data_on_auth.Denied":                    {sessionID: "ia_denied_without_data"},
 		"Error_when_broker_returns_no_data_on_auth.Retry":                     {sessionID: "ia_retry_without_data"},
-		"Error_when_calling_IsAuthenticated_a_second_time_without_cancelling": {sessionID: "ia_second_call", secondCall: true, cancelFirstCall: true},
+		"Error_when_calling_IsAuthenticated_a_second_time_without_cancelling": {sessionID: "ia_second_call", secondCall: true},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -264,7 +264,7 @@ func TestIsAuthenticated(t *testing.T) {
 			time.Sleep(time.Second)
 
 			if tc.secondCall {
-				if !tc.cancelFirstCall {
+				if tc.cancelFirstCall {
 					cancel()
 					<-done
 				}

internal/brokers/dbusbroker.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	"github.com/godbus/dbus/v5"
+	"github.com/ubuntu/authd/internal/brokers/auth"
 	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/authd/log"
 	"github.com/ubuntu/decorate"
@@ -98,9 +99,16 @@ func (b dbusBroker) SelectAuthenticationMode(ctx context.Context, sessionID, aut
 }
 
 // IsAuthenticated calls the corresponding method on the broker bus and returns the user information and access.
-func (b dbusBroker) IsAuthenticated(_ context.Context, sessionID, authenticationData string) (access, data string, err error) {
-	// We don’t want to cancel the context when the parent call is cancelled.
-	call, err := b.call(context.Background(), "IsAuthenticated", sessionID, authenticationData)
+func (b dbusBroker) IsAuthenticated(ctx context.Context, sessionID, authenticationData string) (access, data string, err error) {
+	call, err := b.call(ctx, "IsAuthenticated", sessionID, authenticationData)
+	if errors.Is(err, context.Canceled) {
+		// In contrast to gRPC, D-Bus does not support cancelling ongoing method calls through context cancellation.
+		// BusObject.CallWithContext only aborts the call on the client side when the context is cancelled.
+		// So if that happens, we need to call CancelIsAuthenticated to also cancel the ongoing call on the broker side,
+		// to avoid that the broker continuous its work to authenticate the user.
+		b.CancelIsAuthenticated(context.Background(), sessionID)
+		return auth.Cancelled, "", nil
+	}
 	if err != nil {
 		return "", "", err
 	}
@@ -121,8 +129,7 @@ func (b dbusBroker) EndSession(ctx context.Context, sessionID string) (err error
 
 // CancelIsAuthenticated calls the corresponding method on the broker bus.
 func (b dbusBroker) CancelIsAuthenticated(ctx context.Context, sessionID string) {
-	// We don’t want to cancel the context when the parent call is cancelled.
-	if _, err := b.call(context.Background(), "CancelIsAuthenticated", sessionID); err != nil {
+	if _, err := b.call(ctx, "CancelIsAuthenticated", sessionID); err != nil {
 		log.Errorf(ctx, "could not cancel IsAuthenticated call for session %q: %v", sessionID, err)
 	}
 }

internal/brokers/testdata/golden/TestIsAuthenticated/Error_when_broker_returns_invalid_access

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	data: 
-	err: invalid access authentication key: invalid
+	err: invalid authentication reply: invalid

internal/services/errmessages/error.go

@@ -5,6 +5,11 @@ type ToDisplayError struct {
 	error
 }
 
+// Unwrap returns the underlying error.
+func (e ToDisplayError) Unwrap() error {
+	return e.error
+}
+
 // NewToDisplayError returns a new ErrorToDisplay.
 func NewToDisplayError(err error) error {
 	return ToDisplayError{err}

internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_broker_returns_invalid_access/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: can't check authentication: invalid access authentication key: invalid
+	err: can't check authentication: invalid authentication reply: invalid

pam/main-exec.go

@@ -7,11 +7,13 @@ import (
 	"fmt"
 	"os"
 	"runtime"
+	"syscall"
 	"time"
 
 	"github.com/msteinert/pam/v2"
 	"github.com/ubuntu/authd/log"
 	"github.com/ubuntu/authd/pam/internal/dbusmodule"
+	"golang.org/x/sys/unix"
 )
 
 var (
@@ -25,6 +27,19 @@ func init() {
 	// calling the dbus services from the process and so that the module PID
 	// check won't fail.
 	runtime.LockOSThread()
+
+	// Ask the kernel to send SIGTERM if the parent dies
+	if err := unix.Prctl(unix.PR_SET_PDEATHSIG, uintptr(syscall.SIGTERM), 0, 0, 0); err != nil {
+		log.Errorf(context.Background(), "failed to set PDEATHSIG: %v", err)
+		os.Exit(1)
+	}
+
+	// Check if parent is still alive
+	ppid := unix.Getppid()
+	if ppid == 1 {
+		log.Error(context.Background(), "parent is already gone; exiting")
+		os.Exit(1)
+	}
 }
 
 func mainFunc() error {