-
Notifications
You must be signed in to change notification settings - Fork 14
Support cross-platform #98
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
base: main
Are you sure you want to change the base?
Support cross-platform #98
Conversation
Thank you for opening this PR, @peaceful-james! The code changes are minimal and straightforward. The only problem I see is that we're forcing users to accept both forms of credentials. This sounds nice as a default, but I expect some users will want a bit more control. Would it make sense to allow rendering two (or more) What do you think? |
@type1fool Yes that is a good idea. I will look into this. |
@type1fool I gave it a quick go tonight. It definitely feels better than it was. Interested in getting your eyes on it before I fix tests. |
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.
Much improved. Just a few minor nits.
Co-authored-by: Owen Bickford <[email protected]>
Co-authored-by: Owen Bickford <[email protected]>
Bug in AuthenticationComponent whereby challenge was being assigned as a fxn
@type1fool I snuck in a fix for what seems to be a typo in the authentication component (see last commit). |
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.
I haven't yet run the generator in a demo repo, but the changes look good overall. My last complaint is about the button change.
<span><%= @display_text %></span> | ||
</.button> | ||
</span> | ||
<button |
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.
I could go either way on sticking with the base button component or using the raw HTML button. However there are two issues:
- AuthenticationComponent still uses the base component.
- This change is out of scope for supporting cross-platform credentials.
In projects, I do tend to apply more styling to HTML elements in CSS to reduce component maintenance and complexity. A discussion about sensible defaults would be good before making this change.
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.
I opened #99 to start the conversation for this topic.
Overview
Resolves #87 (I think)
What problem is being solved by this branch?
Basically, when registering, we specify "platform" or "cross-platform". If using, e.g. YubiKey, then it must be "cross-platform".
We used to have "platform" hardcoded so it could not be worked around.
Changes
I added an extra button to the RegistrationComponent, for cross-platform.
Default button label is "Sign Up With Connected Device" but it can be customized.
See this screenshot of platform and cross-platform registration buttons being rendered with custom labels.
I made 1 cheeky extra change which was to remove some camelCase atoms (which cause compiler warnings, which is not pleasant). These atoms were simply map keys and that map was just being sent to the client (JS) so switching to string keys should cause no issues.
Tests
Some broke, I fixed them.
Collaborators