-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(run): add oauth2 identity provider #4570
Conversation
.../org/camunda/bpm/spring/boot/starter/security/oauth2/impl/OAuth2IdentityProviderFactory.java
Outdated
Show resolved
Hide resolved
...in/java/org/camunda/bpm/spring/boot/starter/security/oauth2/impl/OAuth2IdentityProvider.java
Show resolved
Hide resolved
When I run
Details
Am I doing anything wrong? I can send you the example. Exception
|
Am I doing anything wrong? Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see the implementation is broken with the latest pushed changes. I have 1-2 classes to review but sharing the feedback earlier to give indication of the failures see above comments.
import org.camunda.bpm.engine.impl.cfg.ProcessEngineConfigurationImpl; | ||
import org.camunda.bpm.engine.spring.SpringProcessEnginePlugin; | ||
|
||
public class OAuth2IdentityProviderPlugin extends SpringProcessEnginePlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 At the moment, I don't see the benefit but there's SpringBootProcessEnginePlugin
which the other starter plugins use. So you can use that instead for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed! 👍
Since I merged the previous PR, I will do the rebase of this one now. |
bb3bd32
to
2109336
Compare
http.authorizeHttpRequests(c -> c | ||
.requestMatchers(webappPath + "/app/**").authenticated() | ||
.requestMatchers(webappPath + "/api/**").authenticated() | ||
.anyRequest().permitAll() | ||
) | ||
.anonymous(AbstractHttpConfigurer::disable) | ||
.oauth2Login(Customizer.withDefaults()) | ||
.oidcLogout(Customizer.withDefaults()) | ||
.oauth2Client(Customizer.withDefaults()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Please note that in the previous PR #cors()
was removed but with the current changes and without it Camunda Run cannot be started when oauth2 flag is enabled. => cors
config is required.
Current error that I observe when cors line is removed:
Details
2024-09-04T15:04:30.282+02:00 ERROR 18404 --- [ main] o.s.boot.SpringApplication : Application run failed
org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'filterChain' defined in class path resource [org/camunda/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.class]: Failed to instantiate [org.springframework.security.web.SecurityFilterChain]: Factory method 'filterChain' threw exception with message: Bean named 'corsFilter' is expected to be of type 'org.springframework.web.filter.CorsFilter' but was actually of type 'org.springframework.boot.web.servlet.FilterRegistrationBean'
at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:648) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:636) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1337) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1167) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:562) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:522) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:337) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:335) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:975) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:962) ~[spring-context-6.1.10.jar!/:6.1.10]
at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:624) ~[spring-context-6.1.10.jar!/:6.1.10]
at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:146) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:754) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:456) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:335) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1363) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1352) ~[spring-boot-3.3.1.jar!/:3.3.1]
at org.camunda.bpm.run.CamundaBpmRun.main(CamundaBpmRun.java:25) ~[!/:7.22.0-SNAPSHOT]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[na:na]
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
at java.base/java.lang.reflect.Method.invoke(Method.java:568) ~[na:na]
at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:91) ~[camunda-bpm-run-core.jar:7.22.0-SNAPSHOT]
at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:53) ~[camunda-bpm-run-core.jar:7.22.0-SNAPSHOT]
at org.springframework.boot.loader.launch.PropertiesLauncher.main(PropertiesLauncher.java:574) ~[camunda-bpm-run-core.jar:7.22.0-SNAPSHOT]
Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.security.web.SecurityFilterChain]: Factory method 'filterChain' threw exception with message: Bean named 'corsFilter' is expected to be of type 'org.springframework.web.filter.CorsFilter' but was actually of type 'org.springframework.boot.web.servlet.FilterRegistrationBean'
at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:177) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:644) ~[spring-beans-6.1.10.jar!/:6.1.10]
... 26 common frames omitted
Caused by: org.springframework.beans.factory.BeanNotOfRequiredTypeException: Bean named 'corsFilter' is expected to be of type 'org.springframework.web.filter.CorsFilter' but was actually of type 'org.springframework.boot.web.servlet.FilterRegistrationBean'
at org.springframework.beans.factory.support.AbstractBeanFactory.adaptBeanInstance(AbstractBeanFactory.java:422) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:403) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:205) ~[spring-beans-6.1.10.jar!/:6.1.10]
at org.springframework.context.support.AbstractApplicationContext.getBean(AbstractApplicationContext.java:1240) ~[spring-context-6.1.10.jar!/:6.1.10]
at org.springframework.security.config.annotation.web.configurers.CorsConfigurer.getCorsFilter(CorsConfigurer.java:85) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.springframework.security.config.annotation.web.configurers.CorsConfigurer.configure(CorsConfigurer.java:73) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.springframework.security.config.annotation.web.configurers.CorsConfigurer.configure(CorsConfigurer.java:41) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.springframework.security.config.annotation.AbstractConfiguredSecurityBuilder.configure(AbstractConfiguredSecurityBuilder.java:376) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.springframework.security.config.annotation.AbstractConfiguredSecurityBuilder.doBuild(AbstractConfiguredSecurityBuilder.java:330) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.springframework.security.config.annotation.AbstractSecurityBuilder.build(AbstractSecurityBuilder.java:38) ~[spring-security-config-6.3.1.jar:6.3.1]
at org.camunda.bpm.spring.boot.starter.security.oauth2.CamundaSpringSecurityOAuth2AutoConfiguration.filterChain(CamundaSpringSecurityOAuth2AutoConfiguration.java:113) ~[camunda-bpm-spring-boot-starter-security-7.22.0-SNAPSHOT.jar:7.22.0-SNAPSHOT]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[na:na]
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
at java.base/java.lang.reflect.Method.invoke(Method.java:568) ~[na:na]
at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:140) ~[spring-beans-6.1.10.jar!/:6.1.10]
... 27 common frames omitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add a config class to the oauth2 module that disables cors since otherwise it clashes with runs CorsFilter:
@Bean
public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
http.cors(AbstractHttpConfigurer::disable);
return http.build();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed! 👍
Spring Boot Starter
This is expected. It is Spring Boot's default behavior. We decided not to apply any customizations.
When you use the Spring Boot Starter, you have the power. The default configuration is used, and it might clash with OAuth2. We didn't aim for a great getting started experience for the Spring Boot Starter since the focus is clearly on Run. We could create a follow-up ticket to improve this. Run
I think when using the |
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java
Show resolved
Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java
Show resolved
Hide resolved
I performed additional testing with the focus on Camunda Run. It's seems working as expected if the cors line is added and @ConditionalOnProperty is adjusted (so the enabling and disabling the identity provider is working). Other then that Camunda Run is looking good. |
I thought I did.
@Bean
public FilterRegistrationBean processEngineAuthenticationFilter() {
FilterRegistrationBean registration = new FilterRegistrationBean();
registration.setName("camunda-auth");
registration.setFilter(getProcessEngineAuthenticationFilter());
registration.addInitParameter("authentication-provider",
"org.camunda.bpm.engine.rest.security.auth.impl.HttpBasicAuthenticationProvider");
registration.addUrlPatterns("/*");
return registration;
}
@Bean
public Filter getProcessEngineAuthenticationFilter() {
return new ProcessEngineAuthenticationFilter();
}
Then I tested with basic auth both scenarios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Tested Camunda Run with oauth2.identity-provider.enabled: true
and it works as we agreed.
I checked the created follow ups and they make sense.
Let's merge for the alpha5.
I will follow up with @danielkelemen and QA on testing and next tasks.
related to #4453