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.
|
@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>
Details
keycloak-webauthn-metadata.jsonat Quarkus build timeAccount UI
Old
New
Login UI