pam: Don't offer existing users to choose a different provider - #1709
pam: Don't offer existing users to choose a different provider#1709adombeck wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the PAM UI flow to stop offering broker/provider re-selection for users who are already persistently bound to a non-local broker (per the post-#1546 behavior), by introducing a brokerBoundForUser internal event and using it to adjust back-navigation logic and GDM signaling.
Changes:
- Add
brokerBoundForUserevent emitted byAutoSelectForUserwhenGetBrokerreturns a non-local broker (indicating a DB binding). - Update UI/native stage back-navigation to skip
Stage_brokerSelectionwhen the user is bound, and update the GDM adapter to emit the expectedBrokerSelectedevent. - Update PAM dummy client behavior and refresh an integration-test golden file to reflect the new “go back” label.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pam/internal/pam_test/pam-client-dummy.go | Adjust dummy GetBroker to return an empty response when no broker is known. |
| pam/internal/pam_test/pam-client-dummy_test.go | Minor formatting update for TestGetBroker expectations. |
| pam/internal/adapter/nativemodel.go | Track “user bound to broker” state to alter back-navigation in native flow. |
| pam/internal/adapter/model.go | Track “user bound to broker” state in orchestrator to alter back-navigation and propagate bound event. |
| pam/internal/adapter/gdmmodel.go | Emit BrokerSelected to GDM when a bound broker is detected. |
| pam/internal/adapter/brokerselection.go | Introduce brokerBoundForUser, refactor broker selection handling, and emit the new event from AutoSelectForUser. |
| pam/integration-tests/testdata/golden/TestCLIChangeAuthTok/Retry_if_new_password_is_rejected_by_broker | Update golden output to reflect new “go back to user selection” label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1709 +/- ##
==========================================
- Coverage 88.00% 87.96% -0.04%
==========================================
Files 96 96
Lines 7009 7030 +21
Branches 112 112
==========================================
+ Hits 6168 6184 +16
- Misses 785 790 +5
Partials 56 56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a8ff6b3 to
e420cfe
Compare
|
e2e-test results look good |
nooreldeenmansour
left a comment
There was a problem hiding this comment.
For a logged-in user, that doesn't seem to work properly on the GDM though. I am able to view the brokers selection list for an existing user by going back
It does work as expected on SSH though
That patch (tests included) seems to work, diff --git a/pam/internal/adapter/gdmmodel_test.go b/pam/internal/adapter/gdmmodel_test.go
index d7d88c841..d571c2604 100644
--- a/pam/internal/adapter/gdmmodel_test.go
+++ b/pam/internal/adapter/gdmmodel_test.go
@@ -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),
@@ -874,9 +874,6 @@ func TestGdmModel(t *testing.T) {
events: []*gdm.EventData{
gdm_test.ChangeStageEvent(proto.Stage_brokerSelection),
},
- commands: []tea.Cmd{
- sendEvent(gdmTestWaitForStage{stage: proto.Stage_brokerSelection}),
- },
}),
},
},
@@ -884,10 +881,9 @@ func TestGdmModel(t *testing.T) {
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,
@@ -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": {
diff --git a/pam/internal/adapter/model.go b/pam/internal/adapter/model.go
index 09498c54e..3b97b7eaf 100644
--- a/pam/internal/adapter/model.go
+++ b/pam/internal/adapter/model.go
@@ -347,6 +347,9 @@ func (m uiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
case ChangeStage:
safeMessageDebug(msg)
+ if m.userIsBoundToBroker && msg.Stage == proto.Stage_brokerSelection {
+ msg.Stage = proto.Stage_userSelection
+ }
return m, m.changeStage(msg.Stage)
case StageChanged:With it, for a GDM logged-in user, going back shows the flows, and going back one more time skips the providers and shows the users list |
e420cfe to
7420bae
Compare
Oh, I had an unpushed commit locally which already does that. Pushed it now |
|
Waiting for the CI to pass before requesting another review |
| // brokerBoundForUser 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 brokerBoundForUser struct { | ||
| brokerID string | ||
| } | ||
|
|
There was a problem hiding this comment.
Shouldn't it be brokerBoundToUser?
Since #1546, users are bound to the broker (identity provider) they first authenticated with. Selecting a different broker would be rejected by authd via SelectBroker. Yet the UI still offered to "go back to choose the provider" whenever multiple brokers were available — including for users who already have a binding. GetBroker only ever returns a non-local broker when the user has a persistent binding in the database (or the in-memory cache populated from the DB). Add a brokerBoundToUser event that AutoSelectForUser emits in that case to suppress broker re-selection in the UI. When brokerBoundToUser is received, both uiModel (interactive terminal) and nativeModel (SSH/native) set userIsBoundToBroker, which causes previousStage() to skip Stage_brokerSelection and return Stage_userSelection instead. The GDM model also handles the event to emit the expected BrokerSelected GDM event. Result: existing users see "go back to user selection" instead of "go back to choose the provider". New users who select a broker manually are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
With "pam: Don't offer existing users to choose a different provider", existing users bypass the "Select your provider:" screen entirely. Pressing Escape from "Select the authentication flow:" no longer navigates back to provider selection; the user is stuck there. Update Try Navigating Back to User Selection to assert the new stuck point: after two Escape presses the screen still shows "Select the authentication flow:", proving the username cannot be changed via su. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n GDM
In GDM mode, stage navigation is driven by gnome-shell rather than by
the PAM module's keyboard handler. When the user clicks back, GDM emits
a StageChanged event directly requesting Stage_brokerSelection. The
ChangeStage handler in uiModel previously forwarded this verbatim,
bypassing the userIsBoundToBroker guard that already protected the ESC
key path in previousStage().
Intercept ChangeStage{brokerSelection} in uiModel.Update and redirect it
to Stage_userSelection when userIsBoundToBroker is set. The GDM model
then sends a changeStage request back to GDM to update its own stage to
userSelection, so both sides stay consistent.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7420bae to
edb5332
Compare
Since #1546, users are bound to the broker (identity provider) they first authenticated with. Selecting a different broker would be rejected by authd via
SelectBroker. Yet the UI still offered to "go back to choose the provider" whenever multiple brokers were available — including for users who already have a binding.What changed
GetBrokeronly ever returns a non-local broker when the user has a persistent binding in the database (or the in-memory cache populated from the DB). A newbrokerBoundForUserevent is added thatAutoSelectForUseremits in that case to suppress broker re-selection in the UI.When
brokerBoundForUseris received, bothuiModel(interactive terminal) andnativeModel(SSH/native) setuserIsBoundToBroker, which causespreviousStage()to skipStage_brokerSelectionand returnStage_userSelectioninstead. The GDM model also handles the event to emit the expectedBrokerSelectedGDM event.Result
Existing users don't see "go back to choose the provider" anymore. New users who select a broker manually are unaffected.
Closes #1667
UDENG-10923