From f9590b3fbf756726045d220b000d47efcad4c60f Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Tue, 8 Aug 2023 07:01:41 -0400 Subject: [PATCH] Addressing comments --- .gitignore | 1 + internal/brokers/broker_test.go | 4 +- internal/testutils/broker.go | 57 +++++++------------ internal/testutils/config/broker-cfg | 7 --- internal/testutils/config/systembus-mock.conf | 13 ----- internal/testutils/dbus.go | 37 +++++++++--- 6 files changed, 51 insertions(+), 68 deletions(-) delete mode 100644 internal/testutils/config/broker-cfg delete mode 100644 internal/testutils/config/systembus-mock.conf diff --git a/.gitignore b/.gitignore index 550323243..e6d4b2fbf 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ # # Binaries for programs and plugins *.so +/authd pam/pam_authd.h # Test binary, built with `go test -c` diff --git a/internal/brokers/broker_test.go b/internal/brokers/broker_test.go index 5217d4b79..90c8d4011 100644 --- a/internal/brokers/broker_test.go +++ b/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") diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go index b63d9bee5..8c59e64cc 100644 --- a/internal/testutils/broker.go +++ b/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 */ diff --git a/internal/testutils/config/broker-cfg b/internal/testutils/config/broker-cfg deleted file mode 100644 index 0497b4ef4..000000000 --- a/internal/testutils/config/broker-cfg +++ /dev/null @@ -1,7 +0,0 @@ -name = %s -brand_icon = mock_icon.png - -[dbus] -name = com.ubuntu.authd.%s -object = /com/ubuntu/authd/%s -interface = com.ubuntu.authd.%s diff --git a/internal/testutils/config/systembus-mock.conf b/internal/testutils/config/systembus-mock.conf deleted file mode 100644 index 6aa4b1350..000000000 --- a/internal/testutils/config/systembus-mock.conf +++ /dev/null @@ -1,13 +0,0 @@ - - - system - - unix:path=%s - - - - - - - diff --git a/internal/testutils/dbus.go b/internal/testutils/dbus.go index 135e897b9..6f87d63a7 100644 --- a/internal/testutils/dbus.go +++ b/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 = ` + + system + + unix:path=%s + + + + + + + +` // 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,8 +82,8 @@ 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 { @@ -76,3 +91,9 @@ func GetSystemBusConnection() (*dbus.Conn, error) { } 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) +}