refactor: Example and Mock brokers no longer rely on internal/ (#205)

Having the `responses` subpackage in `internal/brokers/` was not great
from a structural POV as it was causing our "external" brokers, such as
the `ExampleBroker` and the `BrokerMock` to rely on internal/ for their
return values. This is not ideal, since their purpose is to mimic
external broker behaviours (in some way or another).

Removing the subpackage does result in some copying while we don't
stabilize the API to move some of these definitions to outside packages,
but I still stand by the motto "a little copying is better than a little
dependency".

internal/brokers/examplebroker/broker.go → examplebroker/broker.go

@@ -21,7 +21,6 @@ import (
 	"time"
 
 	"github.com/google/uuid"
-	"github.com/ubuntu/authd/internal/brokers/responses"
 	"golang.org/x/exp/slices"
 )
 
@@ -35,6 +34,19 @@ const (
 	mustReset
 )
 
+const (
+	// AuthGranted is the response when the authentication is granted.
+	AuthGranted = "granted"
+	// AuthDenied is the response when the authentication is denied.
+	AuthDenied = "denied"
+	// AuthCancelled is the response when the authentication is cancelled.
+	AuthCancelled = "cancelled"
+	// AuthRetry is the response when the authentication needs to be retried (another chance).
+	AuthRetry = "retry"
+	// AuthNext is the response when another MFA (including changing password) authentication is necessary.
+	AuthNext = "next"
+)
+
 type sessionInfo struct {
 	username    string
 	lang        string
@@ -442,26 +454,26 @@ func (b *Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationD
 	}()
 
 	access, data, err = b.handleIsAuthenticated(b.isAuthenticatedCalls[sessionID].ctx, sessionInfo, authData)
-	if access == responses.AuthGranted && sessionInfo.currentAuthStep < sessionInfo.neededAuthSteps {
+	if access == AuthGranted && sessionInfo.currentAuthStep < sessionInfo.neededAuthSteps {
 		sessionInfo.currentAuthStep++
-		access = responses.AuthNext
+		access = AuthNext
 		data = ""
-	} else if access == responses.AuthRetry {
+	} else if access == AuthRetry {
 		sessionInfo.attemptsPerMode[sessionInfo.currentAuthMode]++
 		if sessionInfo.attemptsPerMode[sessionInfo.currentAuthMode] >= maxAttempts {
-			access = responses.AuthDenied
+			access = AuthDenied
 		}
 	}
 
 	// Store last successful authentication mode for this user in the broker.
-	if access == responses.AuthGranted {
+	if access == AuthGranted {
 		b.userLastSelectedModeMu.Lock()
 		b.userLastSelectedMode[sessionInfo.username] = sessionInfo.firstSelectedMode
 		b.userLastSelectedModeMu.Unlock()
 	}
 
 	if err = b.updateSession(sessionID, sessionInfo); err != nil {
-		return responses.AuthDenied, "", err
+		return AuthDenied, "", err
 	}
 
 	return access, data, err
@@ -472,74 +484,74 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, sessionInfo sessionI
 	// Decrypt challenge if present.
 	challenge, err := decodeRawChallenge(b.privateKey, authData["challenge"])
 	if err != nil {
-		return responses.AuthRetry, fmt.Sprintf(`{"message": "could not decode challenge: %v"}`, err), nil
+		return AuthRetry, fmt.Sprintf(`{"message": "could not decode challenge: %v"}`, err), nil
 	}
 
 	// Note that the "wait" authentication can be cancelled and switch to another mode with a challenge.
 	// Take into account the cancellation.
 	switch sessionInfo.currentAuthMode {
 	case "password":
 		if challenge != "goodpass" {
-			return responses.AuthRetry, `{"message": "invalid password, should be goodpass"}`, nil
+			return AuthRetry, `{"message": "invalid password, should be goodpass"}`, nil
 		}
 
 	case "pincode":
 		if challenge != "4242" {
-			return responses.AuthRetry, `{"message": "invalid pincode, should be 4242"}`, nil
+			return AuthRetry, `{"message": "invalid pincode, should be 4242"}`, nil
 		}
 
 	case "totp_with_button", "totp":
 		wantedCode := sessionInfo.allModes[sessionInfo.currentAuthMode]["wantedCode"]
 		if challenge != wantedCode {
-			return responses.AuthRetry, `{"message": "invalid totp code"}`, nil
+			return AuthRetry, `{"message": "invalid totp code"}`, nil
 		}
 
 	case "phoneack1":
 		// TODO: should this be an error rather (not expected data from the PAM module?
 		if authData["wait"] != "true" {
-			return responses.AuthDenied, `{"message": "phoneack1 should have wait set to true"}`, nil
+			return AuthDenied, `{"message": "phoneack1 should have wait set to true"}`, nil
 		}
 		// Send notification to phone1 and wait on server signal to return if OK or not
 		select {
 		case <-time.After(2 * time.Second):
 		case <-ctx.Done():
-			return responses.AuthCancelled, "", nil
+			return AuthCancelled, "", nil
 		}
 
 	case "phoneack2":
 		if authData["wait"] != "true" {
-			return responses.AuthDenied, `{"message": "phoneack2 should have wait set to true"}`, nil
+			return AuthDenied, `{"message": "phoneack2 should have wait set to true"}`, nil
 		}
 
 		// This one is failing remotely as an example
 		select {
 		case <-time.After(2 * time.Second):
-			return responses.AuthDenied, `{"message": "Timeout reached"}`, nil
+			return AuthDenied, `{"message": "Timeout reached"}`, nil
 		case <-ctx.Done():
-			return responses.AuthCancelled, "", nil
+			return AuthCancelled, "", nil
 		}
 
 	case "fidodevice1":
 		if authData["wait"] != "true" {
-			return responses.AuthDenied, `{"message": "fidodevice1 should have wait set to true"}`, nil
+			return AuthDenied, `{"message": "fidodevice1 should have wait set to true"}`, nil
 		}
 
 		// simulate direct exchange with the FIDO device
 		select {
 		case <-time.After(2 * time.Second):
 		case <-ctx.Done():
-			return responses.AuthCancelled, "", nil
+			return AuthCancelled, "", nil
 		}
 
 	case "qrcodewithtypo":
 		if authData["wait"] != "true" {
-			return responses.AuthDenied, `{"message": "qrcodewithtypo should have wait set to true"}`, nil
+			return AuthDenied, `{"message": "qrcodewithtypo should have wait set to true"}`, nil
 		}
 		// Simulate connexion with remote server to check that the correct code was entered
 		select {
 		case <-time.After(2 * time.Second):
 		case <-ctx.Done():
-			return responses.AuthCancelled, "", nil
+			return AuthCancelled, "", nil
 		}
 	}
 
@@ -549,25 +561,25 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, sessionInfo sessionI
 		if challenge != "" {
 			// validate challenge given manually by the user
 			if challenge != "aaaaa" {
-				return responses.AuthDenied, `{"message": "invalid challenge, should be aaaaa"}`, nil
+				return AuthDenied, `{"message": "invalid challenge, should be aaaaa"}`, nil
 			}
 		} else if authData["wait"] == "true" {
 			// we are simulating clicking on the url signal received by the broker
 			// this can be cancelled to resend a challenge
 			select {
 			case <-time.After(10 * time.Second):
 			case <-ctx.Done():
-				return responses.AuthCancelled, "", nil
+				return AuthCancelled, "", nil
 			}
 		} else {
-			return responses.AuthDenied, `{"message": "challenge timeout "}`, nil
+			return AuthDenied, `{"message": "challenge timeout "}`, nil
 		}
 	}
 
 	if _, exists := exampleUsers[sessionInfo.username]; !exists && !strings.HasPrefix(sessionInfo.username, "user-integration") {
-		return responses.AuthDenied, `{"message": "user not found"}`, nil
+		return AuthDenied, `{"message": "user not found"}`, nil
 	}
-	return responses.AuthGranted, fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(sessionInfo.username)), nil
+	return AuthGranted, fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(sessionInfo.username)), nil
 }
 
 // decodeRawChallenge extract the base64 challenge and try to decrypt it with the private key.

internal/brokers/broker.go

@@ -11,17 +11,30 @@ import (
 	"sync"
 
 	"github.com/godbus/dbus/v5"
-	"github.com/ubuntu/authd/internal/brokers/responses"
 	"github.com/ubuntu/authd/internal/log"
 	"github.com/ubuntu/authd/internal/users"
 	"github.com/ubuntu/decorate"
 	"golang.org/x/exp/slices"
 )
 
+const localBrokerName = "local"
+
 const (
-	localBrokerName = "local"
+	// AuthGranted is the response when the authentication is granted.
+	AuthGranted = "granted"
+	// AuthDenied is the response when the authentication is denied.
+	AuthDenied = "denied"
+	// AuthCancelled is the response when the authentication is cancelled.
+	AuthCancelled = "cancelled"
+	// AuthRetry is the response when the authentication needs to be retried (another chance).
+	AuthRetry = "retry"
+	// AuthNext is the response when another MFA (including changing password) authentication is necessary.
+	AuthNext = "next"
 )
 
+// AuthReplies is the list of all possible authentication replies.
+var AuthReplies = []string{AuthGranted, AuthDenied, AuthCancelled, AuthRetry, AuthNext}
+
 type brokerer interface {
 	NewSession(ctx context.Context, username, lang, mode string) (sessionID, encryptionKey string, err error)
 	GetAuthenticationModes(ctx context.Context, sessionID string, supportedUILayouts []map[string]string) (authenticationModes []map[string]string, err error)
@@ -151,7 +164,7 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa
 	}
 
 	// Validate access authentication.
-	if !slices.Contains(responses.AuthReplies, access) {
+	if !slices.Contains(AuthReplies, access) {
 		return "", "", fmt.Errorf("invalid access authentication key: %v", access)
 	}
 
@@ -160,7 +173,7 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa
 	}
 
 	switch access {
-	case responses.AuthGranted:
+	case AuthGranted:
 		rawUserInfo, err := unmarshalAndGetKey(data, "userinfo")
 		if err != nil {
 			return "", "", err
@@ -177,12 +190,12 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa
 		}
 		data = string(d)
 
-	case responses.AuthDenied, responses.AuthRetry:
+	case AuthDenied, AuthRetry:
 		if _, err := unmarshalAndGetKey(data, "message"); err != nil {
 			return "", "", err
 		}
 
-	case responses.AuthCancelled, responses.AuthNext:
+	case AuthCancelled, AuthNext:
 		if data != "{}" {
 			return "", "", fmt.Errorf("access mode %q should not return any data, got: %v", access, data)
 		}

internal/brokers/broker_test.go

@@ -11,9 +11,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd/internal/brokers"
-	"github.com/ubuntu/authd/internal/brokers/responses"
 	"github.com/ubuntu/authd/internal/testutils"
-	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 )
 
 var supportedLayouts = map[string]map[string]string{
@@ -288,8 +286,8 @@ func TestCancelIsAuthenticated(t *testing.T) {
 
 		wantAnswer string
 	}{
-		"Successfully cancels IsAuthenticated": {sessionID: "IA_wait", wantAnswer: responses.AuthCancelled},
-		"Call returns denied if not cancelled": {sessionID: "IA_timeout", wantAnswer: responses.AuthDenied},
+		"Successfully cancels IsAuthenticated": {sessionID: "IA_wait", wantAnswer: brokers.AuthCancelled},
+		"Call returns denied if not cancelled": {sessionID: "IA_timeout", wantAnswer: brokers.AuthDenied},
 	}
 	for name, tc := range tests {
 		tc := tc
@@ -326,7 +324,7 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke
 		brokerName = strings.ReplaceAll(t.Name(), "/", "_")
 	}
 
-	cfgPath, cleanup, err := brokertestutils.StartBusBrokerMock(cfgDir, brokerName)
+	cfgPath, cleanup, err := testutils.StartBusBrokerMock(cfgDir, brokerName)
 	require.NoError(t, err, "Setup: could not start bus broker mock")
 	t.Cleanup(cleanup)
 
@@ -343,5 +341,5 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke
 // prefixID is a helper function that prefixes the given ID with the test name to avoid conflicts.
 func prefixID(t *testing.T, id string) string {
 	t.Helper()
-	return t.Name() + brokertestutils.IDSeparator + id
+	return t.Name() + testutils.IDSeparator + id
 }

internal/brokers/manager_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd/internal/brokers"
 	"github.com/ubuntu/authd/internal/testutils"
-	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 )
 
 var (
@@ -337,13 +336,13 @@ func TestStartAndEndSession(t *testing.T) {
 	require.NoError(t, *firstErr, "First NewSession should not return an error, but did")
 	require.NoError(t, *secondErr, "Second NewSession should not return an error, but did")
 
-	require.Equal(t, b1.ID+"-"+brokertestutils.GenerateSessionID("user1"),
+	require.Equal(t, b1.ID+"-"+testutils.GenerateSessionID("user1"),
 		*firstID, "First NewSession should return the expected session ID, but did not")
-	require.Equal(t, brokertestutils.GenerateEncryptionKey(b1.Name),
+	require.Equal(t, testutils.GenerateEncryptionKey(b1.Name),
 		*firstKey, "First NewSession should return the expected encryption key, but did not")
-	require.Equal(t, b2.ID+"-"+brokertestutils.GenerateSessionID("user2"),
+	require.Equal(t, b2.ID+"-"+testutils.GenerateSessionID("user2"),
 		*secondID, "Second NewSession should return the expected session ID, but did not")
-	require.Equal(t, brokertestutils.GenerateEncryptionKey(b2.Name),
+	require.Equal(t, testutils.GenerateEncryptionKey(b2.Name),
 		*secondKey, "Second NewSession should return the expected encryption key, but did not")
 
 	assignedBroker, err := m.BrokerFromSessionID(*firstID)

internal/brokers/withexamples.go

@@ -7,7 +7,7 @@ import (
 	"os"
 	"time"
 
-	"github.com/ubuntu/authd/internal/brokers/examplebroker"
+	"github.com/ubuntu/authd/examplebroker"
 	"github.com/ubuntu/authd/internal/log"
 	"github.com/ubuntu/authd/internal/testutils"
 )

internal/services/pam/pam.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/ubuntu/authd"
 	"github.com/ubuntu/authd/internal/brokers"
-	"github.com/ubuntu/authd/internal/brokers/responses"
 	"github.com/ubuntu/authd/internal/log"
 	"github.com/ubuntu/authd/internal/users"
 	"github.com/ubuntu/decorate"
@@ -209,7 +208,7 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res
 	}
 
 	// Update database and local groups on granted auth.
-	if access == responses.AuthGranted {
+	if access == brokers.AuthGranted {
 		var user users.UserInfo
 		if err := json.Unmarshal([]byte(data), &user); err != nil {
 			return nil, fmt.Errorf("user data from broker invalid: %v", err)

internal/services/pam/pam_test.go

@@ -17,7 +17,6 @@ import (
 	"github.com/ubuntu/authd/internal/brokers"
 	"github.com/ubuntu/authd/internal/services/pam"
 	"github.com/ubuntu/authd/internal/testutils"
-	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 	"github.com/ubuntu/authd/internal/users"
 	cachetests "github.com/ubuntu/authd/internal/users/cache/tests"
 	grouptests "github.com/ubuntu/authd/internal/users/localgroups/tests"
@@ -161,7 +160,7 @@ func TestSelectBroker(t *testing.T) {
 			}
 
 			if tc.username != "" {
-				tc.username = t.Name() + brokertestutils.IDSeparator + tc.username
+				tc.username = t.Name() + testutils.IDSeparator + tc.username
 			}
 
 			var sessionMode authd.SessionMode
@@ -604,7 +603,7 @@ func initBrokers() (brokerConfigPath string, cleanup func(), err error) {
 		_ = os.RemoveAll(tmpDir)
 		return "", nil, err
 	}
-	_, brokerCleanup, err := brokertestutils.StartBusBrokerMock(brokersConfPath, "BrokerMock")
+	_, brokerCleanup, err := testutils.StartBusBrokerMock(brokersConfPath, "BrokerMock")
 	if err != nil {
 		_ = os.RemoveAll(tmpDir)
 		return "", nil, err
@@ -673,7 +672,7 @@ func startSession(t *testing.T, client authd.PAMClient, username string) string
 	t.Helper()
 
 	// Prefixes the username to avoid concurrency issues.
-	username = t.Name() + brokertestutils.IDSeparator + username
+	username = t.Name() + testutils.IDSeparator + username
 
 	sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{
 		BrokerId: mockBrokerGeneratedID,