fix(pam/model): Initialize user only after we've got the brokers list

[3v1n0]

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.

You can see an example of this failure in this CI job:

• https://github.com/ubuntu/authd/actions/runs/11708304677/job/32609750206?pr=619

UDENG-5161

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.99%. Comparing base (be4916e) to head (9372736).

Files with missing lines	Patch %	Lines
pam/internal/adapter/model.go	71.42%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
- Coverage   83.06%   82.99%   -0.08%     
==========================================
  Files          80       80              
  Lines        8579     8584       +5     
  Branches       75       75              
==========================================
- Hits         7126     7124       -2     
- Misses       1124     1129       +5     
- Partials      329      331       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adombeck]

This is because what it could happen is:

We start in parallel:
-> User selection
-> UI Layouts -> Available brokers fetching

This reminds me of another thing I've noticed which I find odd: When I type sudo login foo@bar, first the Username: foo@bar is printed for a split second, and then that is overwritten by Enter your local password immediately after. Is that also caused by doing two things in parallel? Can we avoid that?

[3v1n0]

Can we avoid that?

So, that is something that should more or less have been handled via previous changes on user selection model, but if you get with latest code I also had this change that should help with that: 3v1n0@2d18463

I have done it for a future branch but we can handle it sooner if you want.

[3v1n0]

Ah, a further simple way to enforce it, it should be to use:

diff --git a/pam/internal/adapter/userselection.go b/pam/internal/adapter/userselection.go
index b43809ec..57eac971 100644
--- a/pam/internal/adapter/userselection.go
+++ b/pam/internal/adapter/userselection.go
@@ -153,3 +153,11 @@ func (m *userSelectionModel) Focus() tea.Cmd {
 	m.selected = false
 	return m.Model.Focus()
 }
+
+// View renders a text view of the user selection UI.
+func (m userSelectionModel) View() string {
+	if !m.enabled {
+		return ""
+	}
+	return m.Model.View()
+}

In fact the problem you mentioned can be easily triggered by using:

diff --git a/pam/internal/adapter/brokerselection.go b/pam/internal/adapter/brokerselection.go
index b6522916..a49029a0 100644
--- a/pam/internal/adapter/brokerselection.go
+++ b/pam/internal/adapter/brokerselection.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"strconv"
+	"time"
 
 	"github.com/charmbracelet/bubbles/list"
 	tea "github.com/charmbracelet/bubbletea"
@@ -254,6 +255,7 @@ func (d itemLayout) Render(w io.Writer, m list.Model, index int, item list.Item)
 // getAvailableBrokers returns available broker list from authd.
 func getAvailableBrokers(client authd.PAMClient) tea.Cmd {
 	return func() tea.Msg {
+		time.Sleep(5 * time.Second)
 		brokersInfo, err := client.AvailableBrokers(context.TODO(), &authd.Empty{})
 		if err != nil {
 			return pamError{

And then just run env AUTHD_PAM_RUNNER_USER=user@foo.bar go run -tags=withpamrunner ./pam/tools/pam-client login

Let me know if you want to include this in this PR too.

Thee good thing is that the one you mentioned is only a visual issue and not really
causing any security issue as we ignore all the input events there as per 3a6e144

[adombeck]

Ah, a further simple way to enforce it, it should be to use:

That looks like a simple fix indeed

Let me know if you want to include this in this PR too.

Seems unrelated, so lets maybe do it in a separate PR?