WIP: Added basic external authentication with a Keycloak Default handler #1447
Conversation
Signed-off-by: Matthias De Bie <mattydebie@gmail.com>
|
Thanks for the PR @mattydebie. Just to let you know that we haven't forgotten the PR and that we are going to review it asap. Same for your 2nd open PR. |
|
Hey @filiphr , thanks for the update. Really appreciate you guys taking the time! |
There was a problem hiding this comment.
Thanks a lot @mattydebie for working on an external authentication within the Flowable UI applications. Sorry for the delay, but the holidays got in between 😉. I went through the PR and added some comments.
As you know the UI applications are Spring Boot applications which means that they are using Spring Security for the security. Have you had a look at the Keycloak Spring Boot Adapter which uses the Keycloak Spring Security Adapter. There are some other resources out there. I personally haven't tried it out, but I've seen a presentation about it on a conference.
Maybe if we go through that route, using Spring Boot and Spring Security authentication, we would be able to be more flexible with other implementations, as in the end we delegate this to the 3rd party providers. I think in such case it is not even needed to use our privileges and users as the 3rd party provider manages that. The only thing that would need to be done is to make sure that the 3rd party provider has the needed privileges. I think that in such case all other apps need to go through the 3rd party provider and not through IDM, as we are going to have an SSO over an SSO (IDM is a kind of an SSO as well) 😄.
When we bring this to the Flowable UI application this is going to be something extremely powerful 😄
| protected PrivilegeService privilegeService; | ||
|
|
||
| @Autowired | ||
| protected SSOHandler ssoHandler; |
There was a problem hiding this comment.
The Flowable UI IDM Application is a Spring Boot application so we can make this an optional bean. Then we won't need the isActiveMethod (I think)
| @RequestMapping(value = "/sso/external", method = RequestMethod.GET) | ||
| public void getSsoExternalUrl(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
|
|
||
| String urlBase = request.getRequestURL().toString().replace("/sso/external", "/sso/return"); |
There was a problem hiding this comment.
I don't quite follow what this method should be doing?
Also what should this method do, what is the return type? From what I can understand we just set a plain text with the external URL from the SSOHandler. Why not just have a return method that would return that if the SSOHandler is there.
There was a problem hiding this comment.
This method indeed returns the url to the SSO in a response. The login page has to know where to redirect to and this was the only way I found to work.
If the frontend could get the url by just using a plain method, then that would indeed be a better approach.
| } | ||
|
|
||
| @RequestMapping(value = "/sso/return", method = RequestMethod.GET) | ||
| public void onSsoReturnGet(HttpServletRequest request, HttpServletResponse response) { |
There was a problem hiding this comment.
Why do we need a GET and a POST is this some keycloak requirement, or we are making it flexible for different SSOs? From where are this app/sso/return methods invoked?
There was a problem hiding this comment.
This is indeed to provide a flexible implementation. Some SSO's return with a POST, some with a GET.
app/sso/return is the redirectUri that you pass to the SSO; when you successfully log in the SSO returns to this redirectUri
|
|
||
| } catch (NotFoundException e) { | ||
| // userInfo not found: create new account | ||
| createSsoAccount(request, response, ssoUserInfo); |
There was a problem hiding this comment.
Maybe we need put this creation of an account under some flag, otherwise this might blow up the system if users just start creating accounts
There was a problem hiding this comment.
I'm not sure I follow your here. This catch block only gets triggered when the SSO says OK - valid user but flowable IDM does not recognize it yet.
| createSsoAccount(request, response, ssoUserInfo); | ||
|
|
||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Maybe a proper logging here?
| private static Logger LOGGER = LoggerFactory.getLogger(IdmSSOResource.class); | ||
|
|
||
| private void loginWithSso(HttpServletRequest request, HttpServletResponse response, User user) throws IOException { | ||
| String redirectTo = request.getRequestURL().toString().replace("/app/sso/return", ""); |
There was a problem hiding this comment.
I also don't understand what this redirection should do? Why is the redirectUrl coming from a cookie? Why do we need to do the replace?
There was a problem hiding this comment.
When you get redirected to the login page, the page saves the query parameter redirectUri (the one that tells the idm where to go after successful login). Because you would lose this parameter when going to the SSO, I saved it in the cookies. This was again the only way I found to work to save the redirectPath.
The replace was done to get the baseURL. <fullurl>/app/sso/return.replace("/app/sso/return", "") = the full url.
This is indeed a dirty hack, if you know how to get it properly, please let me know.
| try { | ||
| loginWithSso(request, response, userService.getUserInformation(userInfo.getId()).getUser()); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Maybe a proper logging here?
| userInfo.getFirstName(), | ||
| userInfo.getLastName(), | ||
| userInfo.getEmail(), | ||
| UUID.randomUUID().toString() |
There was a problem hiding this comment.
Do we really need to create a password in this case?
There was a problem hiding this comment.
The password is created so that no-one can just enter a username without password and get logged in. But the created user also gets forced to login through the SSO this way.
| } | ||
| } else { | ||
|
|
||
| response.setContentType(MediaType.TEXT_HTML_VALUE); |
There was a problem hiding this comment.
Why not return some other status here or maybe a body whether the login was successful or not and then display this appropriately within the FE instead of hardcoding the HTML here?
There was a problem hiding this comment.
Hardcoding the html was indeed a test case. Will change this!
[edit]: fixed typo
| * @author mattydebie | ||
| */ | ||
| @Component | ||
| public class KeyCloakSSOHandler implements SSOHandler { |
There was a problem hiding this comment.
With this the Keycloak is a mandatory SSO. Shouldn't we make this optional through an opt in?
Something else, this class is not thread safe. Calling getExternalUrl sets some state which is then used within the handleSsoReturn
There was a problem hiding this comment.
It should be optional, I thought is was. Per default there should be a SSOHandler provided that returns as inactive. Will take a look into the OptionalBeans though.
getExternalUrl only merges some config, so it normally never changes.
|
No worries about the delay, being with your family during the holidays is more important!
I have to admit that I did not look into the Keycloak Spring Boot Adapter, did not stumble upon it as KeyCloak was not the intended target for us. Just an easy example to test this on.
In this PR you indeed only use the external SSO at login, to get basic information and to see if the user has permission to log in. All following requests needed for the Flowable UI apps go through the regular IDM (to not break the workings of flowable). I admit, one SSO would have been better. I will have to check this out. In the mean time, I'll reply to your comments. |
|
Just to make things clear, is it useful for me to keep working on this PR and apply the suggested changes or are you more inclined to deny this to check out the spring security integration? |
|
Hi @filiphr Thanks in advance |
|
Hey @mattydebie, Sorry for the delay in replies.
A contribution is always useful 😉. Are you not interested in the Spring Security approach? The Flowable UI Apps rely mostly on the Spring Security integration, there might be something specific, but we can easily change that. |
|
Ofcourse Spring security would be nice too. It would just mean I'll have to start over again 😛 .. I'll try and check the spring security again as well. But I think I ran into some kind of trouble when looking at it the first time.. 🤔 |
|
Hi, my name is Jan and I'm Matty's colleague. Since he will not have time for this anymore, I was asked to take over the task of making SSO work with Flowable. So how can we move forward from where we are? It seems that you prefer a Spring Security solution and from a bit of experimentation I did with Spring Security, it appears that this approach should be feasible. I am always in favour of a clean solution, although it may require some 'babysitting' from you or the other Flowable maintainers because my experience with Flowable architecture, Spring and Spring Security is very limited. I could use some high-level help figuring out what would need to be done. So far I think the following actions are necessary:
Since maintaining a long-lived fork is rarely a good idea, we would like to have this feature properly integrated in Flowable. Can you let me know which path to take to make this happen? |
|
Hi Jan, Exactly, a Spring Security focused solution definitely has our preference. For a first implementation we can use properties (from the application.properties) file, instead of having a UI for this (this could be a second version). If IDM is disable then you need to ensure that an authenticated user has the correct groups as well. In the IDM application you can define which groups a user is part of, so how will that part be replaced with the SSO feature? Thanks, Tijs |
|
Hi @tijsrademakers , Do you see any issues with this approach? I believe this problem has been solved many times in all sorts of software, so unless Flowable uses groups in some kind of non-standard way, there should be no problem with this. Thanks, |
|
@tijsrademakers |
|
@tijsrademakers @filiphr |
|
@VeridianDynamics I think you are looking at the IDM groups for permissions (i.e. to control access different web applications). In addition, and this is the original use, groups define who user tasks should be directed to. So unless you have no user tasks you will need groups for this purpose at least. Given you're using a Keycloak implementation you can leverage its Authorization functionality to give you a set of entitlements for the user. There's a lot to grok but it's described here: https://www.keycloak.org/docs/latest/authorization_services/index.html |
|
Is there any progress on this feature? I could help out if there is any work to do. |
|
@dereisele, we've tried to get in sync on this topic with the core flowable members, but decided to postpone this issue until after the 6.5 release and see then on how to continue on this topic. We'll forst merge everything in the upcoming 6.5 release and will pick this up again afterwards. We (inuits/flowable-engine) have a working keycloak and wso2 idm integration based on OIDC, but there are still several issues that should be covered before we get this PR accepted. There are different solutions for those issues dough, so this would require feedback from the core team to continue on the approved solution. |
|
Hi, @tijsrademakers, @VeridianDynamics, @filiphr , @tstephen , since 6.5 is out now, is there any plan to get this finished? Regards, |
|
Is this how to create
|
|
All, Sorry for the delay, but as promised we did pickup the Keycloak OIDC authentication again for 6.5.0. Functionally everything was ok immediately in 6.5.0, but it took some while to clean everything up. Since there are quite some changes since we initiated this PR, we decided to put everything together in another branch for now which starts from the current flowable/flowable-engine master. We'd like to read your comments about it and whether or not to continue in another PR? See oidc-auth branch of inuits/flowable-engine Changes/improvements compared to this PR:
The OIDC implementation uses an OpenID Connect implementation on Spring Security. This was previously also already the case. Additional properties in application.properties:
In your realm on Keycloak you should configure an additional client using following settings
The KeycloakSsoHandler will register new users in flowable-idm if they don't exist yet and update existing ones. Currently the handler gets all info from the userinfo endpoint, also tenant and privileges, but this should also be read from the JWT access token if available there; we'll improve this soon. Tenant and privileges claims are optional now, but:
We have also a branch ready with support for multiple tenants on a single user, but this has much more impact on existing code although it works nice. See tenant-mapping branch of inuits/flowable-engine Please give your advice whether to continue in this PR or create a new one |
|
@tomvda I think a new PR would be the cleanest approach, thanks. |
thanks will look at it |
|
Hello, Sorry if I am pushing, but my company is very interested in this feature. Regards |
I did a check-out of the previous commit and was working using the source code. maybe you could do similar till they |
Hi, |
You could try this too premium-minds/flowable-keycloak#1 (comment) |
|
This PR should be closed now and will continue in PR #2281 as advised above |
|
Thanks for creating the new PR, we will review it and provide feedback. |
WIP for getting external authentication in flowable. With the code as it is, it's possible to register/login with OAUTH (KeyCloak for example, but I also tested with another OAuth from a client).
Short description of what happened:
I've added an SSOHandler interface which you can inject yourself to add your custom logic/sso. This SsoHandler should provide the correct external url to the sso login page. When everything works out the SSOHandler has to return a SSOUserInfo object.
This userinfo is used to decide wether to Login the user or to register a new account for that user.
When:
Comments and remarks are super welcome, as this can be improved A LOT.
What it doesn't do (yet)