Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions e2e-tests/resources/broker.resource
Original file line number Diff line number Diff line change
Expand Up @@ -173,22 +173,20 @@ Check That Username Cannot Be Changed When Using su


Try Navigating Back to User Selection
# First screen is the autoselected local password mode
# First screen is the autoselected local password flow
Match Text regex:Enter\\s*your\\s*(?:local\\s*)?password: 120
Hid.Keys Combo Escape

# The previous screen should be the authentication mode selection
# The previous screen should be the authentication flow selection
Match Text Select the authentication flow: 120
Hid.Keys Combo Escape

# The previous screen should be the broker selection
Match Text Select your provider: 120
Hid.Keys Combo Escape

# The previous screen is the username prompt, but it shouldn't be possible to get there
Match Text Select your provider: 120
# For existing users the provider is already known, so the provider-selection
# screen is skipped. Pressing Escape should not navigate back to user or
# provider selection; the user is stuck at the authentication-flow screen.
Match Text Select the authentication flow: 120
Hid.Keys Combo Escape
Match Text Select your provider: 120
Match Text Select the authentication flow: 120


Log In With Remote User Through CLI: Local Password And Expect Failure
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ Enter your new password
New password:
>

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

New password:
> *********

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

Expand All @@ -126,7 +126,7 @@ New password:
Confirm password:
>

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

Expand All @@ -135,23 +135,23 @@ New password:
Confirm password:
> *********

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

New password:
>
new password does not match criteria: must be 'goodpass'

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

New password:
> ********
new password does not match criteria: must be 'goodpass'

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

Expand All @@ -160,7 +160,7 @@ New password:
Confirm password:
>

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
Enter your new password

Expand All @@ -169,7 +169,7 @@ New password:
Confirm password:
> ********

Press escape key to go back to choose the provider
Press escape key to go back to user selection
────────────────────────────────────────────────────────────────────────────────
PAM_AUTHTOK: "goodpass"
PAM_OLDAUTHTOK: "authd2404"
Expand Down
61 changes: 42 additions & 19 deletions pam/internal/adapter/brokerselection.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ type brokerSelected struct {
// brokerSelectionRequired is the internal event that a broker needs to be selected.
type brokerSelectionRequired struct{}

// brokerBoundToUser is the internal event that the user is already bound to a
// specific broker (i.e. they have a persistent broker binding in the database).
// This prevents the UI from offering to "go back" to broker selection because
// switching to a different broker would be rejected by authd.
type brokerBoundToUser struct {
brokerID string
}

// selectBroker selects a given broker.
func selectBroker(brokerID string) tea.Cmd {
return func() tea.Msg {
Expand Down Expand Up @@ -99,33 +107,42 @@ func (m brokerSelectionModel) Update(msg tea.Msg) (brokerSelectionModel, tea.Cmd
broker := convertTo[brokerItem](msg.item)
return m, selectBroker(broker.id)

case brokerSelected:
case brokerBoundToUser:
safeMessageDebug(msg)
broker := brokerFromID(msg.brokerID, m.availableBrokers)
if broker == nil {
log.Infof(context.TODO(), "broker %q is not part of current active brokers", msg.brokerID)
return m, nil
}
// Select correct line to ensure model is synchronised
for i, b := range m.Items() {
b := convertTo[brokerItem](b)
if b.id != broker.Id {
continue
}
m.Select(i)
}
return m.handleBrokerSelected(msg.brokerID)

return m, sendEvent(BrokerSelected{
BrokerID: broker.Id,
})
case brokerSelected:
safeMessageDebug(msg)
return m.handleBrokerSelected(msg.brokerID)
}

var cmd tea.Cmd
m.List, cmd = m.List.Update(msg)
return m, cmd
}

// AutoSelectForUser requests if any broker was used by this user to automatically selects it.
// handleBrokerSelected finds the broker by ID and emits BrokerSelected.
func (m brokerSelectionModel) handleBrokerSelected(brokerID string) (brokerSelectionModel, tea.Cmd) {
broker := brokerFromID(brokerID, m.availableBrokers)
if broker == nil {
log.Infof(context.TODO(), "broker %q is not part of current active brokers", brokerID)
return m, nil
}
// Select correct line to ensure model is synchronised
for i, b := range m.Items() {
b := convertTo[brokerItem](b)
if b.id != broker.Id {
continue
}
m.Select(i)
}

return m, sendEvent(BrokerSelected{
BrokerID: broker.Id,
})
}

// AutoSelectForUser requests if any broker was used by this user to automatically select it.
func AutoSelectForUser(client authd.PAMClient, username string) tea.Cmd {
return func() tea.Msg {
r, err := client.GetBroker(context.TODO(),
Expand All @@ -134,14 +151,20 @@ func AutoSelectForUser(client authd.PAMClient, username string) tea.Cmd {
})
// We keep a chance to manually select the broker, not a blocker issue.
if err != nil {
log.Infof(context.TODO(), "can't get broker for %q", username)
log.Infof(context.TODO(), "can't get broker for %q", username)
return brokerSelectionRequired{}
}
Comment thread
adombeck marked this conversation as resolved.
brokerID := r.GetBroker()
if brokerID == "" {
return brokerSelectionRequired{}
}

if brokerID != brokers.LocalBrokerName {
// Any non-local broker from GetBroker comes from a DB binding — the
// user is bound and must not be offered re-selection.
return brokerBoundToUser{brokerID: brokerID}
}

return selectBroker(brokerID)()
}
}
Expand Down
5 changes: 5 additions & 0 deletions pam/internal/adapter/gdmmodel.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ func (m gdmModel) Update(msg tea.Msg) (gdmModel, tea.Cmd) {
BrokersReceived: &gdm.Events_BrokersReceived{BrokersInfos: msg.brokers},
})

case brokerBoundToUser:
return m, m.emitEvent(&gdm.EventData_BrokerSelected{
BrokerSelected: &gdm.Events_BrokerSelected{BrokerId: msg.brokerID},
})

case brokerSelected:
return m, m.emitEvent(&gdm.EventData_BrokerSelected{
BrokerSelected: &gdm.Events_BrokerSelected{BrokerId: msg.brokerID},
Expand Down
10 changes: 3 additions & 7 deletions pam/internal/adapter/gdmmodel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ func TestGdmModel(t *testing.T) {
wantStage: proto.Stage_challenge,
wantPAMReturnValue: gdmTestEarlyStopReturnValue,
},
"Implicitly_cancelled_for_a_waiting_auth_mode_with_preset_PAM_user_and_server_side_broker_and_authMode_selection": {
"Back_to_broker_selection_skips_to_user_selection_for_a_waiting_auth_mode_with_preset_PAM_user_and_server_side_broker_and_authMode_selection": {
clientOptions: append(slices.Clone(multiBrokerClientOptions),
pam_test.WithGetBrokerReturn(firstBrokerInfo.Id, nil),
pam_test.WithIsAuthenticatedWantWait(time.Millisecond*1500),
Expand All @@ -874,20 +874,16 @@ func TestGdmModel(t *testing.T) {
events: []*gdm.EventData{
gdm_test.ChangeStageEvent(proto.Stage_brokerSelection),
},
commands: []tea.Cmd{
sendEvent(gdmTestWaitForStage{stage: proto.Stage_brokerSelection}),
},
}),
},
},
},
wantSelectedBroker: firstBrokerInfo.Id,
wantGdmRequests: []gdm.RequestType{
gdm.RequestType_uiLayoutCapabilities,
gdm.RequestType_changeStage, // -> broker Selection
gdm.RequestType_changeStage, // -> authMode Selection
gdm.RequestType_changeStage, // -> form with wait
gdm.RequestType_changeStage, // -> broker selection
gdm.RequestType_changeStage, // -> user selection
},
wantGdmEvents: []gdm.EventType{
gdm.EventType_userSelected,
Expand All @@ -899,7 +895,7 @@ func TestGdmModel(t *testing.T) {
gdm.EventType_authEvent,
},
wantGdmAuthRes: []*authd.IAResponse{{Access: auth.Cancelled}},
wantStage: proto.Stage_brokerSelection,
wantStage: proto.Stage_userSelection,
wantPAMReturnValue: gdmTestEarlyStopReturnValue,
},
"Authenticated_with_preset_PAM_user_and_server_side_broker_and_authMode_selection_and_after_various_retries": {
Expand Down
20 changes: 19 additions & 1 deletion pam/internal/adapter/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type uiModel struct {
client authd.PAMClient

sessionStartingForBroker string
userIsBoundToBroker bool
currentSession *sessionInfo

healthCheckCancel func()
Expand Down Expand Up @@ -287,9 +288,21 @@ func (m uiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
return m, nil
}

m.userIsBoundToBroker = false
// Got user and brokers? Time to auto or manually select.
return m, AutoSelectForUser(m.client, m.username())

case brokerBoundToUser:
safeMessageDebug(msg)
m.userIsBoundToBroker = true
// Propagate to sub-models (brokerSelectionModel needs to emit BrokerSelected).
var cmd tea.Cmd
var cmds tea.BatchMsg
m.brokerSelectionModel, cmd = m.brokerSelectionModel.Update(msg)
cmds = append(cmds, cmd)
cmds = append(cmds, m.updateClientModel(msg))
return m, tea.Batch(cmds...)

case BrokerSelected:
safeMessageDebug(msg)
if m.sessionStartingForBroker == "" {
Expand Down Expand Up @@ -334,6 +347,11 @@ func (m uiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {

case ChangeStage:
safeMessageDebug(msg)
// If the user is bound to a broker, skip broker selection when navigating
// back (e.g. GDM sends StageChanged{brokerSelection} on back-navigation).
if msg.Stage == proto.Stage_brokerSelection && m.userIsBoundToBroker {
msg.Stage = proto.Stage_userSelection
}
return m, m.changeStage(msg.Stage)

case StageChanged:
Expand Down Expand Up @@ -554,7 +572,7 @@ func (m uiModel) previousStage() proto.Stage {
if currentStage > proto.Stage_authModeSelection && len(m.availableAuthModes()) > 1 {
return proto.Stage_authModeSelection
}
if currentStage > proto.Stage_brokerSelection && len(m.availableBrokers()) > 1 {
if !m.userIsBoundToBroker && currentStage > proto.Stage_brokerSelection && len(m.availableBrokers()) > 1 {
return proto.Stage_brokerSelection
}
return proto.Stage_userSelection
Expand Down
7 changes: 6 additions & 1 deletion pam/internal/adapter/nativemodel.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type nativeModel struct {
currentStage proto.Stage
busy bool
userSelectionAllowed bool
userIsBoundToBroker bool
}

const (
Expand Down Expand Up @@ -200,6 +201,7 @@ func (m nativeModel) Update(msg tea.Msg) (nativeModel, tea.Cmd) {
return m, cmd
}

m.userIsBoundToBroker = false
return m.startAsyncOp(m.userSelection)

case brokersListReceived:
Expand All @@ -218,6 +220,9 @@ func (m nativeModel) Update(msg tea.Msg) (nativeModel, tea.Cmd) {
case authModesReceived:
m.authModes = msg.authModes

case brokerBoundToUser:
m.userIsBoundToBroker = true

case brokerSelectionRequired:
if m.busy {
// We may receive multiple concurrent requests, but due to the sync nature
Expand Down Expand Up @@ -895,7 +900,7 @@ func (m nativeModel) previousStage() proto.Stage {
if m.currentStage > proto.Stage_authModeSelection && len(m.authModes) > 1 {
return proto.Stage_authModeSelection
}
if m.currentStage > proto.Stage_brokerSelection && len(m.availableBrokers) > 1 {
if !m.userIsBoundToBroker && m.currentStage > proto.Stage_brokerSelection && len(m.availableBrokers) > 1 {
return proto.Stage_brokerSelection
}
return proto.Stage_userSelection
Expand Down
5 changes: 4 additions & 1 deletion pam/internal/pam_test/pam-client-dummy.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,10 @@ func (dc *DummyClient) GetBroker(ctx context.Context, in *authd.GBRequest, opts
return nil, errors.New("no username provided")
}
brokerID := dc.brokerForUser[in.Username]
return &authd.GBResponse{Broker: brokerID}, nil
if brokerID != "" {
return &authd.GBResponse{Broker: brokerID}, nil
}
return &authd.GBResponse{}, nil
}

// SelectBroker simulates SelectBroker using the provided parameters.
Expand Down
6 changes: 4 additions & 2 deletions pam/internal/pam_test/pam-client-dummy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,10 @@ func TestGetBroker(t *testing.T) {
WithBrokerForUser("user0", "broker0"),
WithBrokerForUser("user1@example.com", "broker1"),
),
args: &authd.GBRequest{Username: "user1@example.com"},
wantRet: &authd.GBResponse{Broker: "broker1"},
args: &authd.GBRequest{Username: "user1@example.com"},
wantRet: &authd.GBResponse{
Broker: "broker1",
},
},

// Error cases
Expand Down
Loading