feat(errors): Add package to control errors returned by the services (#…

…434)

To improve user experience, we need to remove some chained errors
displayed when something fails during authentication. To keep the broker
messages intact, we now rely on a separate error type and gRPC
interceptors to remove and format the final error messages that are
displayed.

UDENG-3420
UDENG-3419
UDENG-3412
UDENG-3418

internal/brokers/dbusbroker.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/godbus/dbus/v5"
 	"github.com/ubuntu/authd/internal/log"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/decorate"
 	"gopkg.in/ini.v1"
 )
@@ -59,7 +60,7 @@ func (b dbusBroker) NewSession(ctx context.Context, username, lang, mode string)
 
 	call := b.dbusObject.CallWithContext(ctx, dbusMethod, 0, username, lang, mode)
 	if err = call.Err; err != nil {
-		return "", "", err
+		return "", "", errmessages.NewErrorToDisplay(err)
 	}
 	if err = call.Store(&sessionID, &encryptionKey); err != nil {
 		return "", "", err
@@ -74,7 +75,7 @@ func (b dbusBroker) GetAuthenticationModes(ctx context.Context, sessionID string
 
 	call := b.dbusObject.CallWithContext(ctx, dbusMethod, 0, sessionID, supportedUILayouts)
 	if err = call.Err; err != nil {
-		return nil, err
+		return nil, errmessages.NewErrorToDisplay(err)
 	}
 	if err = call.Store(&authenticationModes); err != nil {
 		return nil, err
@@ -89,7 +90,7 @@ func (b dbusBroker) SelectAuthenticationMode(ctx context.Context, sessionID, aut
 
 	call := b.dbusObject.CallWithContext(ctx, dbusMethod, 0, sessionID, authenticationModeName)
 	if err = call.Err; err != nil {
-		return nil, err
+		return nil, errmessages.NewErrorToDisplay(err)
 	}
 	if err = call.Store(&uiLayoutInfo); err != nil {
 		return nil, err
@@ -104,7 +105,7 @@ func (b dbusBroker) IsAuthenticated(_ context.Context, sessionID, authentication
 
 	call := b.dbusObject.Call(dbusMethod, 0, sessionID, authenticationData)
 	if err = call.Err; err != nil {
-		return "", "", err
+		return "", "", errmessages.NewErrorToDisplay(err)
 	}
 	if err = call.Store(&access, &data); err != nil {
 		return "", "", err
@@ -119,7 +120,7 @@ func (b dbusBroker) EndSession(ctx context.Context, sessionID string) (err error
 
 	call := b.dbusObject.CallWithContext(ctx, dbusMethod, 0, sessionID)
 	if err = call.Err; err != nil {
-		return err
+		return errmessages.NewErrorToDisplay(err)
 	}
 
 	return nil

internal/daemon/daemon_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd/internal/daemon"
 	"github.com/ubuntu/authd/internal/daemon/testdata/grpctestservice"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/credentials/insecure"
@@ -149,7 +150,7 @@ func TestServe(t *testing.T) {
 			t.Parallel()
 
 			registerGRPC := func(context.Context) *grpc.Server {
-				return grpc.NewServer()
+				return grpc.NewServer(grpc.UnaryInterceptor(errmessages.RedactErrorInterceptor))
 			}
 			socketPath := filepath.Join(t.TempDir(), "manual.socket")
 
@@ -214,7 +215,7 @@ func TestQuit(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			grpcServer := grpc.NewServer()
+			grpcServer := grpc.NewServer(grpc.UnaryInterceptor(errmessages.RedactErrorInterceptor))
 			defer grpcServer.Stop()
 			registerGRPC := func(context.Context) *grpc.Server {
 				var service testGRPCService
@@ -295,7 +296,7 @@ func createClientConnection(t *testing.T, socketPath string) (success bool, disc
 	go func() {
 		defer close(connected)
 		var err error
-		conn, err = grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
+		conn, err = grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithUnaryInterceptor(errmessages.FormatErrorMessage))
 		require.NoError(t, err, "Could not connect to grpc server")
 
 		// The daemon tests require an active connection, so we need to block here until the connection is ready.

internal/services/errmessages/error.go

@@ -0,0 +1,11 @@
+package errmessages
+
+// ErrToDisplay defines an error that needs to be sent unaltered to the client.
+type ErrToDisplay struct {
+	error
+}
+
+// NewErrorToDisplay returns a new ErrorToDisplay.
+func NewErrorToDisplay(err error) error {
+	return ErrToDisplay{err}
+}

internal/services/errmessages/internal_test.go

@@ -0,0 +1,100 @@
+package errmessages
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+func TestRedactErrorInterceptor(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		inputError error
+
+		wantMessage string
+	}{
+		"Trim input down to ErrToDisplay": {
+			inputError:  fmt.Errorf("Error to be redacted: %w", ErrToDisplay{errors.New("Error to be shown")}),
+			wantMessage: "Error to be shown",
+		},
+		"Return original error": {
+			inputError:  errors.New("Not a redacted error"),
+			wantMessage: "Not a redacted error",
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := RedactErrorInterceptor(context.TODO(), testRequest{tc.inputError}, nil, testHandler)
+			require.Error(t, err, "RedactErrorInterceptor should return an error")
+			require.Equal(t, tc.wantMessage, err.Error(), "RedactErrorInterceptor returned unexpected error message")
+		})
+	}
+}
+
+func TestFormatErrorMessage(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		inputError error
+
+		wantMessage string
+	}{
+		"Non-gRPC error is left untouched": {
+			inputError:  errors.New("Non-gRPC error"),
+			wantMessage: "Non-gRPC error",
+		},
+		"Unrecognized error is left untouched": {
+			inputError:  status.Error(100, "Unrecognized error"),
+			wantMessage: "error Code(100) from server: Unrecognized error",
+		},
+		"Code Canceled is left untouched": {
+			inputError:  status.Error(codes.Canceled, "Canceled error"),
+			wantMessage: "rpc error: code = Canceled desc = Canceled error",
+		},
+
+		"Parse code Unavailable": {
+			inputError:  status.Error(codes.Unavailable, "Unavailable error"),
+			wantMessage: "couldn't connect to authd daemon: Unavailable error",
+		},
+		"Parse code DeadlineExceeded": {
+			inputError:  status.Error(codes.DeadlineExceeded, "DeadlineExceeded error"),
+			wantMessage: "service took too long to respond. Disconnecting client",
+		},
+		"Parse code Unknown": {
+			inputError:  status.Error(codes.Unknown, "Unknown error"),
+			wantMessage: "Unknown error",
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			err := FormatErrorMessage(context.TODO(), "", testRequest{tc.inputError}, nil, nil, testInvoker)
+			require.Error(t, err, "FormatErrorMessage should return an error")
+			require.Equal(t, tc.wantMessage, err.Error(), "FormatErrorMessage returned unexpected error message")
+		})
+	}
+}
+
+type testRequest struct {
+	err error
+}
+
+func testHandler(ctx context.Context, req any) (any, error) {
+	//nolint:forcetypeassert // This is only used in the tests and we know the type.
+	return nil, req.(testRequest).err
+}
+
+func testInvoker(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, opts ...grpc.CallOption) error {
+	//nolint:forcetypeassert // This is only used in the tests and we know the type.
+	return req.(testRequest).err
+}

internal/services/errmessages/redactor.go

@@ -0,0 +1,62 @@
+// Package errmessages formats the error messages that are sent to the client.
+package errmessages
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"log/slog"
+
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+// RedactErrorInterceptor redacts some of the attached errors before sending it to the client.
+//
+// It unwraps the error up to the first ErrToDisplay and sends it to the client. If none is found, it sends the original error.
+func RedactErrorInterceptor(ctx context.Context, req any, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
+	m, err := handler(ctx, req)
+	if err != nil {
+		slog.Warn(err.Error())
+		var redactedError ErrToDisplay
+		if !errors.As(err, &redactedError) {
+			return m, err
+		}
+		return m, redactedError
+	}
+	return m, nil
+}
+
+// FormatErrorMessage formats the error message received by the client to avoid printing useless information.
+//
+// It converts the gRPC error to a more human-readable error with a better message.
+func FormatErrorMessage(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
+	err := invoker(ctx, method, req, reply, cc, opts...)
+	if err == nil {
+		return nil
+	}
+	st, grpcErr := status.FromError(err)
+	if !grpcErr {
+		return err
+	}
+
+	switch st.Code() {
+	// no daemon
+	case codes.Unavailable:
+		err = fmt.Errorf("couldn't connect to authd daemon: %v", st.Message())
+	// timeout
+	case codes.DeadlineExceeded:
+		err = fmt.Errorf("service took too long to respond. Disconnecting client")
+	// regular error without annotation
+	case codes.Unknown:
+		err = errors.New(st.Message())
+	// likely means that IsAuthenticated got cancelled, so we need to keep the error intact
+	case codes.Canceled:
+		break
+	// grpc error, just format it
+	default:
+		err = fmt.Errorf("error %s from server: %v", st.Code(), st.Message())
+	}
+	return err
+}

internal/services/manager.go

@@ -7,6 +7,7 @@ import (
 	"github.com/ubuntu/authd"
 	"github.com/ubuntu/authd/internal/brokers"
 	"github.com/ubuntu/authd/internal/log"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/authd/internal/services/nss"
 	"github.com/ubuntu/authd/internal/services/pam"
 	"github.com/ubuntu/authd/internal/services/permissions"
@@ -57,7 +58,7 @@ func NewManager(ctx context.Context, cacheDir, brokersConfPath string, configure
 func (m Manager) RegisterGRPCServices(ctx context.Context) *grpc.Server {
 	log.Debug(ctx, "Registering GRPC services")
 
-	opts := []grpc.ServerOption{permissions.WithUnixPeerCreds(), grpc.UnaryInterceptor(m.globalPermissions)}
+	opts := []grpc.ServerOption{permissions.WithUnixPeerCreds(), grpc.ChainUnaryInterceptor(m.globalPermissions, errmessages.RedactErrorInterceptor)}
 	grpcServer := grpc.NewServer(opts...)
 
 	authd.RegisterNSSServer(grpcServer, m.nssService)

internal/services/manager_test.go

@@ -11,11 +11,10 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd"
 	"github.com/ubuntu/authd/internal/services"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/authd/internal/testutils"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
-	"google.golang.org/grpc/status"
 )
 
 func TestNewManager(t *testing.T) {
@@ -89,7 +88,7 @@ func TestAccessAuthorization(t *testing.T) {
 		require.NoError(t, <-serverDone, "gRPC server should not return an error from serving")
 	}()
 
-	conn, err := grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	conn, err := grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithUnaryInterceptor(errmessages.FormatErrorMessage))
 	require.NoError(t, err, "Setup: could not dial the server")
 
 	// Global authorization for PAM is always denied for non root user.
@@ -100,10 +99,7 @@ func TestAccessAuthorization(t *testing.T) {
 	// Global authorization for NSS is always granted for non root user.
 	nssClient := authd.NewNSSClient(conn)
 	_, err = nssClient.GetPasswdByName(context.Background(), &authd.GetPasswdByNameRequest{Name: ""})
-	// The returned error should be InvalidArgument, as the name is empty (and prooving we called the method).
-	s, ok := status.FromError(err)
-	require.True(t, ok, "Expected a GRPC error from the server")
-	require.Equal(t, s.Code(), codes.InvalidArgument, "Expected an InvalidArgument error, and thus, the method was called")
+	require.Error(t, err, "Expected a GRPC error from the server")
 
 	err = conn.Close()
 	require.NoError(t, err, "Teardown: could not close the client connection")

internal/services/nss/nss_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd"
 	"github.com/ubuntu/authd/internal/brokers"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/authd/internal/services/nss"
 	"github.com/ubuntu/authd/internal/services/permissions"
 	permissionstestutils "github.com/ubuntu/authd/internal/services/permissions/testutils"
@@ -291,7 +292,7 @@ func newNSSClient(t *testing.T, sourceDB string, currentUserNotRoot bool) (clien
 
 	service := nss.NewService(context.Background(), newUserManagerForTests(t, sourceDB), newBrokersManagerForTests(t), &pm)
 
-	grpcServer := grpc.NewServer(permissions.WithUnixPeerCreds(), grpc.UnaryInterceptor(enableCheckGlobalAccess(service)))
+	grpcServer := grpc.NewServer(permissions.WithUnixPeerCreds(), grpc.ChainUnaryInterceptor(enableCheckGlobalAccess(service), errmessages.RedactErrorInterceptor))
 	authd.RegisterNSSServer(grpcServer, service)
 	done := make(chan struct{})
 	go func() {

internal/services/pam/pam_test.go

@@ -17,6 +17,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd"
 	"github.com/ubuntu/authd/internal/brokers"
+	"github.com/ubuntu/authd/internal/services/errmessages"
 	"github.com/ubuntu/authd/internal/services/pam"
 	"github.com/ubuntu/authd/internal/services/permissions"
 	permissionstestutils "github.com/ubuntu/authd/internal/services/permissions/testutils"
@@ -745,7 +746,7 @@ func newPamClient(t *testing.T, m *users.Manager, brokerManager *brokers.Manager
 
 	service := pam.NewService(context.Background(), m, brokerManager, pm)
 
-	grpcServer := grpc.NewServer(permissions.WithUnixPeerCreds(), grpc.UnaryInterceptor(enableCheckGlobalAccess(service)))
+	grpcServer := grpc.NewServer(permissions.WithUnixPeerCreds(), grpc.ChainUnaryInterceptor(enableCheckGlobalAccess(service), errmessages.RedactErrorInterceptor))
 	authd.RegisterPAMServer(grpcServer, service)
 	done := make(chan struct{})
 	go func() {
@@ -757,7 +758,7 @@ func newPamClient(t *testing.T, m *users.Manager, brokerManager *brokers.Manager
 		<-done
 	})
 
-	conn, err := grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	conn, err := grpc.NewClient("unix://"+socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithUnaryInterceptor(errmessages.FormatErrorMessage))
 	require.NoError(t, err, "Setup: Could not connect to GRPC server")
 
 	t.Cleanup(func() { _ = conn.Close() }) // We don't care about the error on cleanup

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

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: missing key "userinfo" in returned message, got: {}
+	err: can't check authentication: missing key "userinfo" in returned message, got: {}

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

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: failed to update user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not update local groups for user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not fetch existing local group: open testdata/TestIsAuthenticated/does_not_exists.group: no such file or directory
+	err: can't check authentication: failed to update user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not update local groups for user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not fetch existing local group: open testdata/TestIsAuthenticated/does_not_exists.group: no such file or directory

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

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: broker "BrokerMock": IsAuthenticated errored out
+	err: broker "BrokerMock": IsAuthenticated errored out

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

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: invalid access authentication key: invalid
+	err: can't check authentication: invalid access authentication key: invalid

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

@@ -1,5 +1,5 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: response returned by the broker is not a valid json: invalid character 'i' looking for beginning of value
+	err: can't check authentication: response returned by the broker is not a valid json: invalid character 'i' looking for beginning of value
 Broker returned: invalid

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

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: message is not JSON formatted: json: cannot unmarshal string into Go value of type brokers.userInfo
+	err: can't check authentication: message is not JSON formatted: json: cannot unmarshal string into Go value of type brokers.userInfo