Skip to content

Scoped WebAuthn: Refactor SessionData#36666

Merged
Joerger merged 4 commits intomasterfrom
joerger/scoped-webauthn-session-data
Jan 19, 2024
Merged

Scoped WebAuthn: Refactor SessionData#36666
Joerger merged 4 commits intomasterfrom
joerger/scoped-webauthn-session-data

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Jan 13, 2024

Moves custom webauthn SessionData struct out of webauthn.proto to an internal implementation. This struct was previously only used to standardize json marshalling/unmarshalling for backend storage. Now it also includes Teleport extensions.

This was requested as part of the work for Scoped Webauthn Challenges.

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Jan 13, 2024
@Joerger Joerger requested a review from codingllama January 13, 2024 02:18
@Joerger Joerger changed the title Joerger/scoped webauthn session data Scoped WebAuthn: Refactor SessionData Jan 13, 2024
@github-actions github-actions Bot requested a review from GavinFrazar January 13, 2024 02:19
@Joerger Joerger requested a review from rosstimothy January 13, 2024 02:20
Comment thread lib/auth/webauthn/session.go Outdated
Comment thread lib/auth/webauthn/session.go Outdated
Comment thread lib/auth/webauthn/session.go Outdated
Comment thread lib/auth/webauthn/login_test.go Outdated
Comment thread lib/auth/webauthn/session.go Outdated
@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch from bfefc29 to 837f078 Compare January 16, 2024 20:41
@Joerger Joerger requested a review from codingllama January 16, 2024 20:41
@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch from 837f078 to adeba02 Compare January 16, 2024 21:17
@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch from adeba02 to 745a86d Compare January 17, 2024 02:25
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Please address the comments, otherwise LGTM.

Comment thread lib/auth/webauthntypes/webauthn.go Outdated
Comment thread lib/auth/webauthntypes/webauthn.go Outdated
Comment thread lib/auth/webauthntypes/webauthn.go Outdated
@codingllama
Copy link
Copy Markdown
Contributor

It also looks like you have a few test failures.

@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch from 745a86d to 8e3a094 Compare January 17, 2024 18:28
Base automatically changed from joerger/scoped-webauthn-proto to master January 17, 2024 18:58
@Joerger Joerger enabled auto-merge January 17, 2024 21:18
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Jan 17, 2024

@r0mant @zmb3 Can i get a flaky test skip? See #36832

Comment thread lib/auth/webauthntypes/webauthn.go Outdated
@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch 2 times, most recently from 945d258 to 7587f2f Compare January 19, 2024 01:53
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Jan 19, 2024

@codingllama I am still getting flaky test issues with TestIdentityService_UpsertGlobalWebauthnSessionData_maxLimit, though I can no longer reproduce it locally after your fix. Any ideas?

@codingllama
Copy link
Copy Markdown
Contributor

It looks like that when you run multiple tests they interact in a bad way. Let me see what I can do.

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Jan 19, 2024

@r0mant @zmb3 Can I please get a flaky test skip to unblock this?

@codingllama
Copy link
Copy Markdown
Contributor

Flaky fix at #36945.

@Joerger Joerger force-pushed the joerger/scoped-webauthn-session-data branch from 7587f2f to a092a54 Compare January 19, 2024 19:52
@Joerger Joerger changed the base branch from master to codingllama/globallimiter-flaky-again January 19, 2024 19:53
@Joerger Joerger disabled auto-merge January 19, 2024 19:53
Base automatically changed from codingllama/globallimiter-flaky-again to master January 19, 2024 21:09
@Joerger Joerger added this pull request to the merge queue Jan 19, 2024
Merged via the queue into master with commit 00f37d4 Jan 19, 2024
@Joerger Joerger deleted the joerger/scoped-webauthn-session-data branch January 19, 2024 21:40
Joerger added a commit that referenced this pull request Jan 19, 2024
* fix: Use random scopes on TestIdentityService_UpsertGlobalWebauthnSessionData_maxLimit

* Replace webauthn SessionData proto definition with an internal definition.

* Remove unused SessionData proto message.

* Address comments; Fix test.

---------

Co-authored-by: Alan Parra <alan.parra@goteleport.com>
github-merge-queue Bot pushed a commit that referenced this pull request Jan 20, 2024
* fix: Use random scopes on TestIdentityService_UpsertGlobalWebauthnSessionData_maxLimit

* Replace webauthn SessionData proto definition with an internal definition.

* Remove unused SessionData proto message.

* Address comments; Fix test.

---------

Co-authored-by: Alan Parra <alan.parra@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants