Skip to content

Commit

Permalink
pam/model: Initialize user only after we've got the brokers list
Browse files Browse the repository at this point in the history
One of the races we were facing (mostly in native model) is that we
could end up in a situation in which we receive an authd error on
getting the brokers list *after* that the user selection view is already
shown, leading to racy golden files ordering.

This is because what it could happen is:
 - We start in parallel:
   -> User selection
   -> UI Layouts -> Available brokers fetching

Now let's "imagine" that the brokers fetching fails (as when we've not
the permissions to read it, as for being not root).

If the user selection result arrives first, then we end up showing the
user selection UI (that is blocking in the native model) and only then
we handle the AvailableBrokers result (the error).

If the broker selection arrives first, we show first the error instead.

Now, this kind of approach made sense when we had no errors during this
phases, to get the user name as early as possible, but since we're now
doing various permissions checks in the broker, it's better to always go
in order instead of performing such operations in parallel, so now:
 - UI Layouts -> Available brokers fetching
 - User selection starts

Blocking as soon as we've a failure, as highlighted by the new golden
files.
  • Loading branch information
3v1n0 committed Nov 8, 2024
1 parent be4916e commit 9d19a42
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 12 deletions.
Original file line number Diff line number Diff line change
@@ -1,23 +1,29 @@
> ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM Authenticate() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
────────────────────────────────────────────────────────────────────────────────
> ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM Authenticate() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
>
────────────────────────────────────────────────────────────────────────────────
> ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM Authenticate() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
>
────────────────────────────────────────────────────────────────────────────────
Original file line number Diff line number Diff line change
@@ -1,23 +1,29 @@
> ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM ChangeAuthTok() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
────────────────────────────────────────────────────────────────────────────────
> ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM ChangeAuthTok() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
>
────────────────────────────────────────────────────────────────────────────────
> ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK} force_native_client=true
Username:
PAM Error Message: could not get current available brokers: permission denied: this action is on
ly allowed for root users. Current user is XXXX
PAM ChangeAuthTok() for user "" exited with error (PAM exit code: 4): System error
acct=incomplete
PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
dispatch
>
>
────────────────────────────────────────────────────────────────────────────────
2 changes: 1 addition & 1 deletion pam/internal/adapter/brokerselection.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (m brokerSelectionModel) Update(msg tea.Msg) (brokerSelectionModel, tea.Cmd
}
var cmds []tea.Cmd
cmds = append(cmds, m.SetItems(allBrokers))
cmds = append(cmds, sendEvent(UsernameOrBrokerListReceived{}))
cmds = append(cmds, sendEvent(BrokerListReceived{}))

return m, tea.Batch(cmds...)

Expand Down
16 changes: 13 additions & 3 deletions pam/internal/adapter/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ type UIModel struct {

/* global events */

// UsernameOrBrokerListReceived is received either when the user name is filled (pam or manually) and we got the broker list.
type UsernameOrBrokerListReceived struct{}
// BrokerListReceived is received when we got the broker list.
type BrokerListReceived struct{}

// UsernameAndBrokerListReceived is received either when the user name is filled (pam or manually) and we got the broker list.
type UsernameAndBrokerListReceived struct{}

// BrokerSelected signifies that the broker has been chosen.
type BrokerSelected struct {
Expand Down Expand Up @@ -170,7 +173,14 @@ func (m *UIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
return m, m.quit()

// Events
case UsernameOrBrokerListReceived:
case BrokerListReceived:
log.Debugf(context.TODO(), "%#v, brokers: %#v", msg, m.availableBrokers())
if m.availableBrokers() == nil {
return m, nil
}
return m, m.userSelectionModel.SelectUser()

case UsernameAndBrokerListReceived:
log.Debugf(context.TODO(), "%#v, user: %q, brokers: %#v", msg, m.username(), m.availableBrokers())
if m.username() == "" {
return m, nil
Expand Down
9 changes: 7 additions & 2 deletions pam/internal/adapter/userselection.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,13 @@ func newUserSelectionModel(pamMTx pam.ModuleTransaction, clientType PamClientTyp
}
}

// Init initializes userSelectionModel, by getting it from PAM if prefilled.
// Init initializes userSelectionModel.
func (m *userSelectionModel) Init() tea.Cmd {
return nil
}

// SelectUser selects the user name to be used, by getting it from PAM if prefilled.
func (m userSelectionModel) SelectUser() tea.Cmd {
pamUser, err := m.pamMTx.GetItem(pam.User)
if cmd := maybeSendPamError(err); cmd != nil {
return cmd
Expand Down Expand Up @@ -98,7 +103,7 @@ func (m userSelectionModel) Update(msg tea.Msg) (userSelectionModel, tea.Cmd) {
if !m.selected {
return m, nil
}
return m, sendEvent(UsernameOrBrokerListReceived{})
return m, sendEvent(UsernameAndBrokerListReceived{})

case userRequired:
log.Debugf(context.TODO(), "%#v", msg)
Expand Down

0 comments on commit 9d19a42

Please sign in to comment.