Repository navigation
Conversation
62c0b10 to
e1e4a79
Compare
There was a problem hiding this comment.
Might be better to just have a endpoint to set the default rather than moveToFirst/moveAfter
There was a problem hiding this comment.
Set default button should probably be before the remove button.
|
Thanks, I'm surprised that we don't already have a way to set the default two factor mechanism. Wondering if rather than having a set default button we should consider having a drop-down for two-factor to allow setting the default. |
How would that look like? A select under "Set up authenticator application"? |
Enable the credential API endpoints to allow a user to set it's default authenticator application in the account console and implement a way to set a default authenticator application. Previously the first added authenticator application was seen as the default and could not be changed by the user.
e1e4a79 to
97900eb
Compare
|
@stianst @jelly I recall we discussed it some times ago, but not sure we agreed on how exactly to do this... (That is likely reason why not yet available in account console). Thanks for the PR as it is good opportunity to restore the discussion :-) +1 to have the drop-down for two-factor. One of the reasons is, that user can have multiple 2-factor credentials from multiple types. For example he can have 3 OTP credentials (Authenticator application) and 2 WebAuthn credentials (Security Key). It will be nice if user can select default credential from all those 5 credentials. With drop-down, it is easier to achieve it as drop-down will contain all 5 credentials and user will choose the default from them. With the "Set Default" button, it is possible usability challenge how to nicely show, which is the default credential from all those 5 credentials (As credentials are currently grouped by "credential types"). However the bigger challenge is, how to address this for 1st-factor credential? For example user can have Password credential and 2 WebAuthn-passwordless keys. It will be nice if he can choose the default "1st-factor" from those 3 credentials. That is bigger challenge as there is currently separate category for "Basic Authentication" (with Password credential) and separate category for "Passwordless" . Will it be good to merge those into single category "Basic authentication" and remove the passwordless category from account console as part of this effort? Then we can show Password together with WebAuthn-passwordless security keys and user can select default from them for the "Basic Authentication" section. |
There was a problem hiding this comment.
Adding the comment for the label, which is specific to "Authenticator application" . Will be nice if this PR is not tightly coupled to "Authenticator application" type. And even better if it is not specific only to "showing default 2-factor credential", but works also for "1st-factor credential" .
| invalidRequestMessage=Invalid Request | ||
| successSetDefaultMessage={0} was set as default. | ||
| setDefaultCredTitle=Set {0} as default | ||
| setDefaultCred=Set {0} as default authenticator application? |
There was a problem hiding this comment.
This implies that this PR is just for set default credential of type "authenticator application" . I think it will be good if this PR is not specific only to "Authenticator application" (OTP) credential type, but can be used for all other credential types of all factors.
There is |
|
@mposolda @stianst @jelly I create a quick sketch to reflect the two solutions you've mentioned. (I'm just updating the UI of the account console so this page is in the new look.) I agree that it's better to move the webauthn-passwordless together with the password - that will make it easier to set the default for the 1st-factor credential. For the solution that adding a 'Set default' button to each row, I tried to address the problem that how to clarify the scope of the default credential. But from the UX perspective, the dropdown option may be neater. Here are the screens for your reference: [Dropdown option] [Set default buttons] |
The dropdown looks like the better solution to me, I don't know how the passwordless security key basic authentication works. I'll look into updating the PR with a dropdown soon. |
|
@yih-wang +1 for the dropdown. Thanks for looking into this! One minor note: We can hide the "Default to use" text with dropdown in case there is only single credential (or non-credentials) for the particular type. For example if user has only password and NOT any passwordless webauthn keys, we don't need to show it for "Basic Authentication" . Same for two-factors if user has only single or zero two-factor credentials. |
You can take a look at https://www.keycloak.org/docs/latest/server_admin/index.html#_webauthn . You need security key for test WebAuthn, to be able to setup security key for the user. Another option is to temporarily cherry-pick this PR to your branch #8730 to be able to test with multiple types of two-factor mechanisms ("Recovery codes" is another type of two-factor authentication in addition to OTP and WebAuthn). Just make sure to NOT include this change in your final PR :-) But anyway, I think you will be able to change the PR to dropdown even without multiple two-factor types available. The ability to test with multiple types of two-factor mechanisms (EG the case when user has 3 OTP and 1 WebAuthn credential) is nice as a "bonus" .
Thanks! |
+1. That makes sense to me. |
|
+1 To dropdown. It's definitively the cleaner solution. I'm also in favour of hiding the dropdown if user only has one choice ;) |
|
@jelly Hi, did you had a chance to look at the dropdown option or not yet? |
I lost my dev setup, so need to find time to reset that up so in short no sadly. |
|
@jelly Hello, any progress here? :)) |
Hey! Sorry, I've been busy and need to find some time to re-setup my development environment. So if someone else wants to take over that's fine by me. |
|
@jelly Thanks for the clarification. I am closing the PR for now as it is going to be done anyway in a bit different way. I've created follow-up issue #14340, which references this PR . Anyone is welcome to comment on that issue that you are interested in contributing it (so there are not multiple contributors working on this at the same time). |
This implements the ability to set the "default" authenticator application simply by calling the
moveToFirstAPI call.Authenticator application details page changes, adding a "set default" button.
Set default dialog
Is this behaviour acceptable? I saw there is a requirement for adding tests, but I couldn't find tests for the remove action to base my tests on?
I've just looked at: