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

Add Reactive One-Time Token Login support #15792

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Reactive One-Time Token Login support #15792

wants to merge 1 commit into from

Conversation

CrazyParanoid
Copy link

Closes gh-15699

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 11, 2024
@CrazyParanoid CrazyParanoid reopened this Sep 11, 2024
@marcusdacoregio marcusdacoregio self-assigned this Sep 12, 2024
@marcusdacoregio marcusdacoregio added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 12, 2024
marcusdacoregio
marcusdacoregio suggested changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Thanks @CrazyParanoid. I've left some feedback inline.

In addition to that, would you please add documentation based on the existing servlet docs? If you don't have time to do that, can you please create a ticket?

config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
@@ -244,7 +253,7 @@
* }
* }
* </pre>
*
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please not change those extra lines, paragraphs, white lines etc? They are only creating extra noise

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, fixed.

...pringframework/security/authentication/ott/reactive/InMemoryReactiveOneTimeTokenService.java Outdated
Comment on lines 70 to 72
catch (Throwable ex) {
return Mono.error(ex);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Error extends Throwable, I think catching an Exception would be better here.

config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
Comment on lines 5913 to 6054
if (this.oneTimeTokenService == null) {
this.oneTimeTokenService = new InMemoryReactiveOneTimeTokenService();
}
return this.oneTimeTokenService;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably initialize the oneTimeTokenService field with InMemoryReactiveOneTimeTokenService and remove this method

Copy link
Author

Choose a reason for hiding this comment

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

I added initialization of oneTimeTokenService with InMemoryReactiveOneTimeTokenService, but I left the getOneTimeTokenService method in order to be able to get oneTimeTokenService as a bean.

config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
Comment on lines 5932 to 5935
if (this.authenticationConverter == null) {
return new ServerOneTimeTokenAuthenticationConverter();
}
return this.authenticationConverter;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably initialize the authenticationConverter field with ServerOneTimeTokenAuthenticationConverter and remove this method

...rg/springframework/security/web/server/authentication/ott/GenerateOneTimeTokenWebFilter.java Outdated
private ServerGeneratedOneTimeTokenHandler generatedOneTimeTokenHandler = new ServerRedirectGeneratedOneTimeTokenHandler(
"/login/ott");

public GenerateOneTimeTokenWebFilter(ReactiveOneTimeTokenService oneTimeTokenService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please require the ServerGeneratedOneTimeTokenHandler in the constructor? Like a88a774

@marcusdacoregio marcusdacoregio removed their assignment Sep 17, 2024
@CrazyParanoid
Copy link
Author

Hi @marcusdacoregio. Thanks for your feedback! All your comments have been resolved.
I'll add documentation for Reactive One-Time Token Login in the next couple of days.

@CrazyParanoid
Copy link
Author

Hi @marcusdacoregio ! I have resolved all errors and conflicts and added documentation. Could you please review it?

@jzheaux jzheaux self-assigned this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcusdacoregio marcusdacoregio Awaiting requested review from marcusdacoregio

Assignees

@jzheaux jzheaux

Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
Spring Security Team
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Reactive One-Time Token Login support
4 participants
@CrazyParanoid @marcusdacoregio @jzheaux @spring-projects-issues