Repository navigation
Update WebAuthn Metadata service and show icons - #48551
Conversation
a16abb2 to
299945f
Compare
299945f to
f2b5f03
Compare
|
We might potentially remove the transport information from the login UI, as it might not be something that the user would want to care about IMHO - even for some security keys using the BLE, or NFC? When there's the label for the specific authn + authenticator type, the authenticator should be easily recognized without the need to know the transport type. WDYT? |
There was a problem hiding this comment.
Thanks @mabartos! The presentation looks much better but I don't know if we should parse the file and use real image files instead of the inline data. Something in the lines I have done in this branch. I created a quick python file that parses the json and creates the files and prints the new json. The files has been copies to login theme and account.
I don't know what it's better. I see pros and cons for the two options. Just for your consideration.
|
@rmartinc I tried to prevent storing these icons at all, but it will be probably better than using the base64 encoded SVG/PNG files. As even with the browser caching, it might be better than keeping it in memory - as some icons can be close to 64kB + the encoding... I don't see any harm to include directly the parsed icons in the /public folder... but we should probably use more specific path like I'll update the PR, and reuse some of your logic. Thanks! |
1083909 to
9b29601
Compare
|
@rmartinc Updated. |
There was a problem hiding this comment.
Looks good @mabartos! I see there are dark icons, I should have done something bad in my script because previously there were only light. 😄
The increase in size for the zip is less than 2MB so I suppose we can cope with this.
a8fa3e8 to
a96443f
Compare
|
@rmartinc Yes, there were some issues in the script, so it should be fixed now + provide guidance for future us on how to update the metadata. I've just added a simple changes to default icons when there's no provider found - just to cover all cases (should happen only in tiny amount of cases) |
Closes keycloak#48539 Closes keycloak#48540 Signed-off-by: Martin Bartoš <mabartos@redhat.com> Co-authored-by: rmartinc <rmartinc@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
There was a problem hiding this comment.
looks good from a UI point of view
| const items = [ | ||
| ...(showIcon | ||
| ? [ | ||
| <DataListCell | ||
| key="icon" | ||
| data-testrole="icon" | ||
| className="pf-v5-c-data-list__cell pf-m-icon pf-v5-u-display-flex pf-v5-u-align-items-center pf-v5-u-pt-0" | ||
| > | ||
| <div className="pf-v5-c-icon pf-m-xl"> | ||
| <picture> | ||
| {iconDarkSrc && ( | ||
| <source | ||
| srcSet={iconDarkSrc} | ||
| media="(prefers-color-scheme: dark)" | ||
| /> | ||
| )} | ||
| <img | ||
| src={iconSrc} | ||
| alt="" | ||
| width="40" | ||
| height="40" | ||
| style={{ maxWidth: "none" }} | ||
| /> | ||
| </picture> | ||
| </div> | ||
| </DataListCell>, | ||
| ] | ||
| : []), |
There was a problem hiding this comment.
It's a bit of a nit pick, but you don't need the spread operator:
| const items = [ | |
| ...(showIcon | |
| ? [ | |
| <DataListCell | |
| key="icon" | |
| data-testrole="icon" | |
| className="pf-v5-c-data-list__cell pf-m-icon pf-v5-u-display-flex pf-v5-u-align-items-center pf-v5-u-pt-0" | |
| > | |
| <div className="pf-v5-c-icon pf-m-xl"> | |
| <picture> | |
| {iconDarkSrc && ( | |
| <source | |
| srcSet={iconDarkSrc} | |
| media="(prefers-color-scheme: dark)" | |
| /> | |
| )} | |
| <img | |
| src={iconSrc} | |
| alt="" | |
| width="40" | |
| height="40" | |
| style={{ maxWidth: "none" }} | |
| /> | |
| </picture> | |
| </div> | |
| </DataListCell>, | |
| ] | |
| : []), | |
| const items = showIcon | |
| ? [ | |
| <DataListCell | |
| key="icon" | |
| data-testrole="icon" | |
| className="pf-v5-c-data-list__cell pf-m-icon pf-v5-u-display-flex pf-v5-u-align-items-center pf-v5-u-pt-0" | |
| > | |
| <div className="pf-v5-c-icon pf-m-xl"> | |
| <picture> | |
| {iconDarkSrc && ( | |
| <source | |
| srcSet={iconDarkSrc} | |
| media="(prefers-color-scheme: dark)" | |
| /> | |
| )} | |
| <img | |
| src={iconSrc} | |
| alt="" | |
| width="40" | |
| height="40" | |
| style={{ maxWidth: "none" }} | |
| /> | |
| </picture> | |
| </div> | |
| </DataListCell>, | |
| ] | |
| : []; |
There was a problem hiding this comment.
@edewit Thanks for the input. I think we're ok as is for now based on my information about the spread operator. We can improve it in some other PR.
* Update WebAuthn Metadata service and show icons Closes keycloak#48539 Closes keycloak#48540 Signed-off-by: Martin Bartoš <mabartos@redhat.com> Co-authored-by: rmartinc <rmartinc@redhat.com> * Revert back the transport type for providerId-less authenticators Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com> Co-authored-by: rmartinc <rmartinc@redhat.com>
|
Could you point us to the applicable copyright licenses or documented distribution basis for the passkey and social-provider assets imported by this change and included in Keycloak 26.8.1? We want to preserve the correct notices for unmodified redistribution rather than assume the parent project's Apache license covers every third-party icon. Please distinguish any identifying-use/trademark conditions from copyright permissions. Thank you. |
Details
keycloak-webauthn-metadata.jsonat Quarkus build timeAccount UI
Old
New
Login UI