-
Notifications
You must be signed in to change notification settings - Fork 38
pam/adapter: Ignore stale stopAuthentication from a superseded challenge #1626
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
Draft
adombeck
wants to merge
2
commits into
main
Choose a base branch
from
fix-gdm-challenge-race
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+216
−9
Draft
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package adapter | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| "github.com/canonical/authd/pam/internal/gdm" | ||
| "github.com/canonical/authd/pam/internal/gdm_test" | ||
| "github.com/canonical/authd/pam/internal/proto" | ||
| tea "github.com/charmbracelet/bubbletea" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // collectMessages runs a command and recursively flattens the batch/sequence | ||
| // messages it produces into the concrete messages they ultimately deliver. | ||
| // tea.Batch and tea.Sequence return []tea.Cmd-shaped messages whose concrete | ||
| // types are unexported, so they are detected structurally via reflection. | ||
| func collectMessages(cmd tea.Cmd) []tea.Msg { | ||
| if cmd == nil { | ||
| return nil | ||
| } | ||
| msg := cmd() | ||
| if cmds, ok := asCmdSlice(msg); ok { | ||
| var msgs []tea.Msg | ||
| for _, c := range cmds { | ||
| msgs = append(msgs, collectMessages(c)...) | ||
| } | ||
| return msgs | ||
| } | ||
| return []tea.Msg{msg} | ||
| } | ||
|
|
||
| // asCmdSlice reports whether msg is a []tea.Cmd-shaped batch/sequence message | ||
| // and, if so, returns its commands. | ||
| func asCmdSlice(msg tea.Msg) ([]tea.Cmd, bool) { | ||
| v := reflect.ValueOf(msg) | ||
| if v.Kind() != reflect.Slice || v.Type().Elem() != reflect.TypeOf(tea.Cmd(nil)) { | ||
| return nil, false | ||
| } | ||
| cmdType := reflect.TypeOf(tea.Cmd(nil)) | ||
| cmds := make([]tea.Cmd, v.Len()) | ||
| for i := range cmds { | ||
| cmd, ok := v.Index(i).Convert(cmdType).Interface().(tea.Cmd) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| cmds[i] = cmd | ||
| } | ||
| return cmds, true | ||
| } | ||
|
|
||
| func containsAuthModeSelected(msgs []tea.Msg, id string) bool { | ||
| for _, msg := range msgs { | ||
| if m, ok := msg.(authModeSelected); ok && m.id == id { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func TestGdmModelIgnoresAuthModeSelectedEcho(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // After we select an auth mode, GDM echoes the selection back as a poll | ||
| // event. Acting on that echo would re-run SelectAuthenticationMode and, for | ||
| // device auth, mint a second device code while the poll is still on the | ||
| // first one (UDENG-8799). | ||
| m := gdmModel{} | ||
| m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) | ||
| require.Equal(t, "device_auth_qr", m.pendingEchoAuthModeID, | ||
| "selecting an auth mode should record the expected echo") | ||
|
|
||
| echo := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} | ||
| msgs := collectMessages(m.handlePollResponse(echo)) | ||
| require.False(t, containsAuthModeSelected(msgs, "device_auth_qr"), | ||
| "echo of the just-selected auth mode must not trigger a re-selection") | ||
| require.Empty(t, m.pendingEchoAuthModeID, | ||
| "consuming the echo should clear the expected echo") | ||
| } | ||
|
|
||
| func TestGdmModelActsOnAuthModeChange(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // A genuine change to a different auth mode must still be acted on. | ||
| m := gdmModel{} | ||
| m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) | ||
|
|
||
| change := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("password")} | ||
| msgs := collectMessages(m.handlePollResponse(change)) | ||
| require.True(t, containsAuthModeSelected(msgs, "password"), | ||
| "selecting a different auth mode must trigger a re-selection") | ||
| } | ||
|
|
||
| func TestGdmModelActsOnSameAuthModeReselection(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Suppression is a one-shot: only the immediate echo of our own selection | ||
| // is dropped. A later genuine re-selection of the same auth mode (the user | ||
| // picking it again) must be honored, because the pending echo has already | ||
| // been consumed. | ||
| m := gdmModel{} | ||
| m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) | ||
|
|
||
| echo := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} | ||
| _ = collectMessages(m.handlePollResponse(echo)) | ||
| require.Empty(t, m.pendingEchoAuthModeID, | ||
| "the echo should have been consumed") | ||
|
|
||
| reselect := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} | ||
| msgs := collectMessages(m.handlePollResponse(reselect)) | ||
| require.True(t, containsAuthModeSelected(msgs, "device_auth_qr"), | ||
| "a genuine re-selection of the same auth mode must be honored") | ||
| } | ||
|
|
||
| func TestGdmModelStageChangeClearsPendingEcho(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // A genuine re-selection always follows a stage change back into | ||
| // authModeSelection. The stage change must drop any echo we were still | ||
| // expecting, so that the re-selection is acted on instead of being | ||
| // mistaken for the (never-delivered) echo of the previous selection. | ||
| m := gdmModel{} | ||
| m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) | ||
| require.Equal(t, "device_auth_qr", m.pendingEchoAuthModeID) | ||
|
|
||
| m, _ = m.Update(StageChanged{Stage: proto.Stage_challenge}) | ||
| m, _ = m.Update(StageChanged{Stage: proto.Stage_authModeSelection}) | ||
| require.Empty(t, m.pendingEchoAuthModeID, | ||
| "a stage change must drop a still-pending echo") | ||
|
|
||
| reselect := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} | ||
| msgs := collectMessages(m.handlePollResponse(reselect)) | ||
| require.True(t, containsAuthModeSelected(msgs, "device_auth_qr"), | ||
| "re-selecting the same auth mode after a stage change must be honored") | ||
| } |
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change here goes against the point of using returned copy of the model itself to do changes, in fact ideally no method should use a pointer receiver.