Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(run): add spring security oauth2 integration #4481

Merged
merged 2 commits into from
Sep 4, 2024
Merged

feat(run): add spring security oauth2 integration #4481

merged 2 commits into from
Sep 4, 2024

Conversation

danielkelemen
Copy link
Member

related to #4452

@danielkelemen danielkelemen added the ci:no-build Prevents any CI stage from running. label Jul 5, 2024
@danielkelemen danielkelemen self-assigned this Jul 5, 2024
@danielkelemen danielkelemen mentioned this pull request Jul 5, 2024
Closed
@danielkelemen danielkelemen force-pushed the 4451-run-boot-sso-modules branch 2 times, most recently from 27fc72c to 05ae585 Compare July 5, 2024 16:06
@danielkelemen danielkelemen force-pushed the 4452-starter-security-integration branch from 3499802 to 1cfa56e Compare July 5, 2024 16:17
@danielkelemen danielkelemen force-pushed the 4451-run-boot-sso-modules branch from 05ae585 to c4cf52a Compare August 22, 2024 15:12
@danielkelemen danielkelemen force-pushed the 4452-starter-security-integration branch 2 times, most recently from 931d8c7 to 4f1ca56 Compare August 29, 2024 12:41
@danielkelemen danielkelemen added ci:run Runs the integration tests for the Run distribution. ci:spring-boot Runs the integration tests for the Spring Boot starter. and removed ci:no-build Prevents any CI stage from running. labels Aug 29, 2024
@danielkelemen danielkelemen force-pushed the 4452-starter-security-integration branch from 4f1ca56 to 0058ff8 Compare August 29, 2024 14:19
Base automatically changed from 4451-run-boot-sso-modules to master August 29, 2024 14:41
@danielkelemen danielkelemen force-pushed the 4452-starter-security-integration branch from 0058ff8 to ca23804 Compare August 29, 2024 14:43
yanavasileva
yanavasileva reviewed Aug 30, 2024
View reviewed changes
Copy link
Member

@yanavasileva yanavasileva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not done with the review, I just wanted to share my questions so we can meet to discuss them.

.../org/camunda/bpm/spring/boot/starter/security/oauth2/impl/ClientsNotConfiguredCondition.java Show resolved Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
yanavasileva
yanavasileva requested changes Sep 2, 2024
View reviewed changes
Copy link
Member

@yanavasileva yanavasileva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review is done. I added the notes of our call.
I did some testing of starter-security and Camunda Run and everything worked as specified in the ticket.

...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java Outdated Show resolved Hide resolved
@danielkelemen
Copy link
Member Author

In all call, we discussed that maybe /api/* should be added here as well. In my test without it, everything looked ok. However, could you check on your side as well?

Yes, It should be added. It still works without it because Spring Security filter chain secures /api/* too and usually a /app request happens earlier due to page load that configures Camunda Auth. If I just call /api without previous login, then it gives unauthorized, even if I log in because the filter doesn't configure the Camunda auth for those URLs.

yanavasileva
yanavasileva approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@yanavasileva yanavasileva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Tested with

  • webapp-ee + spring-security
  • Camunda Run EE

@yanavasileva
Copy link
Member

Merging as CI is green and review is done.

@yanavasileva yanavasileva merged commit b53ad55 into master Sep 4, 2024
2 checks passed
@yanavasileva yanavasileva deleted the 4452-starter-security-integration branch September 4, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanavasileva yanavasileva yanavasileva approved these changes

Assignees

@danielkelemen danielkelemen

Labels
ci:run Runs the integration tests for the Run distribution. ci:spring-boot Runs the integration tests for the Spring Boot starter.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielkelemen @yanavasileva