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

brokers/pam: Support (error) messages on Next authentication result #815

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Feb 26, 2025

When a broker sends a message with an auth.Next result, we should inform the user about, so make the PAM module to present the error and wait a reasonable time for the user to read it, before proceeding to the next step.

UDENG-6180

Closes: #774

In this case we make the user to wait some time before being actually
able to continue to the next step so that the UI can show the message
clearly, given that's not related to the upcoming case.
@3v1n0 3v1n0 requested a review from a team as a code owner February 26, 2025 17:55
@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.06%. Comparing base (36511cd) to head (c19590f).
Report is 439 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #815      +/-   ##
==========================================
- Coverage   83.43%   79.06%   -4.37%     
==========================================
  Files          83      102      +19     
  Lines        8689    10401    +1712     
  Branches       74       74              
==========================================
+ Hits         7250     8224     +974     
- Misses       1111     1711     +600     
- Partials      328      466     +138     

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

@3v1n0 3v1n0 force-pushed the auth-next-messages branch 6 times, most recently from 44c44f2 to 080c182 Compare February 27, 2025 00:11
@3v1n0 3v1n0 marked this pull request as draft February 27, 2025 01:07
We need to test the Next case with an error message, so we limit the
case to when a reset is needed for a login session, so that we can
ensure that this happens as expected.
@3v1n0 3v1n0 force-pushed the auth-next-messages branch 2 times, most recently from 5e03a84 to e6519ef Compare February 27, 2025 19:46
We have now to wait a longer time, so let's increase the test timeout.

As per the current text, the sole reading time is computed in 5.4960s
plus the delay and extra (makes it 6.9960), so let's be a bit more
genreous.
Adapt timeout to the running environment
We may end up not queuing the events, leading to a failure since the
changes in this branch (racy bubbletea, you know!).

Mostly this is because now we have a timeout that triggers the
GetAuthenticationModesRequested{} event, that implies that the
authentication modes gets reloaded ant the first-one gets auto-selected,
so in the GDM tests we need to ensure that when "auth.Next" is returned
then we must first wait for the new challenge stage, and finally to
switch back to the auth-mode selection.

See: https://github.com/ubuntu/authd/actions/runs/13550269562/job/37879512072
Env variables should always override arguments as golden rule, so let's
do it.
While we're trying to do this smartly checking the build information for
the binaries we're testing, this doesn't work particularly well in some
test binaries (debug.BuildInfo Settings are inconsistently exposed).

So, let's just be consitent in CI, ensuring that wait times are
multiplied as expected.
@3v1n0 3v1n0 force-pushed the auth-next-messages branch from f73f34f to c19590f Compare February 27, 2025 20:56
@3v1n0 3v1n0 marked this pull request as ready for review February 27, 2025 21:13
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.

Show a message if refresh token is expired and device authentication is required
3 participants