-
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(oauth2): add sso logout support #4604
Conversation
I added some details in the corresponding issue: #4455 |
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.
Added a question and a suggestion to plugin.js
...ot-starter/starter-security/src/main/resources/plugin-webapp/sso-logout-plugin/app/plugin.js
Outdated
Show resolved
Hide resolved
...ot-starter/starter-security/src/main/resources/plugin-webapp/sso-logout-plugin/app/plugin.js
Outdated
Show resolved
Hide resolved
4548c10
to
f51fb32
Compare
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.
👍 Looks good. I have only improvement suggestion for the logging.
Tested Camunda Run + okta with enabled and disabled sso logout.
...da/bpm/spring/boot/starter/security/oauth2/CamundaSpringSecurityOAuth2AutoConfiguration.java
Outdated
Show resolved
Hide resolved
/** | ||
* Enable SSO Logout. Default {@code {baseUrl}}. | ||
*/ | ||
private String postLogoutRedirectUri = "{baseUrl}"; |
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.
✍️ In docs, we need to add that this must be provided if sso-logout is enabled. At least for Okta, this property is mandatory.
/** | ||
* Enable SSO Logout. Default {@code false}. | ||
*/ | ||
private boolean enabled = false; |
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.
✍️ We need to document this and default value.
|
||
var validateTokenFilter = new AuthorizeTokenFilter(clientManager); |
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.
🔧 Refactored this so it uses the same @Bean
structure as other classes.
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.
👍
#4455