Repository navigation
Make required actions configurable (#28400) - #28419
Conversation
|
@stianst FYI this is the PR for initial support for configurable required actions we spoke about yesterday. What's already working:
What's still missing:
Some UI Experiment: I temporarily made the |
5c02f24 to
f586f87
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
f586f87 to
ca2527a
Compare
There was a problem hiding this comment.
Some remarks inline.
There was a problem hiding this comment.
I'm not sure whether this is the proper way to perform validation, but it could be used to prevent bad configurations to be written to the database.
There was a problem hiding this comment.
This and the following helper functions should be moved elsewhere... just placed them here because the authenticator config methods were also located here.
There was a problem hiding this comment.
Nitpick: Can be ok to introduce new class like RequiredActionHelper and add those methods here as they don't have much to do with credentials...
c00553b to
adb2e54
Compare
e0b6341 to
4b14b48
Compare
|
I now have an initial version working, that allows to inspect, add, update and remove required action configuration via the admin ui. Required Actions List: Required Actions Config Modal: |
There was a problem hiding this comment.
Added some more remarks to the places where I'm unsure
There was a problem hiding this comment.
Here we need to provide some configurable dummy required action to test the retrieval of the configurable properties.
There was a problem hiding this comment.
This is just for the sake of the example, and will be removed from the final PR
2b07eb1 to
add85a9
Compare
|
Thanks to @edewit the new UI now looks a bit better: Config screen: @edewit would it be possible to show the translated name instead of the alias? (Update Password instead of "UPDATE_PASSWORD"). Also I think the config dialog would scale better if the name of the required action would be on a separat line. Some users tend to have quite long (but descriptive) names for required actions. |
There was a problem hiding this comment.
Thanks,
I've added one minor comment inline. In general, I am not 100% sure if we want to rely on ComponentModel, which was the approach discussed on the meeting AFAIR. But maybe not... (added some more details in the email).
There was a problem hiding this comment.
Do we need isConfigurable and setConfigurable on the model? It seems that the configurable field is not backed in the DB and it is always derived based on the factory (like in RealmAdapter.entityToModel(RequiredActionProviderEntity entity)).
Also I believe it is not needed in the JSON representation object, which is used for export/import, but I understand it can be useful for have this in the UI layer. Is it possible to decouple those?
There was a problem hiding this comment.
Thanks for taking a look! Yes I added the configurable property to the RequiredActionProviderModel and RequiredActionProviderRepresentation . Initially I thought about deriving the "configurability" of a RequiredAction from a non-empty configuration. However, with this we currently cannot differentiate an empty configuration from a non-configurable required action, as the config propery is currently always initialized with an empty map here: https://github.com/keycloak/keycloak/blob/main/core/src/main/java/org/keycloak/representations/idm/RequiredActionProviderRepresentation.java#L35
If we would change this to be null by default, then I could remove the configurable from the model and the representation property.
I was also thinking about using the component model but then realized that required actions already have a database backed config HashMap, so I kept using that.
There was a problem hiding this comment.
I understand that the info about "configurable" is needed in the UI. But I think it is not needed in the model layer and might be cleaner to remove it from here.
I see the options like:
- Keep
configurableat the model layer andRequiredActionProviderRepresentation. Not so clean IMO. - Use the
nullto indicate. It can work, but just afraid a bit of things likeNullPointerExceptionetc. - Add stuff inside admin UI java extension (module
rest/admin-ui-extin the workspace) and make admin console to use it. I can imagine something like this in the packageorg.keycloak.admin.ui.rest.modelinside admin UI extension (the stuff in the modulerest, which contains REST endpoints used solely by admin console):
public class ConfigurableRequiredActionProviderRepresentation extends RequiredActionProviderRepresentation {
private boolean configurable;
// Getter and setter for "configurable" here ...
}
And then make org.keycloak.admin.ui.rest.AuthenticationManagementResource to return required actions like those, so the admin console UI is able to figure if action is configurable or not. The disadvantage of the extension is additional code.
My vote is either (2) or (3). WDYT?
72b1f78 to
6853f8c
Compare
|
A quick experiment for better validation support: |
|
Now using the required action |
3030ba3 to
42f0d33
Compare
|
@mposolda thanks for your review and the helpful suggestions! Regarding your proposal for moving forward, I went with a combination of (2) and (3) as discussued above:
The latest changes of this PR include:
Since all the endpoints that handle requiredactions work with the requiredaction The only difference is the following, which only has an alias available at the moment: However, using the aliases all the time means that required action aliases within a realm must be unique - is this guaranteed somehow?
I also tried to have
As an example I adapted the
Export in a realm with required action config and importing it again correctly shows the required action config values.
The PR - at least for the initial capability is on the finish line. I stell need to add a test for the client library but after that we should be good. Perhaps @edewit could take a look again on the translation of error messages if a validation fails :) |
77ef3df to
537b222
Compare
|
@jonkoops This PR currently fails due to an unrelated test: I ran the cypress tests locally and there it fails too. I currently don't see a link to my changes. Do you have an idea what the problem is? |
|
Thanks for calling help :) It seems to fail because the menu popup does not work: |
|
Looks like a test interference. I have introduced #29507 and rerun the admin UI testsuite from |
11842d3 to
18ca9ea
Compare
6a35e7a to
a98b7df
Compare
|
Unfortunately I'm blocked by the failing UI tests, which are unrelated to this PR. The same tests also fail on my machine locally when I run the latest Keycloak main (I tested this with cypress on electron, and chrome). Any help would be highly appreciated :) |
3afaec2 to
76bd1c8
Compare
76bd1c8 to
0e6d552
Compare
Tests have been fixed on main, I have rebased your PR |
There was a problem hiding this comment.
@thomasdarimont Thanks for all the updates!
I've added few comments inline. It can be good to address them, but at the same time, I don't consider any of them as a blocker, so we can possibly do as a follow-up. Whatever you prefer.
Will need review from UI team as well.
There was a problem hiding this comment.
Nitpick: This field is not used and can be removed from this class (probably all the changes from this class can be removed from this PR)
There was a problem hiding this comment.
Nitpick: Can be ok to introduce new class like RequiredActionHelper and add those methods here as they don't have much to do with credentials...
There was a problem hiding this comment.
Nitpick: I can imagine it can be useful to have some util method (maybe in the RequiredActionsHelper as suggested in other comments or in KeycloakModelUtils) like:
RequiredActionConfigModel getRequiredActionConfig(RealmModel realm, String requiredActionProviderId)
as I suppose the similar code will apply always when retrieving configurations from required action
There was a problem hiding this comment.
| .helpText("Configures the duration in seconds this action can be used after the last authentication before the user is required to re-authenticate.") // | |
| .helpText("Configures the duration in seconds this action can be used after the last authentication before the user is required to re-authenticate. This parameter is used just in the context of AIA when the kc_action parameter is available in the request, which is for instance when user himself updates his password in the account console. When the 'Maximum Authentication Age' password policy is used in the realm, it's value has precedence over the value configured here.") // |
Nitpick: just suggesting to add a bit more info to the helpText to clarify it is used just in the context of AIA and about the fact that password policy has preference.
|
@thomasdarimont Since there is conflict and this PR will require changes, WDYT about adding my "nitpicks" (if they makes sense for you) when you work on resolving conflict? :-) Thanks! |
0e6d552 to
d992693
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#testAllConcurrentlyorg.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testMigrateSessionKeycloak CI - Store Model Tests |
- Add tests for crud operations on configurable required actions - Add support exposing the required action configuration via RequiredActionContext - Make configSaveError message reusable in other contexts - Introduced admin-ui specific endpoint for retrieving required actions with config metadata Fixes keycloak#28400 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> Co-authored-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
d992693 to
2724bfe
Compare
|
@mposolda Thanks for your review and the recommendations. I fixed all your nitpicks and rebased to latest main. |
There was a problem hiding this comment.
@thomasdarimont @edewit Thanks a lot for all the updates and review!
This PR adds support for configurable required actions.
Some implementation notes:
RequiredActionConfigModelto hold the configration of a RequiredActionAuthenticationManagementResourcewith CRUD operations for RequiredAction config managementRequiredActionConfigModelvia RequiredActionContext to usersFixes #28400