Skip to content

pam/nativemodel: Ensure we're in the expected stage before initiating it #818

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

Merged
merged 7 commits into from
Mar 12, 2025

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Feb 27, 2025

Fixed handling of focus for authentication stage when not using the CLI interface, and fix the race we had for some time, making impossible to go back to auth-selection if going back from the challenge stage was fast enough (VHS quick typing speed was a good friend here :)).

From main commit:

In some cases (when the "goBack{}" request via the 'r' cancel key is
performed very quickly on the UI) we might end up doing a
nativeAuthSelection{} request when we are still technically performing
the stage change from the challenge stage to auth-mode selection.

This was particularly a problem in the "Wait" cases, given that these
events could have been arriving without a proper order, because the
cancellation of the previous event could lead to a delayed stage-change
request that we didn't handle, but it could happen also in non-wait
cases, for example:

  authd-pam-exec-DEBUG: 23:46:32.560: authenticate: called method
    Prompt((1, 'Gimme your password:\n> '))
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
    (tea.Cmd)(nil)}
  Native model update: adapter.nativeGoBack{}
  adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.nativeStageChangeRequest{Stage:2}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
    (tea.Cmd)(0xca47c0)}
  Native model update: adapter.nativeChangeStage{Stage:3}
  *adapter.ChangeStage{Stage:2}
  *adapter.authenticationModel: Reset"
  *adapter.authModeSelectionModel: Focus"
  Native model update: adapter.nativeAuthSelection{}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
        (tea.Cmd)(0xc96b00), (tea.Cmd)(0xca47c0)}
  Native model update: adapter.nativeChangeStage{Stage:2}
  adapter.authModeSelectionModel: adapter.authModeSelectionFocused{}
  Native model update: adapter.authModeSelectionFocused{}

or:

  authenticate: called method Prompt((2, 'Choose action:\n> '))
  Native model update: adapter.nativeChangeStage{Stage:3}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(nil)}
  Native model update: adapter.nativeGoBack{}
  adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.nativeStageChangeRequest{Stage:2}
  Native model update: adapter.nativeChangeStage{Stage:3}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(0x1496660)}
  adapter.ChangeStage{Stage:2}
  *adapter.authenticationModel: Reset
  *adapter.authModeSelectionModel: Focus
  Native model update: adapter.nativeAuthSelection{}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(0x1483980), (tea.Cmd)(0x1496660)}
  Native model update: adapter.nativeChangeStage{Stage:2}
  adapter.authModeSelectionModel: adapter.authModeSelectionFocused{}
  Native model update: adapter.authModeSelectionFocused{}

As visible, the nativeStageChangeRequest (which triggered the selection
via nativeAuthSelection{}) was arriving *before* that the request of
changing the internal stage value (via nativeChangeStage{}) was actually
delivered. And this prevented nativeAuthSelection{} to anything (or any
other stage-change request in general).

So, basically we need to ensure that the request is happening only after
the internal stage is changed, and this is now fixed by relying on the
main StageChange event request that eventually will update the internal
state, after we're triggering again nativeStageChangeRequest{}.

Closes: #719

UDENG-6191

@3v1n0 3v1n0 requested a review from a team as a code owner February 27, 2025 14:54
@3v1n0 3v1n0 force-pushed the native-back-fixes branch from 1959497 to d9cd079 Compare February 27, 2025 14:54
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 48.78049% with 21 lines in your changes missing coverage. Please review.

Project coverage is 79.07%. Comparing base (36511cd) to head (c69f58b).
Report is 459 commits behind head on main.

Files with missing lines Patch % Lines
pam/internal/adapter/nativemodel.go 37.50% 9 Missing and 6 partials ⚠️
pam/internal/adapter/focustracker.go 60.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
- Coverage   83.43%   79.07%   -4.36%     
==========================================
  Files          83      103      +20     
  Lines        8689    10406    +1717     
  Branches       74       74              
==========================================
+ Hits         7250     8229     +979     
- Misses       1111     1713     +602     
- Partials      328      464     +136     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@3v1n0 3v1n0 force-pushed the native-back-fixes branch 2 times, most recently from b8b7305 to dadebf9 Compare March 3, 2025 17:19
@3v1n0 3v1n0 force-pushed the native-back-fixes branch 2 times, most recently from f9f87ea to 424b914 Compare March 11, 2025 17:49
3v1n0 added 7 commits March 12, 2025 14:00
… PAM

When using the gdm or native modules we still use the authentication
model as our data model to track the focused widget status, however
since we don't have any current model set in such case, we end up never
marking the "Authentication" model as ever tracked.

This has implications in the model stage change, since we do use the
focus state to figure out what is the current stage.

In these case add then a very simple focus tracker stage so that the
authentication state can still marked as focused when it's the case.

This should fix some "focus jumps" we may see in some cases (especially
in GDM)
As per previous commit we are now aware all the times when we're in the
authentication phase, so we can safely just reset the authentication
model only after we've a stage change that goes from it to any other
stage.

Note that resetting the authentication also implies waiting for the
previous authentication request being cancelled, reason why it's
important to do every time we switch back from the focus stage and that
we do it before communicating the stage change to other models such as
gdm and cli.
As per previous commit it's not up anymore to perform any cancellation
in the native model, since the generic one handles it for us and in
order.
It's another blocking operation and we should not stop the model
events handling while it's running even if the UI is technically blocked
for input
In some cases (when the "goBack{}" request via the 'r' cancel key is
performed very quickly on the UI) we might end up doing a
nativeAuthSelection{} request when we are still technically performing
the stage change from the challenge stage to auth-mode selection.

This was particularly a problem in the "Wait" cases, given that these
events could have been arriving without a proper order, because the
cancellation of the previous event could lead to a delayed stage-change
request that we didn't handle, but it could happen also in non-wait
cases, for example:

  authd-pam-exec-DEBUG: 23:46:32.560: authenticate: called method
    Prompt((1, 'Gimme your password:\n> '))
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
    (tea.Cmd)(nil)}
  Native model update: adapter.nativeGoBack{}
  adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.nativeStageChangeRequest{Stage:2}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
    (tea.Cmd)(0xca47c0)}
  Native model update: adapter.nativeChangeStage{Stage:3}
  *adapter.ChangeStage{Stage:2}
  *adapter.authenticationModel: Reset"
  *adapter.authModeSelectionModel: Focus"
  Native model update: adapter.nativeAuthSelection{}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0xca47c0),
        (tea.Cmd)(0xc96b00), (tea.Cmd)(0xca47c0)}
  Native model update: adapter.nativeChangeStage{Stage:2}
  adapter.authModeSelectionModel: adapter.authModeSelectionFocused{}
  Native model update: adapter.authModeSelectionFocused{}

or:

  authenticate: called method Prompt((2, 'Choose action:\n> '))
  Native model update: adapter.nativeChangeStage{Stage:3}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(nil)}
  Native model update: adapter.nativeGoBack{}
  adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.isAuthenticatedCancelled{msg:""}
  Native model update: adapter.nativeStageChangeRequest{Stage:2}
  Native model update: adapter.nativeChangeStage{Stage:3}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(0x1496660)}
  adapter.ChangeStage{Stage:2}
  *adapter.authenticationModel: Reset
  *adapter.authModeSelectionModel: Focus
  Native model update: adapter.nativeAuthSelection{}
  Native model update: tea.sequenceMsg{(tea.Cmd)(0x1496660),
    (tea.Cmd)(0x1483980), (tea.Cmd)(0x1496660)}
  Native model update: adapter.nativeChangeStage{Stage:2}
  adapter.authModeSelectionModel: adapter.authModeSelectionFocused{}
  Native model update: adapter.authModeSelectionFocused{}

As visible, the nativeStageChangeRequest (which triggered the selection
via nativeAuthSelection{}) was arriving *before* that the request of
changing the internal stage value (via nativeChangeStage{}) was actually
delivered. And this prevented nativeAuthSelection{} to anything (or any
other stage-change request in general).

So, basically we need to ensure that the request is happening only after
the internal stage is changed, and this is now fixed by relying on the
main StageChange event request that eventually will update the internal
state, after we're triggering again nativeStageChangeRequest{}.

Closes: ubuntu#719
The entry uses an ECHO ON prompt by default so we should wait for the
value being properly rendered.
@3v1n0 3v1n0 force-pushed the native-back-fixes branch from 424b914 to c69f58b Compare March 12, 2025 14:14
@3v1n0 3v1n0 merged commit 9981249 into ubuntu:main Mar 12, 2025
13 of 14 checks passed
3v1n0 added a commit that referenced this pull request Mar 21, 2025
…de (#827)

We are now tracking the focus state also when the gdm
model is used (via the focusTracker model), so it's not up to us to
perform any cancellation in the GDM model either, since the generic
authentication model handles it for us already, and in the proper order.

Controlling it from the gdm model side, has always been a source for
races that sometimes lead to tests failures.

As per this, the AuthResult cancelled event is not sent anymore to gdm
unless if that happens because a wait operation got cancelled, as it
happens in the other models too, given that a stage change now
implicitly already cancels the ongoing authentication request.

Also, many tests cleanups that I had around.

Note that this requires also an update in the gnome-shell side to have a
fully working "back" button (that I'll upload to edge once this lands)

 ~~This depends on #818, since the initial commits are the same~~

UDENG-6243
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.

Issue: Using back command 'r' in native UI can be racy
3 participants