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

Support for OIDC Logout #5356

Merged
merged 4 commits into from
Mar 19, 2019
Merged

Support for OIDC Logout #5356

merged 4 commits into from
Mar 19, 2019

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented May 16, 2018

This commit introduces two new classes for coordinating logout with
an OP. The first is for the user-agent (redirect) use case and the
second is for the server-side use case.

For now they are separate due to settings that don't make sense in
one use case vs the other, however, there is some duplicative logic
that we could either clean up with an abstract class or some
additional configuration logic.

This commit doesn't propose any changes to the OAuth2LoginConfigurer
as I'd first like to get feedback on the classes themselves.

Issue: gh-5350

@jzheaux jzheaux requested review from jgrandja and rwinch May 16, 2018 20:17
@jzheaux jzheaux added status: duplicate A duplicate of another issue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed wip labels Jan 30, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Jan 30, 2019
jgrandja
jgrandja requested changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jzheaux. Please see my comments

...in/java/org/springframework/security/oauth2/client/oidc/logout/OidcLogoutSuccessHandler.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/oauth2/client/oidc/logout/OidcLogoutSuccessHandler.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/oauth2/client/oidc/logout/OidcLogoutSuccessHandler.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/oauth2/client/oidc/logout/OidcLogoutSuccessHandler.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/oauth2/client/oidc/logout/OidcLogoutSuccessHandler.java Outdated Show resolved Hide resolved
@jzheaux
Copy link
Contributor Author

jzheaux commented Mar 6, 2019

@jgrandja, thanks for all the great feedback. I've applied it and also added its reactive counterpart.

I added the reactive at this point since it seems like we are close enough on the servlet code.

Also, note that I stuck with your first suggestion of ClientInitiated - I think it lines up nicely with the spec, see how nicely it reads alongside the javadoc for that class.

Lastly, @rwinch when you get a moment I'd love to get your take on the reactive code. Of particular note is that I duplicate some functionality already found in RedirectServerLogoutSuccessHandler because I couldn't get a delegate to work.

rwinch
rwinch requested changes Mar 13, 2019
View reviewed changes
...ingframework/security/oauth2/client/oidc/logout/OidcClientInitiatedLogoutSuccessHandler.java Outdated
* limitations under the License.
*/

package org.springframework.security.oauth2.client.oidc.logout;
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this to be in the oauth2.client.web.oidc.logout package

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwinch Please see my latest comment on package name as I believe the one I suggested is correct?

...mework/security/oauth2/client/oidc/logout/OidcClientInitiatedServerLogoutSuccessHandler.java Outdated
* limitations under the License.
*/

package org.springframework.security.oauth2.client.oidc.logout;
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this to be in the oauth2.client.web.server.oidc.logout package

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwinch Please see my latest comment on package name as I believe the one I suggested is correct?

...mework/security/oauth2/client/oidc/logout/OidcClientInitiatedServerLogoutSuccessHandler.java Outdated
.map(OAuth2AuthenticationToken.class::cast)
.flatMap(this::endSessionEndpoint)
.map(endSessionEndpoint -> endpointUri(endSessionEndpoint, authentication))
.defaultIfEmpty(this.logoutSuccessUrl)
Copy link
Member

Choose a reason for hiding this comment

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

I think we could create a member (please consider renaming but illustrates point):

private ServerLogoutSuccessHandler delegate = new RedirectServerLogoutSuccessHandler();

// add setter

You can then remove this.logoutSuccessUrl and change .defaultIfEmpty(this.logoutSuccessUrl) to

.switchIfEmpty(delegate.onLogoutSuccess(exchange, authentication).then(Mono.empty()))

rwinch
rwinch approved these changes Mar 13, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I like the result you ended up with.

jgrandja
jgrandja requested changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @jzheaux. See my comments for a couple other quick changes.

...k/security/oauth2/client/web/server/oidc/logout/OidcClientInitiatedLogoutSuccessHandler.java Outdated
* limitations under the License.
*/

package org.springframework.security.oauth2.client.web.server.oidc.logout;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the Servlet version so it should not be under server sub-package.

Please move to -> org.springframework.security.oauth2.client.oidc.web.logout

...k/security/oauth2/client/web/server/oidc/logout/OidcClientInitiatedLogoutSuccessHandler.java Outdated
* @author Josh Cummings
* @since 5.2
* @see <a href="http://openid.net/specs/openid-connect-session-1_0.html#RPLogout">RP-Initiated Logout</a>
* @see {@link org.springframework.security.web.authentication.logout.LogoutSuccessHandler}
Copy link
Contributor

Choose a reason for hiding this comment

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

@link is invalid in @see

...rk/security/oauth2/client/web/oidc/logout/OidcClientInitiatedServerLogoutSuccessHandler.java Outdated
* limitations under the License.
*/

package org.springframework.security.oauth2.client.web.oidc.logout;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the Reactive version so it should be under server sub-package.

Please move to -> org.springframework.security.oauth2.client.oidc.web.server.logout

...rk/security/oauth2/client/web/oidc/logout/OidcClientInitiatedServerLogoutSuccessHandler.java Outdated
* @author Josh Cummings
* @since 5.2
* @see <a href="http://openid.net/specs/openid-connect-session-1_0.html#RPLogout">RP-Initiated Logout</a>
* @see {@link org.springframework.security.web.authentication.logout.LogoutSuccessHandler}
Copy link
Contributor

Choose a reason for hiding this comment

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

@link is invalid in @see AND class reference is Servlet version

@jzheaux jzheaux merged commit a45df2c into spring-projects:master Mar 19, 2019
@jzheaux jzheaux deleted the gh-5350 branch March 19, 2019 15:00
@jzheaux jzheaux self-assigned this Apr 15, 2019
@rwinch rwinch added the type: enhancement A general enhancement label May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

@jgrandja jgrandja jgrandja approved these changes

Assignees

@jzheaux jzheaux

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M2
Development

Successfully merging this pull request may close these issues.
3 participants
@jzheaux @rwinch @jgrandja