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.

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

Max Sessions on WebFlux #13752

Merged

Conversation

marcusdacoregio
Copy link
Contributor

@marcusdacoregio marcusdacoregio commented Aug 30, 2023

@marcusdacoregio marcusdacoregio added in: config An issue in spring-security-config type: enhancement A general enhancement labels Aug 30, 2023
@marcusdacoregio marcusdacoregio self-assigned this Aug 30, 2023
@marcusdacoregio marcusdacoregio marked this pull request as ready for review September 11, 2023 14:36
@rwinch rwinch self-assigned this Sep 25, 2023
@maradanasai
Copy link

HI @marcusdacoregio Thanks for this. Can you please take it forward?

@maradanasai
Copy link

maradanasai commented Nov 9, 2023

Can't wait for this feature release in 6.2.0

@marcusdacoregio
Copy link
Contributor Author

Hi, @maradanasai. Unfortunately, this will not make it into the 6.2 release. I will try to get it added for the first milestone of 6.3 though.

@marcusdacoregio marcusdacoregio added this to the 6.3.0-M1 milestone Nov 23, 2023
@marcusdacoregio marcusdacoregio force-pushed the gh-6192-max-sessions branch 2 times, most recently from c4ddb21 to 162fcfe Compare November 24, 2023 12:48
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) and removed in: config An issue in spring-security-config labels Nov 24, 2023
@marcusdacoregio marcusdacoregio force-pushed the gh-6192-max-sessions branch 5 times, most recently from 406b1cc to 7154041 Compare November 27, 2023 17:58
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

This looks great, @marcusdacoregio. Per your request, I've reviewed the following:

  • Interface naming; I also took the liberty to comment on some class names
  • DSL names and contracts

* @param maxSessions the {@link Function} to use
* @return the {@link ConcurrentSessionsSpec} to continue customizing
*/
public ConcurrentSessionsSpec maxSessions(Function<Authentication, Mono<Integer>> maxSessions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a Function as an API can be a bit more challenging. Have you considered having two methods:

.maxSessions(Function)

.maxSessions(Integer)

In this way, folks can supply a global max, should that be preferred, in a way that is clearly correct.

}

/**
* Sets the {@link Function} to use to determine the maximum number of
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me how I'd state that for some authentications, there is no maximum. For example, is it:

.maxSessions((authentication) -> containsSomeAuthority(authentication) ? Mono.just(Integer.MAX_VALUE) : Mono.just(3))

or something else?

(I can see in the reference that it is -1, but this may be value to place in the JavaDoc, too. Also, since it is a magic number, you might consider a static constant like UNLIMITED. This adds clarity to the user's intent and it also gives you the ability to change that value in the future should it become necessary.)

Copy link
Contributor Author

@marcusdacoregio marcusdacoregio Nov 28, 2023

Choose a reason for hiding this comment

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

Instead of the user dealing with magic numbers, I think it would be better to define a SessionLimit POJO with a SessionLimit#of(int) and SessionLimit#unlimited() static factory methods to achieve a better declarative configuration.

In addition to that, there are now maximumSessions(SessionLimit) and maximumSessions(Function<Authentication, Mono<SessionLimit>) available.

.sessionManagement((sessionManagement) -> sessionManagement
	.concurrentSessions((concurrentSessions) -> concurrentSessions
		.maximumSessions(SessionLimit.of(1))
	)
);

* @since 6.3
* @see AuthenticationWebFilter#setSessionAuthenticationStrategy(ServerSessionAuthenticationStrategy)
*/
public interface ServerSessionAuthenticationStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a new interface is necessary in this case since we already have ServerAuthenticationSuccessHandler, whose method signature is the same.

It seems like the implementations could be added to a DelegatingServerAuthenticationSuccessHandler instance. Do you try that already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've changed the implementation to use the ServerAuthenticationSuccessHandler instead of a new interface.

@marcusdacoregio marcusdacoregio force-pushed the gh-6192-max-sessions branch 3 times, most recently from e26521d to df3d7fd Compare November 29, 2023 16:43
@marcusdacoregio marcusdacoregio force-pushed the gh-6192-max-sessions branch 2 times, most recently from ddd0d82 to e3d457d Compare December 6, 2023 18:04
@marcusdacoregio marcusdacoregio added in: config An issue in spring-security-config and removed in: web An issue in web modules (web, webmvc) labels Dec 11, 2023
@marcusdacoregio marcusdacoregio merged commit 57ab151 into spring-projects:main Dec 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants