Addressing comments pt 2

internal/brokers/broker.go

@@ -93,14 +93,10 @@ func (b Broker) newSession(ctx context.Context, username, lang string) (sessionI
 }
 
 // GetAuthenticationModes calls the broker corresponding method, stripping broker ID prefix from sessionID.
-func (b Broker) GetAuthenticationModes(ctx context.Context, sessionID string, supportedUILayouts []map[string]string) (authenticationModes []map[string]string, err error) {
+func (b *Broker) GetAuthenticationModes(ctx context.Context, sessionID string, supportedUILayouts []map[string]string) (authenticationModes []map[string]string, err error) {
 	sessionID = b.parseSessionID(sessionID)
 
-	b.layoutValidators, err = generateValidators(supportedUILayouts)
-	if err != nil {
-		return nil, fmt.Errorf("could not generate layout validators: %w", err)
-	}
-
+	b.layoutValidators = generateValidators(ctx, supportedUILayouts)
 	authenticationModes, err = b.brokerer.GetAuthenticationModes(ctx, sessionID, supportedUILayouts)
 	if err != nil {
 		return nil, err
@@ -191,11 +187,12 @@ func (b Broker) cancelIsAuthorized(ctx context.Context, sessionID string) {
 //	        }
 //	    }
 //	}
-func generateValidators(supportedUILayouts []map[string]string) (map[string]map[string]fieldValidator, error) {
+func generateValidators(ctx context.Context, supportedUILayouts []map[string]string) map[string]map[string]fieldValidator {
 	validators := make(map[string]map[string]fieldValidator)
 	for _, layout := range supportedUILayouts {
 		if _, exists := layout["type"]; !exists {
-			return nil, fmt.Errorf("Supported UI layouts are invalid")
+			log.Errorf(ctx, "layout %v provided with missing type, it will be ignored", layout)
+			continue
 		}
 
 		layoutValidator := make(map[string]fieldValidator)
@@ -216,7 +213,7 @@ func generateValidators(supportedUILayouts []map[string]string) (map[string]map[
 		}
 		validators[layout["type"]] = layoutValidator
 	}
-	return validators, nil
+	return validators
 }
 
 // validateUILayout validates the layout fields and content according to the broker validators and returns the layout

internal/brokers/broker_test.go

@@ -2,6 +2,7 @@ package brokers_test
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"path/filepath"
 	"strings"
@@ -14,26 +15,21 @@ import (
 	"github.com/ubuntu/authd/internal/testutils"
 )
 
-var supportedLayouts = []map[string]string{
-	// form layout
-	{
-		"type":  "form",
-		"label": "required",
-		"entry": "optional:chars,chars_password",
-		"wait":  "optional:true,false",
+var supportedLayouts = map[string]map[string]string{
+	"required-value": {
+		"type":  "required-value",
+		"value": "required:value_type, other_value_type",
 	},
-	// qrcode layout
-	{
-		"type":    "qrcode",
-		"label":   "optional",
-		"content": "required",
-		"wait":    "required:true,false",
+	"optional-value": {
+		"type":  "optional-value",
+		"value": "optional:value_type, other_value_type",
 	},
-	// newpassword layout
-	{
-		"type":  "newpassword",
-		"label": "required",
-		"entry": "required:chars,chars_password",
+	"missing-type": {
+		"value": "required:missing_type",
+	},
+	"misconfigured-layout": {
+		"type":  "misconfigured-layout",
+		"value": "required-but-misformatted",
 	},
 }
 
@@ -94,11 +90,14 @@ func TestGetAuthenticationModes(t *testing.T) {
 	t.Parallel()
 
 	tests := map[string]struct {
-		sessionID string
+		sessionID          string
+		supportedUILayouts []string
 
 		wantErr bool
 	}{
-		"Successfully get authentication modes":                        {sessionID: "success"},
+		"Get authentication modes and generate validators":       {sessionID: "success", supportedUILayouts: []string{"required-value", "optional-value"}},
+		"Get authentication modes and ignores invalid UI layout": {sessionID: "success", supportedUILayouts: []string{"required-value", "missing-type"}},
+
 		"Does not error out when no authentication modes are returned": {sessionID: "GAM_empty"},
 
 		// broker errors
@@ -112,15 +111,30 @@ func TestGetAuthenticationModes(t *testing.T) {
 
 			b, _ := newBrokerForTests(t)
 
-			gotModes, err := b.GetAuthenticationModes(context.Background(), tc.sessionID, nil)
+			if tc.supportedUILayouts == nil {
+				tc.supportedUILayouts = []string{"required-value"}
+			}
+
+			// This is normally done in the broker's GetAuthenticationModes method, but we need to do it here to test the SelectAuthenticationMode method.
+			var supportedUILayouts []map[string]string
+			for _, layout := range tc.supportedUILayouts {
+				supportedUILayouts = append(supportedUILayouts, supportedLayouts[layout])
+			}
+			t.Logf("Supported UI layouts: %v", supportedUILayouts)
+
+			gotModes, err := b.GetAuthenticationModes(context.Background(), tc.sessionID, supportedUILayouts)
 			if tc.wantErr {
 				require.Error(t, err, "GetAuthenticationModes should return an error, but did not")
 				return
 			}
 			require.NoError(t, err, "GetAuthenticationModes should not return an error, but did")
 
-			wantModes := testutils.LoadWithUpdateFromGoldenYAML(t, gotModes)
-			require.Equal(t, wantModes, gotModes, "GetAuthenticationModes should return the expected modes, but did not")
+			modesStr, err := json.Marshal(gotModes)
+			require.NoError(t, err, "Post: error when marshaling result")
+
+			got := "MODES:\n" + string(modesStr) + "\n\nVALIDATORS:\n" + b.LayoutValidatorsString()
+			want := testutils.LoadWithUpdateFromGolden(t, got)
+			require.Equal(t, want, got, "GetAuthenticationModes should return the expected modes, but did not")
 		})
 	}
 }
@@ -129,35 +143,25 @@ func TestSelectAuthenticationMode(t *testing.T) {
 	t.Parallel()
 
 	tests := map[string]struct {
-		sessionID string
+		sessionID          string
+		supportedUILayouts []string
 
 		wantErr bool
 	}{
-		"Successfully select form authentication mode":        {sessionID: "SAM_form_success"},
-		"Successfully select qrcode authentication mode":      {sessionID: "SAM_qrcode_success"},
-		"Successfully select newpassword authentication mode": {sessionID: "SAM_newpassword_success"},
+		"Successfully select mode with required value":         {sessionID: "SAM_success_required_value"},
+		"Successfully select mode with optional value":         {sessionID: "SAM_success_optional_value", supportedUILayouts: []string{"optional-value"}},
+		"Successfully select mode with missing optional value": {sessionID: "SAM_missing_optional_value", supportedUILayouts: []string{"optional-value"}},
 
 		// broker errors
-		"Error when selecting authentication mode": {sessionID: "SAM_error", wantErr: true},
+		"Error when selecting invalid auth mode": {sessionID: "SAM_error", wantErr: true},
 
 		/* Layout errors */
-
-		// Layout type errors
-		"Error when broker returns no layout":           {sessionID: "SAM_no_layout", wantErr: true},
-		"Error when broker returns invalid layout type": {sessionID: "SAM_invalid_layout_type", wantErr: true},
-
-		// Type "form" errors
-		"Error when broker returns form with no label":      {sessionID: "SAM_form_no_label", wantErr: true},
-		"Error when broker returns form with invalid entry": {sessionID: "SAM_form_invalid_entry", wantErr: true},
-		"Error when broker returns form with invalid wait":  {sessionID: "SAM_form_invalid_wait", wantErr: true},
-
-		// Type "qrcode" errors
-		"Error when broker returns qrcode with no content":   {sessionID: "SAM_qrcode_no_content", wantErr: true},
-		"Error when broker returns qrcode with invalid wait": {sessionID: "SAM_qrcode_invalid_wait", wantErr: true},
-
-		// Type "newpassword" errors
-		"Error when broker returns newpassword with no label":      {sessionID: "SAM_newpassword_no_label", wantErr: true},
-		"Error when broker returns newpassword with invalid entry": {sessionID: "SAM_newpassword_invalid_entry", wantErr: true},
+		"Error when returns no layout":                          {sessionID: "SAM_no_layout", wantErr: true},
+		"Error when returns layout with no type":                {sessionID: "SAM_no_layout_type", wantErr: true},
+		"Error when returns layout with invalid type":           {sessionID: "SAM_invalid_layout_type", wantErr: true},
+		"Error when returns layout without required value":      {sessionID: "SAM_missing_required_value", wantErr: true},
+		"Error when returns layout with invalid required value": {sessionID: "SAM_invalid_required_value", wantErr: true},
+		"Error when returns layout with invalid optional value": {sessionID: "SAM_invalid_optional_value", wantErr: true},
 	}
 	for name, tc := range tests {
 		tc := tc
@@ -166,9 +170,16 @@ func TestSelectAuthenticationMode(t *testing.T) {
 
 			b, _ := newBrokerForTests(t)
 
+			if tc.supportedUILayouts == nil {
+				tc.supportedUILayouts = []string{"required-value"}
+			}
+
 			// This is normally done in the broker's GetAuthenticationModes method, but we need to do it here to test the SelectAuthenticationMode method.
-			err := brokers.GenerateLayoutValidators(&b, supportedLayouts)
-			require.NoError(t, err, "Setup: could not generate layout validators")
+			supportedUILayouts := make([]map[string]string, len(tc.supportedUILayouts))
+			for _, layout := range tc.supportedUILayouts {
+				supportedUILayouts = append(supportedUILayouts, supportedLayouts[layout])
+			}
+			brokers.GenerateLayoutValidators(&b, supportedUILayouts)
 
 			gotUI, err := b.SelectAuthenticationMode(context.Background(), tc.sessionID, "mode1")
 			if tc.wantErr {

internal/brokers/export_test.go

@@ -2,6 +2,7 @@ package brokers
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/godbus/dbus/v5"
 )
@@ -28,10 +29,18 @@ func (m *Manager) SetBrokerForSession(b *Broker, sessionID string) {
 }
 
 // GenerateLayoutValidators generates the layout validators and assign them to the specified broker.
-func GenerateLayoutValidators(b *Broker, supportedUILayouts []map[string]string) (err error) {
-	b.layoutValidators, err = generateValidators(supportedUILayouts)
-	if err != nil {
-		return err
+func GenerateLayoutValidators(b *Broker, supportedUILayouts []map[string]string) {
+	b.layoutValidators = generateValidators(context.Background(), supportedUILayouts)
+}
+
+func (b *Broker) LayoutValidatorsString() string {
+	var s string
+	for t, v := range b.layoutValidators {
+		layoutStr := fmt.Sprintf("\t%s:\n", t)
+		for field, validator := range v {
+			layoutStr += fmt.Sprintf("\t\t%s: { required: %v, supportedValues: %v }\n", field, validator.required, validator.supportedValues)
+		}
+		s += layoutStr
 	}
-	return nil
+	return s
 }

internal/brokers/testdata/TestGetAuthenticationModes/golden/does_not_error_out_when_no_authentication_modes_are_returned

@@ -1 +1,6 @@
+MODES:
 []
+
+VALIDATORS:
+	required-value:
+		value: { required: true, supportedValues: [value_type  other_value_type] }

internal/brokers/testdata/TestGetAuthenticationModes/golden/get_authentication_modes_and_generate_validators

@@ -0,0 +1,8 @@
+MODES:
+[{"id":"mode1","label":"Mode 1"}]
+
+VALIDATORS:
+	required-value:
+		value: { required: true, supportedValues: [value_type  other_value_type] }
+	optional-value:
+		value: { required: false, supportedValues: [value_type  other_value_type] }

internal/brokers/testdata/TestGetAuthenticationModes/golden/get_authentication_modes_and_ignores_invalid_ui_layout

@@ -0,0 +1,6 @@
+MODES:
+[{"id":"mode1","label":"Mode 1"}]
+
+VALIDATORS:
+	required-value:
+		value: { required: true, supportedValues: [value_type  other_value_type] }

internal/brokers/testdata/TestSelectAuthenticationMode/golden/successfully_select_mode_with_missing_optional_value

@@ -0,0 +1 @@
+type: optional-value

internal/brokers/testdata/TestSelectAuthenticationMode/golden/successfully_select_mode_with_optional_value

@@ -0,0 +1,2 @@
+type: optional-value
+value: value_type

internal/brokers/testdata/TestSelectAuthenticationMode/golden/successfully_select_mode_with_required_value

@@ -0,0 +1,2 @@
+type: required-value
+value: value_type

internal/testutils/broker.go

@@ -135,79 +135,45 @@ func (b *BrokerBusMock) GetAuthenticationModes(sessionID string, supportedUILayo
 // SelectAuthenticationMode returns default values to be used in tests or an error if requested.
 func (b *BrokerBusMock) SelectAuthenticationMode(sessionID, authenticationModeName string) (uiLayoutInfo map[string]string, dbusErr *dbus.Error) {
 	switch sessionID {
-	case "SAM_invalid_layout_type":
-		return map[string]string{
-			"invalid": "invalid",
-		}, nil
-	case "SAM_no_layout":
-		return nil, nil
-	case "SAM_error":
-		return nil, dbus.MakeFailedError(fmt.Errorf("Broker %q: SelectAuthenticationMode errored out", b.name))
-
-	case "SAM_form_no_label":
-		return map[string]string{
-			"type":  "form",
-			"label": "",
-			"entry": "chars_password",
-		}, nil
-	case "SAM_form_invalid_entry":
+	case "SAM_success_required_value":
 		return map[string]string{
-			"type":  "form",
-			"label": "Invalid Entry",
-			"entry": "invalid",
+			"type":  "required-value",
+			"value": "value_type",
 		}, nil
-	case "SAM_form_invalid_wait":
+	case "SAM_success_optional_value":
 		return map[string]string{
-			"type":  "form",
-			"label": "Invalid Wait",
-			"entry": "chars_password",
-			"wait":  "invalid",
+			"type":  "optional-value",
+			"value": "value_type",
 		}, nil
-	case "SAM_form_success":
+	case "SAM_missing_optional_value":
 		return map[string]string{
-			"type":  "form",
-			"label": "Success form",
-			"entry": "chars_password",
+			"type": "optional-value",
 		}, nil
-	case "SAM_qrcode_no_content":
-		return map[string]string{
-			"type":    "qrcode",
-			"content": "",
-			"wait":    "true",
-		}, nil
-	case "SAM_qrcode_invalid_wait":
-		return map[string]string{
-			"type":    "qrcode",
-			"content": "Invalid Wait",
-			"wait":    "invalid",
-		}, nil
-	case "SAM_qrcode_success":
+	case "SAM_invalid_layout_type":
 		return map[string]string{
-			"type":    "qrcode",
-			"content": "Success QRCode",
-			"wait":    "true",
+			"invalid": "invalid",
 		}, nil
-	case "SAM_newpassword_no_label":
+	case "SAM_missing_required_value":
 		return map[string]string{
-			"type":  "newpassword",
-			"label": "",
-			"entry": "chars_password",
+			"type": "required-value",
 		}, nil
-	case "SAM_newpassword_invalid_entry":
+	case "SAM_invalid_required_value":
 		return map[string]string{
-			"type":  "newpassword",
-			"label": "Invalid Entry",
-			"entry": "invalid",
+			"type":  "required-value",
+			"value": "invalid value",
 		}, nil
-	case "SAM_newpassword_success":
+	case "SAM_invalid_optional_value":
 		return map[string]string{
-			"type":  "newpassword",
-			"label": "Success newpassword",
-			"entry": "chars_password",
+			"type":  "optional-value",
+			"value": "invalid value",
 		}, nil
-	default:
-		return map[string]string{}, nil
+	case "SAM_error":
+		return nil, dbus.MakeFailedError(fmt.Errorf("Broker %q: SelectAuthenticationMode errored out", b.name))
+	case "SAM_no_layout":
+		return nil, nil
 	}
+	// Should never get here
+	return map[string]string{}, nil
 }
 
 // IsAuthorized returns default values to be used in tests or an error if requested.