Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pam/userselection: Do not draw the model when not enabled #627

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Nov 8, 2024

When the model is not enabled we may still have it focused, since it's the default initial model that we have in the CLI mode.

This may cause to a visual issue (not a security one, since the input is disabled for this):

  • login with the UI using a preset user
  • while the client is connecting to the server we may show a "frozen" user selection view with the pre-selected user written in.

This is not really something we need to show, so do not draw anything until we've something to show.

This can be replicated easily by applying:

diff --git a/pam/internal/adapter/brokerselection.go b/pam/internal/adapter/brokerselection.go
index 54725222..31c56f7b 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 running env [email protected] go run -tags=withpamrunner ./pam/tools/pam-client login

UDENG-5166

When the model is not enabled we may still have it focused, since it's
the default initial model that we have in the CLI mode.

This may cause to a visual issue (not a security one, since the input
is disabled for this):
 - login with the UI using a preset user
 - while the client is connecting to the server we may show a "frozen"
   user selection view with the pre-selected  user written in.

This is not really something we need to show, so do not draw anything
until we've something to show.
@3v1n0 3v1n0 requested a review from a team as a code owner November 8, 2024 04:41
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.04%. Comparing base (36f3d0a) to head (381738e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
+ Coverage   82.99%   83.04%   +0.05%     
==========================================
  Files          80       80              
  Lines        8584     8589       +5     
  Branches       75       75              
==========================================
+ Hits         7124     7133       +9     
+ Misses       1129     1126       -3     
+ Partials      331      330       -1     

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

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

pam/internal/adapter/userselection.go Show resolved Hide resolved
Copy link
Contributor

@adombeck adombeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be less frequent, but sometimes I still see the "Select your authentication method" for a split second when using login, before it asks me for the local password

@adombeck
Copy link
Contributor

adombeck commented Nov 8, 2024

It seems to be less frequent, but sometimes I still see the "Select your authentication method" for a split second when using login, before it asks me for the local password

You can see it here (in the third try)

Screencast.from.2024-11-08.13-53-37.trimmed.mp4

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Nov 8, 2024

Well... That's another issue 😅.

This PR covers the user selection, not the broker selection... And that one isn't likely also even "fixable", unless we accept adding a timeout not showing anything until we're in a stage for some human time...

So open a new issue, please

@adombeck
Copy link
Contributor

adombeck commented Nov 8, 2024

This PR covers the user selection, not the broker selection...

I see

So open a new issue, please

Done, that's https://warthogs.atlassian.net/browse/UDENG-5170

@adombeck adombeck dismissed their stale review November 8, 2024 14:10

Out of scope

@3v1n0 3v1n0 merged commit 0704179 into ubuntu:main Nov 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants