KEYCLOAK-14774 Add AuthenticationFlows and AuthenticatorConfig to KeycloakRealm CRD #230
Conversation
|
@slaskawi I created this PR for initial review/some questions. I can create an e2e test similar to keycloakRealmWithIdentityProviderTest or extend it. Which do you prefer? As far as I can see no unit tests work at this level currently. If you have an idea for a unit test, please shout out. |
|
@slaskawi should the defaults in the Java code be updated? Perhaps in https://github.com/keycloak/keycloak/blob/master/core/src/main/java/org/keycloak/representations/idm/AuthenticationFlowRepresentation.java#L27-L35 Should any boolean default to true? |
chlunde
changed the title
|
@chlunde Thanks for the Pull Request! That's a very nice piece of work!
I believe you already implemented a test (
There's no much sense in testing those CRs at unit level. They need to be executed against a running Keycloak instance with a realm API endpoints. Testing the out on e2e level is the only way to do it.
I think it does make sense. @stianst @mposolda WDYT?
Probably not. |
@slaskawi I think maybe |
|
Re. e2e testing @slaskawi ProblemThe tests in this PR currently would not break if the "Auto Link" is missing from the import, it just checks that the IDP is there. This means we only test that the import works but if for example unknown fields are ignored on import, we will not catch errors using this e2e test. Suggested solutionI wonder if the best e2e test is to export the realm and write jsonpath queries (or similar) to verify the contents, instead of scraping the UI and exercising the logic in keycloak. Is it be sufficient that the e2e tests that keycloak is configured, not how keycloak behaves? Otherwise the e2e tests will have the same complexity as the keycloak e2e tests and need a real LDAP server, another OIDC provider etc. This increases runtime, complexity and increases the likelyhook of flaky tests. Problems with export testThe downside is that the current keycloak export foarmat does not always match the REST API JSON. For example, some types are exported as components/subcomponents. So I guess the export format is subject to change and the tests could break on upgrade. Suggested implementation
|
|
Should priority be optional? If it is, I think the Java side should be updated to calculate a priority to preserve the order in the CRD. Currently everything gets the same priority, so I assume this means unpredictable behavior. |
|
If we create some additional types for deserializing the JSON, the tests could look like this: backup, err := exportRealm(framework, keycloakCR, realmName)
if err != nil {
return err
}
var found = false
for _, flow := range backup.AuthenticationFlows {
if flow.Alias == autoLinkFlow.Alias {
found = true
if diff := cmp.Diff(autoLinkFlow, flow, cmpopts.IgnoreFields(flow, "ID")); diff != "" {
return fmt.Errorf("AuthenticatorFlow mismatch (-want +got):\n%s", diff)
}
}
}
if !found {
return fmt.Errorf("Imported flow %v missing", autoLinkFlow.Alias)
}
if diff := cmp.Diff(keycloakRealmCR.Spec.Realm.AuthenticatorConfig, backup.AuthenticatorConfig, cmpopts.IgnoreFields(backup.AuthenticatorConfig, "ID")); diff != "" {
return fmt.Errorf("AuthenticatorConfig mismatch (-want +got):\n%s", diff)
}
return nilusing jsonpath was not a good idea because it is not type safe, the code is similar length or longer, and it is not easier to update at all. |
|
@mhajas @miquelsi Do you know when will you be able to look at this? I have a few other changes I would like which depends on the direction we take here, so I'm blocked from finishing this now. Do you think this style of test is OK? #230 (comment) |
chlunde
force-pushed
the
authenticationflows
branch
4 times, most recently
from
f15b1a4
to
76ab08c
Compare
There was a problem hiding this comment.
@chlunde I like your approach for testing but this Pull Request touches too many things at once. Our maintenance process is a bit heavyweight and we need to avoid multi-purpose Pull Requests.
So here's my proposal:
- Could you please create another JIRA with a goal to refactor our Realm/Client/User end to end tests to the new partial backup approach? I think it goes without saying that comparing with a partial export is far better than just throwing a payload at Keycloak and checking if it returned
HTTP 200 OK. But I'd like to make it consistent across our test codebase. - Could you please remove the new partial-test approach from this Pull Request and leave only bits related to adding
AuthenticationFlowandAuthenticationConfig? - Could you please squash your changes into two commits - feature implementation (that's the first commit) and
vendordirectory changes (that's the second one)? - Could you please rebase your changes?
Please let me know once it's ready to be reviewed. And good work - especially for the partial backup. It's very clever!
|
@chlunde Could you please let me know if/when do you plan to introduce changes I mentioned in my review comment? |
|
@slaskawi Sorry, I had more time when you had vacation, and now when you are back I have less time for this 🙁 I created https://issues.redhat.com/browse/KEYCLOAK-15378 - I'll try to create a PR for that soon. So basically you want this PR to be without any e2e testing for now. Would you also like to
|
@chlunde No worries! This is what usually happens in summer :)
@chlunde I definitely prefer solution A. Adding those fields to the CRs is by far more important than the new testing framework. |
chlunde
force-pushed
the
authenticationflows
branch
from
76ab08c
to
86de116
Compare
chlunde
force-pushed
the
authenticationflows
branch
from
86de116
to
d56b5c0
Compare
|
@slaskawi ready for review now. |
|
@chlunde Great stuff! Thanks for the contribution! |
|
Hello @slaskawi @chlunde I still have NPE with this config: apiVersion: keycloak.org/v1alpha1
kind: KeycloakRealm
metadata:
name: sso
namespace: keycloak
spec:
realm:
id: basic
realm: basic
enabled: true
displayName: Qualif
identityProviders:
- alias: agents-example-interne
providerId: oidc
displayName: Agents Example interne
enabled: true
# updateProfileFirstLoginMode: on
# trustEmail: false
# storeToken: false
# addReadTokenRoleOnCreate: false
# authenticateByDefault: false
#linkOnly: false
# firstBrokerLoginFlowAlias: first broker login
config:
clientId: k8s-qt-auth
tokenUrl: https://auth.example.org/auth/realms/agents-example-interne/protocol/openid-connect/token
authorizationUrl: https://auth.example.org/auth/realms/agents-example-interne/protocol/openid-connect/auth
clientAuthMethod: client_secret_post
logoutUrl: https://auth.example.org/auth/realms/agents-example-interne/protocol/openid-connect/logout
syncMode: IMPORT
clientSecret: T0ken
# useJwksUrl: "true"
authenticationFlows:
- alias: browser
providerId: "basic-flow"
description: browser based authentication
authenticationExecutions:
- authenticator: identity-provider-redirector
requirement: ALTERNATIVE
priority: 25
userSetupAllowed: false
autheticatorFlow: false
instanceSelector:
matchLabels:
app: sso |
|
Adding authenticatorConfig:
- alias: "oidc"
config:
defaultProvider: "oidc"Changes the NPE to : |
KEYCLOAK-14774
Additional Information
Why: Allow custom authentication flows
Verification Steps
Checklist:
Additional Notes
Two server-side issues have been created: