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.

Sign up for GitHub

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

Jump to bottom

Address PR #9175 review comments #4

Merged
merged 15 commits into from
Aug 11, 2024
Merged

Address PR #9175 review comments #4

merged 15 commits into from
Aug 11, 2024

Conversation

emlun
Copy link
Owner

@emlun emlun commented Aug 10, 2024 •
edited
Loading

@emlun emlun mentioned this pull request Aug 10, 2024
Open
10 tasks
@rasa
Copy link

rasa commented Aug 10, 2024

@emlun Once this is merged in, I'll retest.

imsodin
imsodin reviewed Aug 10, 2024
View reviewed changes
lib/api/api.go Outdated Show resolved Hide resolved
imsodin
imsodin reviewed Aug 10, 2024
View reviewed changes
lib/api/api.go Outdated Show resolved Hide resolved
imsodin
imsodin approved these changes Aug 10, 2024
View reviewed changes
Copy link

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

Sorry, I started nitting again. Overall looks good. There is one relevant comment about config changes again.

@emlun
Add function structutil.WithDefaults
371ae5d
@emlun
Set default GUIConfiguration.WebauthnRpId and .WebauthnOrigin in tests
38fb26c
@emlun
Remove default rpId in EligibleWebAuthnCredentials
328eb68
@emlun
Remove default reflection from newWebauthnEngine
fa16d27
@emlun
Delete unused receiver
a606a47
@emlun
Use short device ID instead of device name in WebAuthn RP display name
6ddf7a8 
This solves 2 problems: no need to reload the name on config changes, because
the device ID should rarely or never change; and consequently we don't get
credentials with a stale RP display name, as there's no way in WebAuthn L2 to
signal to the authenticator that the RP display name (if stored) should be
updated.
@emlun
Copy link
Owner Author

emlun commented Aug 11, 2024

No need to apologize, I really appreciate the comments!

Last night I added a few changes to also address the discussion on the default-reflection in newWebauthnEngine:

I considered doing this "globally" in mocks.Wrapper.GUIReturns(), and spent an hour or two troubleshooting under the faulty assumption that structutil.SetDefaults would set only the properties with a zero value. Turns out it unconditionally overwrites all properties that have a default value tag, which broke pretty much all of the tests in various surprising ways when I applied it that broadly (for example, all those tests competed for port 8384, which de-parallelized most of the test suite so everything timed out). Given that experience, I decided that it's probably better to be explicit about applying these defaults at the construction site than having it happen magically in some hidden shared utility.

@imsodin Would you like to re-review these changes here or shall I just merge them into syncthing#9175?

@imsodin
Copy link

imsodin commented Aug 11, 2024

Lets merge - there are now also a lot of changes here, so the benefit of being small is no longer there and the "change-indirection" adds quite a bit of cognitive load for me :)

@emlun emlun mentioned this pull request Aug 11, 2024
Closed
@emlun emlun merged commit 229a304 into webauthn Aug 11, 2024
40 checks passed
@emlun emlun deleted the webauthn-review branch August 11, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imsodin imsodin imsodin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emlun @rasa @imsodin