Conversation
|
No admin console support yet. This is covered by a different JIRA. Just pulling together things I need for OSIN integration |
mposolda
left a comment
There was a problem hiding this comment.
Cool! Just few comments:
-
I think constraint "CONSTRAINT_CLIENT_AUTH_FLOW_BINDINGS" needs to be renamed in jpa-changelog-4.0.0.xml to shorter name. I think it will break on Oracle, which has 30 characters limit for names of tables, columns, constraints etc.
-
Just a minor comment, that OAuthClient has method "clientId" already. Maybe new setter method "setClientId" is not needed then?
hmlnarik
left a comment
There was a problem hiding this comment.
Thanks, just a single implementation thought, please see inline.
| * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a> | ||
| * @version $Revision: 1 $ | ||
| */ | ||
| public interface AuthenticationFlowBindings { |
There was a problem hiding this comment.
Should this be rather turned into enum? Then the JPA ClientEntity class would use @MapKeyEnumerated rather than @MapKeyColumn(EnumType.STRING) and the AuthenticationFlowResolver methods could be implemented inside the enum:
public enum AuthenticationFlowBindings {
BROWSER_BINDING {
@Override
public AuthenticationFlowModel resolveFlow(AuthenticationSessionModel authSession) {
...
}
},
DIRECT_GRANT_BINDING {
@Override
public AuthenticationFlowModel resolveFlow(AuthenticationSessionModel authSession) {
...
}
};
public abstract AuthenticationFlowModel resolveFlow(AuthenticationSessionModel authSession);
}That would rid of the String constants and ease adding the new flows in the future since all changes would be needed just in this single class (enum).
There was a problem hiding this comment.
Protocol plugins are going to have their own flows (i.e. OpenShift needs one), so it needs to be string based. See https://issues.jboss.org/browse/KEYCLOAK-6337
There was a problem hiding this comment.
I see, Strings make perfect sense then. Nit: would it be better to use public static final String fields and a regular class instead of interface?
There was a problem hiding this comment.
Is that a best practice? Or is it a Hynek thing? Over the years I usually try and put constants in an interface for a public API.
There was a problem hiding this comment.
It is supposed to be best practice (at least according to Joshua Bloch's Effective Java, Item 19 (full explanation in the book, extract in https://github.com/HugoMatilla/Effective-JAVA-Summary#19-use-interfaces-only-to-define-types). But indeed, it may only be Hynek's thing.
|
Haven't had a chance to look in detail yet, but what happens if client #1 starts authentication with flow #1, but doesn't finish fully (user opens new tab or whatever). In the meantime client #2 starts authentication with flow #2. Now there's already an authentication session in place, but it's for a different flow. |
|
Looks good to me, except the question above. Still unsure what happens if one authentication session is in progress with a different flow. |
|
@stianst Auth sessions can't have more than one client. The flow is determined by the auth sessions's client. |
|
Added marek's fixes. |
|
@mposolda knows the details better than me, but there is a root authentication session that is started when an authentication starts. There can be multiple clients associated with a root authentication session. The root session controls where the user is in the current flow. I believe you can login with username/password, abandon the login, login from a different client and now since you've already entered username/password only otp would be requested. |
|
Just tried this out and it should be OK. I logged in to admin console added a update profile required action. Then re-logged in from admin console, entered username/password. Then opened a new tab with account and it showed username/password. Not how I expected it to behave as I thought it would continue where it left off. @mposolda could you explain how this is expected to work? |
|
@stianst The flow is determined from the client attached to the current AuthenticaitonSessionModel, which represents a specific client and tab_id. RootAuthenticationSessionModel is a different class. iF the client doesn't have an override, then the realm binding is used. See AuthenticationFlowResolver class I added. |
|
@stianst Looking in the comment in RootAuthenticationSessionModel: /** |
|
The RootAuthenticationSession has more authenticationSessions, and authentication executions are tracked per authenticationSession not per RootAuthenticationSession. When application redirects through OIDC/SAML Authentication request to the Keycloak, it always adds new authenticationSession to the existing RootAuthenticationSession, but the authentication executions are tracked from scratch. In other words, with the scenario above, when you open tab1 with admin console, there is new RootAuthenticationSession created and also new authenticationSession created in it. Then when you open tab-2 with accountManagement, there is new authenticationSession created inside the rootSession. So rootSession now has 2 authentication sessions (one for admin console and one for account managmenet) and both tracks authentication executions independently. We had single authenticationSession per whole browser in 3.2 and 3.3 and tracking single set of executions in all browser tabs. So when you do the scenario above in 3.2, then you will be redirected directly to updateProfile requiredAction when you open tab2 with account management. Problem is, that there was lots of usability issues reported for various corner cases around this against KC 3.2 and 3.3. It turned that people rather want (and expect) to be able to start authentication from beginning when they redirect from the application to the initial OIDC/SAML request. This was the behaviour in Keycloak 3.1 and older and nobody had any issues regarding this. |
|
@patriot1burke Thanks! Oracle should be happy now. Hopefully :) |
There was a problem hiding this comment.
Using keycloak-4.0.0.Beta1
Getting below error , while fetching client
getRealmInstance().clients().get(CLIENT_ID)
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "authenticationFlowBindingOverrides" (class org.keycloak.representations.idm.ClientRepresentation), not marked as ignorable (38 known properties: "enabled", "clientAuthenticatorType", "redirectUris", "clientId", "authorizationServicesEnabled", "name", "implicitFlowEnabled", "registeredNodes", "nodeReRegistrationTimeout", "publicClient", "attributes", "protocol", "webOrigins", "protocolMappers", "id", "baseUrl", "surrogateAuthRequired", "adminUrl", "fullScopeAllowed", "frontchannelLogout", "clientTemplate", "directGrantsOnly", "rootUrl", "secret", "useTemplateMappers", "notBefore", "useTemplateScope", "standardFlowEnabled", "description", "directAccessGrantsEnabled", "useTemplateConfig", "serviceAccountsEnabled", "consentRequired", "access", "bearerOnly", "registrationAccessToken", "defaultRoles", "authorizationSettings"])
at [Source: org.jboss.resteasy.client.jaxrs.internal.ClientResponse$InputStreamWrapper@5b3e17b1; line: 1, column: 954] (through reference chain: java.util.ArrayList[0]->org.keycloak.representations.idm.ClientRepresentation["authenticationFlowBindingOverrides"])
No description provided.