Addressing comments

.gitignore

@@ -3,6 +3,7 @@
 #
 # Binaries for programs and plugins
 *.so
+/authd
 pam/pam_authd.h
 
 # Test binary, built with `go test -c`

internal/brokers/broker_test.go

@@ -287,11 +287,11 @@ func TestEndSession(t *testing.T) {
 func newBrokerForTests(t *testing.T) brokers.Broker {
 	t.Helper()
 
-	cfgPath := testutils.StartBusBrokerMock(t, testutils.WithBrokerName(t.Name()))
+	cfgPath := testutils.StartBusBrokerMock(t)
 
 	conn, err := testutils.GetSystemBusConnection()
 	require.NoError(t, err, "Setup: could not connect to system bus")
-	t.Cleanup(func() { _ = conn.Close })
+	t.Cleanup(func() { require.NoError(t, conn.Close(), "Cleanup: Failed to close the connection") })
 
 	b, err := brokers.NewBroker(context.Background(), "BrokerMock", cfgPath, conn)
 	require.NoError(t, err, "Setup: could not create broker")

internal/testutils/broker.go

@@ -23,28 +23,20 @@ const (
 	interfaceFmt  = "com.ubuntu.authd.%s"
 )
 
-//go:embed config/broker-cfg
-var brokerConfigTemplate string
+var brokerConfigTemplate = `name = %s
+brand_icon = mock_icon.png
+
+[dbus]
+name = com.ubuntu.authd.%s
+object = /com/ubuntu/authd/%s
+interface = com.ubuntu.authd.%s
+`
 
 type isAuthorizedCtx struct {
 	ctx        context.Context
 	cancelFunc context.CancelFunc
 }
 
-type brokerOptions struct {
-	brokerName string
-}
-
-// BrokerOption is the function signature used to tweak the broker creation.
-type BrokerOption func(*brokerOptions)
-
-// WithBrokerName overrides the default broker name.
-func WithBrokerName(name string) func(o *brokerOptions) {
-	return func(o *brokerOptions) {
-		o.brokerName = strings.ReplaceAll(name, "/", "_")
-	}
-}
-
 // BrokerMock is a broker mock that should be used in the tests.
 type BrokerMock struct {
 	isAuthorizedCalls   map[string]isAuthorizedCtx
@@ -58,22 +50,16 @@ type BrokerBusMock struct {
 
 // StartBusBrokerMock starts the D-Bus service and exports it on the system bus.
 // It returns the configuration file path for the exported broker.
-func StartBusBrokerMock(t *testing.T, args ...BrokerOption) string {
+func StartBusBrokerMock(t *testing.T) string {
 	t.Helper()
 
-	opts := brokerOptions{
-		brokerName: "BrokerMock",
-	}
-	for _, opt := range args {
-		opt(&opts)
-	}
-
-	busObjectPath := fmt.Sprintf(objectPathFmt, opts.brokerName)
-	busInterface := fmt.Sprintf(interfaceFmt, opts.brokerName)
+	brokerName := strings.ReplaceAll(t.Name(), "/", "_")
+	busObjectPath := fmt.Sprintf(objectPathFmt, brokerName)
+	busInterface := fmt.Sprintf(interfaceFmt, brokerName)
 
 	conn, err := dbus.ConnectSystemBus()
 	require.NoError(t, err, "Setup: could not connect to system bus")
-	t.Cleanup(func() { _ = conn.Close() })
+	t.Cleanup(func() { require.NoError(t, conn.Close(), "Teardown: could not close system bus connection") })
 
 	bus := BrokerBusMock{broker: *NewBrokerMock()}
 	err = conn.Export(&bus, dbus.ObjectPath(busObjectPath), busInterface)
@@ -95,11 +81,12 @@ func StartBusBrokerMock(t *testing.T, args ...BrokerOption) string {
 	require.NoError(t, err, "Setup: could not request mock broker name")
 	require.Equal(t, reply, dbus.RequestNameReplyPrimaryOwner, "Setup: mock broker name already taken")
 
-	configPath := writeConfig(t, opts.brokerName)
+	configPath := writeConfig(t, brokerName)
 
 	t.Cleanup(func() {
-		_, _ = conn.ReleaseName(busInterface)
-		_ = conn.Close()
+		r, err := conn.ReleaseName(busInterface)
+		require.NoError(t, err, "Teardown: could not release mock broker name")
+		require.Equal(t, r, dbus.ReleaseNameReplyReleased, "Teardown: mock broker name not released")
 	})
 
 	return configPath
@@ -108,12 +95,7 @@ func StartBusBrokerMock(t *testing.T, args ...BrokerOption) string {
 func writeConfig(t *testing.T, name string) string {
 	t.Helper()
 
-	if name == "" {
-		name = "BrokerMock"
-	}
-
-	tmpDir := t.TempDir()
-	cfgPath := filepath.Join(tmpDir, "broker-cfg")
+	cfgPath := filepath.Join(t.TempDir(), "broker-cfg")
 
 	s := fmt.Sprintf(brokerConfigTemplate, name, name, name, name)
 	err := os.WriteFile(cfgPath, []byte(s), 0600)
@@ -124,11 +106,10 @@ func writeConfig(t *testing.T, name string) string {
 
 // NewBrokerMock creates a new fresh broker mock.
 func NewBrokerMock() *BrokerMock {
-	b := BrokerMock{
+	return &BrokerMock{
 		isAuthorizedCalls:   map[string]isAuthorizedCtx{},
 		isAuthorizedCallsMu: sync.Mutex{},
 	}
-	return &b
 }
 
 /* BrokerBusMock implementation */

internal/testutils/dbus.go

@@ -3,8 +3,7 @@ package testutils
 
 import (
 	"context"
-	// embed is used to embed the mock dbus configuration files for tests.
-	_ "embed"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -16,14 +15,30 @@ import (
 
 const defaultSystemBusAddress = "unix:path=/var/run/dbus/system_bus_socket"
 
-//go:embed config/systembus-mock.conf
-var systemBusMockCfg string // It has a formatting directive for the listen path that will be used by the bus.
+var systemBusMockCfg = `<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
+<busconfig>
+  <type>system</type>
+  <keep_umask/>
+  <listen>unix:path=%s</listen>
+  <policy context="default">
+    <allow user="*"/>
+    <allow send_destination="*" eavesdrop="true"/>
+    <allow eavesdrop="true"/>
+    <allow own="*"/>
+  </policy>
+</busconfig>
+`
 
 // StartSystemBusMock starts a mock dbus daemon and returns a cancel function to stop it.
 //
 // This function uses t.Setenv to set the DBUS_SYSTEM_BUS_ADDRESS environment, so it shouldn't be used in parallel tests
 // that rely on the mentioned variable.
 func StartSystemBusMock() (func(), error) {
+	if isRunning() {
+		return nil, errors.New("system bus mock is already running")
+	}
+
 	tmp, err := os.MkdirTemp(os.TempDir(), "authd-system-bus-mock")
 	if err != nil {
 		return nil, err
@@ -34,7 +49,7 @@ func StartSystemBusMock() (func(), error) {
 
 	err = os.WriteFile(cfgPath, []byte(fmt.Sprintf(systemBusMockCfg, listenPath)), 0600)
 	if err != nil {
-		_ = os.RemoveAll(tmp)
+		err = errors.Join(err, os.RemoveAll(tmp))
 		return nil, err
 	}
 
@@ -43,7 +58,7 @@ func StartSystemBusMock() (func(), error) {
 	cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath)
 	if err := cmd.Start(); err != nil {
 		busCancel()
-		_ = os.RemoveAll(tmp)
+		err = errors.Join(err, os.RemoveAll(tmp))
 		return nil, err
 	}
 	// Give some time for the daemon to start.
@@ -67,12 +82,18 @@ func StartSystemBusMock() (func(), error) {
 // GetSystemBusConnection returns a connection to the system bus with a safety check to avoid mistakenly connecting to the
 // actual system bus.
 func GetSystemBusConnection() (*dbus.Conn, error) {
-	if busAddr := os.Getenv("DBUS_SYSTEM_BUS_ADDRESS"); busAddr == "" || busAddr == defaultSystemBusAddress {
-		return nil, fmt.Errorf("system bus mock is not running. If that's intended, manually connect to the system bus instead of using this function")
+	if !isRunning() {
+		return nil, errors.New("system bus mock is not running. If that's intended, manually connect to the system bus instead of using this function")
 	}
 	conn, err := dbus.ConnectSystemBus()
 	if err != nil {
 		return nil, err
 	}
 	return conn, nil
 }
+
+// isRunning checks if the system bus mock is running.
+func isRunning() bool {
+	busAddr := os.Getenv("DBUS_SYSTEM_BUS_ADDRESS")
+	return !(busAddr == "" || busAddr == defaultSystemBusAddress)
+}