Skip to content

Add support for One-Time Token Authentication #15492

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
Merged
merged 1 commit into from
Sep 3, 2024
Merged

Add support for One-Time Token Authentication #15492

merged 1 commit into from
Sep 3, 2024

Conversation

marcusdacoregio
Copy link
Contributor

@marcusdacoregio marcusdacoregio commented Jul 29, 2024 •
edited
Loading

Run ./gradlew publishToMavenLocal and try the Magic Link sample: https://github.com/marcusdacoregio/passwordless-spring-security

@marcusdacoregio marcusdacoregio self-assigned this Jul 29, 2024
@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch 3 times, most recently from 92e918a to c70b2b2 Compare August 12, 2024 12:42
@marcusdacoregio marcusdacoregio marked this pull request as ready for review August 12, 2024 12:43
@marcusdacoregio marcusdacoregio added in: config An issue in spring-security-config type: enhancement A general enhancement labels Aug 12, 2024
@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch 4 times, most recently from 3c46abf to ac04741 Compare August 12, 2024 18:08
sjohnr
sjohnr reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

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

@marcusdacoregio this looks great! I had a few minor comments for your consideration.

@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch from ac04741 to 74dcab3 Compare August 14, 2024 20:22
@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch 3 times, most recently from fdb949c to dfbe909 Compare August 20, 2024 12:44
jzheaux
jzheaux requested changes Aug 22, 2024
View reviewed changes
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 is so cool, @marcusdacoregio! I've left my feedback inline.

docs/modules/ROOT/pages/servlet/authentication/onetimetoken.adoc Outdated
// ...
.formLogin(Customizer.withDefaults())
.oneTimeTokenLogin((ott) -> ott
.showSubmitPage(false)
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 this would benefit from a more complete example, given that they will need to allow /my-ott-submit in authorizeHttpRequests. Also, a statement that this doesn't change the POST location.

...ingframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurer.java Outdated
* configured.
* @param show
*/
public OneTimeTokenLoginConfigurer<H> showSubmitPage(boolean show) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably call this generateSubmitPage or maybe showGeneratedSubmitPage (or showDefaultSubmitPage)

...ingframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurer.java Outdated
* Defaults to {@code POST /ott/generate}.
* @param generateUrl
*/
public OneTimeTokenLoginConfigurer<H> generateUrl(String generateUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could benefit from being called authenticationRequestUrl since it's conceptually similar to OAuth and SAML RP-initiated flows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it feels odd to me that we'd depart from referring to this as an "authentication request", but then not depart from "loginProcessingUrl". Either they should both be contextual to this auth mechanism (generateTokenUrl and tokenProcessingUrl) or they should both align with other auth mechs (authenticationRequestUrl and loginProcessingUrl).

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 prefer to use naming that matches OTT rather than borrowing from other stack's terminology. The URL is not an authentication request. It requests generating a OTT and maps to OneTimeTokenService.generate, so I prefer this name.

Copy link
Contributor

@jzheaux jzheaux Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In that case, I think the PR would improve by consistently avoid referring to it as an authentication request. Some examples are:

Most of these are minor cleanups except for the last one, and so I've bolded it.

I think the conversation about using OTT terminology for loginProcessingUrl is still a good one to have.

rwinch
rwinch requested changes Aug 23, 2024
View reviewed changes
...ingframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurer.java Outdated
* Defaults to {@code POST /ott/generate}.
* @param generateUrl
*/
public OneTimeTokenLoginConfigurer<H> generateUrl(String generateUrl) {
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 prefer to use naming that matches OTT rather than borrowing from other stack's terminology. The URL is not an authentication request. It requests generating a OTT and maps to OneTimeTokenService.generate, so I prefer this name.

...ngframework/security/web/authentication/ott/RedirectGeneratedOneTimeTokenSuccessHandler.java Outdated
* @author Marcus da Coregio
* @since 6.4
*/
public final class RedirectGeneratedOneTimeTokenSuccessHandler implements GeneratedOneTimeTokenSuccessHandler {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how useful this is since nothing is done with the OneTimeToken. How would the user receive the token to consume it?

Perhaps a more useful implementation would be to set the OneTimeToken on a request attribute and then forward to a new URL. We could provide Spring MVC argument resolver to resolve the token to simplify processing this in Spring. I think all of this could be done in a future iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about a forward tho. If we forward from POST /ott/generate to /login/ott it will be a POST request, not a GET, I'm not sure how useful that is. Maybe the default implementation could store the OneTimeToken in the session and then redirect to the location.

Copy link
Member

@rwinch rwinch Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the location that receives OTT would be in charge of doing something with it (e.g. sending it in an email, sending in a text message, placing it in session, etc) and then it would redirect to a new URL for the success page.

I'd like to reiterate that this can be done in a future iteration and does not need to be done now.

Copy link
Contributor Author

@marcusdacoregio marcusdacoregio Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, we can continue the discussion on #15697

...ork/security/web/authentication/ott/RequestParameterOneTimeTokenGenerateRequestResolver.java Outdated
* @author Marcus da Coregio
* @since 6.4
*/
public final class RequestParameterOneTimeTokenGenerateRequestResolver implements OneTimeTokenGenerateRequestResolver {
Copy link
Member

Choose a reason for hiding this comment

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

Like the corresponding interface, I'm not sure that this is needed today.

@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch from dfbe909 to f98f133 Compare August 27, 2024 12:54
This was referenced Aug 27, 2024
Open
Closed
@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch 2 times, most recently from bd30072 to 2d33d68 Compare September 3, 2024 12:26
@marcusdacoregio marcusdacoregio force-pushed the gh-15114-passwordless-ott branch from 2d33d68 to 28485be Compare September 3, 2024 12:46
@marcusdacoregio marcusdacoregio merged commit 00e4a8f into spring-projects:main Sep 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch requested changes

@jzheaux jzheaux jzheaux requested changes

+1 more reviewer

@sjohnr sjohnr sjohnr approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@marcusdacoregio marcusdacoregio

Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcusdacoregio @rwinch @jzheaux @sjohnr