pam/gdmmodel: Simplify handling of cancellation events and cleanup code #827
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
- Coverage 83.43% 79.33% -4.11%
==========================================
Files 83 104 +21
Lines 8689 10502 +1813
Branches 74 75 +1
==========================================
+ Hits 7250 8332 +1082
- Misses 1111 1706 +595
- Partials 328 464 +136 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
from
94ee619 to
ee5282e
Compare
3v1n0
changed the title
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
5 times, most recently
from
f54b56d to
873036b
Compare
adombeck
reviewed
Mar 12, 2025
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
2 times, most recently
from
bd434c9 to
28940a9
Compare
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
2 times, most recently
from
1772c29 to
45e3746
Compare
adombeck
reviewed
Mar 19, 2025
adombeck
reviewed
Mar 19, 2025
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
from
45e3746 to
ca4dd85
Compare
adombeck
approved these changes
Mar 20, 2025
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
5 times, most recently
from
3941e52 to
fd2d4ed
Compare
…ange It's easier to track when debugging and also more consistent everywhere, so that the event gets queued and performed without any priority.
On authModesReceived{} message we were aggressively selecting the first
authentication mode, despite the authentication stage was not being
focused, and this could have lead to troubles because a previously
queued stage change could then interfere with the request, like:
- Request brokers and queue stage change to auth mode selection when done
- Brokers received: automatic broker selection request is queued
- Authentication starts
- Stage change that was previously queued finally happens
- We end up being back to auth mode selection
Also: https://github.com/ubuntu/authd/actions/runs/13813999841/job/38797674173
This has been, mostly, highlighted by the change to request the stage
change via messages (previous commit), but we could have been wrong
with the order of the subsequent other events anyways.
So, to prevent this to happen and to ensure that we follow the policy
that a stage doesn't do anything until it has the focus, delay the
automatic broker selection until the authentication selection stage is
actually focused.
See also commit 5133a87 that had similar rationale and also related to
the authentication selection procedure.
Closes: ubuntu#730
We use it to ensure that no secret is shared in logs, so let's ensure that it works as expected in a test
When using the SafeString we could end up trying to deference a null pointer, fix it and add a test
As per a bubbletea issue sub-sequences of messages may not preserve the order, so let's just handle them by appending the messages to the commands we already have instead, so that there's no risk that the order is not preserved and that the subsequence is executed when all the remaining commands also are See: charmbracelet/bubbletea#847
It should be easier to debug than simple numbers.
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
Only add a message if the cancellation has happened with a message
In case an event from the adapter package gets to Update function, show the debug information for it. We only do it when debugging is enabled during tests, so even if it's not the cleanest solution it's good enough to figure out what's going on
Add a generic command message that will allow to nest messages even if we're not waiting for a specific stage.
It allows to wait for authentication to be started and then continue with next command events. This is needed because gdmTestSendAuthDataWhenReady is delivered as soon as received, so can't be waited in a sequence
…hentication
In some tests we need to send events or commands juts after the the
authentication data is sent, however we were doing this without a clear
order, because the gdmTestSendAuthDataWhenReady{} commands may have been
completed when the next command was processed.
So use instead the nested commands via gdmTestSendAuthDataWhenReadyFull,
in this way we're sure that the next commands will be processed only
after that we're done with the authentication.
This should fix various tests races
While we have an isAuthenticated event already, that is notified only when the authentication request has been started, that is something that does not happen automatically for non-wait clases. So add a new message to signal that the authentication phase is over and that the authentication model is not focused anymore so that we can use it in the gdm model to track that no authentication is happening anymore We could do the same by using the stage changes too, but this is cleaner and it's not confused with other potential cases in which we may trigger a stage change for other reasons
We were notifying the stage change after blur, while especially after commit a3507bf, it's better notify the stage change when the new stage is actually focused.
…focused We used to start the authentication as soon as the model was filled, but this could lead to start it before that the stage was actually active (as being marked as focused). This was not correct and might have lead to change stage events to arrive later than authentication started events. Make this order clearer now by only starting the authentication when the stage has been focused and ensure this is always the case in tests. In the CLI case now we've also to avoid writing the view if the authentication has not been started yet, or the fields may not accept input when they're about to start
If we have a qrcode button to regenerate the qr code, we should depend on it in order to ensure that the whole view is focused, or we may end up consider the qr code view as focused when it's actually not yet the case and Blur() call would be a no-op basically
…start
We used to start the authentication when the authentication model was
focused, and this was fine for the CLI model, but other models still
depended on a specific stage change call that had no clear order and
that could have caused effects before or later than the delivery of the
related startAuthentication{} message.
So cleanup this logic and be consistent everywhere by using a new
StageChanged{} event that is propagated when the stage has changed and
that is now the one responsible for also starting the authentication,
so that is now clearly happening only after we're in the challenge stage
As per the fact we're now tracking the focus state also when the gdm model is used (via the focusTracker model) it's not up anymore 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. 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. Finally, update tests expectations accordingly
Add some tests verifying that once a wait operation has been started we are able to cancel it both via explicit IsAuthenticatedCancelled request or via implicit change stage request
The expected stage was swapped with the actual one
We used to block to send some events not to end up queuing them without the right order, but as per the previous changes this is not a problem anymore, so we can safely avoid blocking when performing pam events emissions (that leads to PAM conversations)
As we do already with qrcode, allow tuning the form UI layout. In fact by default in GDM we do support waiting, but we do not support any button, so make all this optional
In the non-button layout we don't support any button label, so add a test for this using the GDM model, since that's the one that does not support buttons in the form UI
They should just be ignored
We're sending thousands of requests, all with the same header content but we were using Go to init them which isn't great due to the fact that we end up having to allocate and free the same string maaaany times for no reason, while we can just use a defined value that gcc will nicely optimize in various places So, add few optimized functions and use them to init the protocol in the main cases, while we still keep a full initializer for testing purposes
We were using a blocking API to send polling events but we can now handle them asynchronously, given there's no risk that we're sending an event before the previous has been handled
Process all the poll replies messages at first and then continue with the next polling, so that we improve the messages ordering
The protocol changed slightly so let's ensure that we've a safe upgrade path.
3v1n0
force-pushed
the
gdm-model-testing-fixes
branch
from
fd2d4ed to
981850e
Compare
|
Pushing together with fixed gnome-shell with breaks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 sameUDENG-6243