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

Feature Request: Support for setIntendedUrl() #357

Closed
watercayman opened this issue Apr 6, 2023 · 4 comments · Fixed by #364
Closed

Feature Request: Support for setIntendedUrl() #357

watercayman opened this issue Apr 6, 2023 · 4 comments · Fixed by #364
Assignees
Labels
Scope: Improvement Proposed improvements, or feature additions.

Comments

@watercayman
Copy link

watercayman commented Apr 6, 2023

SDK Version

7.5.1

PHP Version

PHP 8.2

Composer Version

2.x

What happened?

It looks like the base \Auth0\Laravel\Contract\Http\Middleware\Stateful\Authenticate has been modified. Thus the custom middleware that I've written breaks. It seems to get stuck in a redirect loop. Re-writing the custom middleware to match the new SDK version does fix the issue. I didn't see this in the upgrade guide tho.

Reason for custom middleware: I need to redirect based on an incomming url, like from an email. I used @laurensgroeneveld 's excellent solution from last year: #272 and simply added a line redirect()->setIntendedUrl(url()->full()); to the original middleware.

The only change I'm making is:

  if (null !== $credential) {
        if ('' === $scope || $guard->hasScope($scope, $credential)) {
            $guard->login($credential, Guard::SOURCE_SESSION);

            return $next($request);
        }

        abort(Response::HTTP_FORBIDDEN, 'Forbidden');
    }else {
        // CUSTOM CODE TO ALLOW FOR URLS FROM EMAIL OR TOKENS
        redirect()->setIntendedUrl(url()->full());
    }

@evansims To prevent breaking changes in future, would it be possible to add that setIntendedUrl() line into the SDK's middleware? I think this aligns with expected behavior for Laravel, and would then eliminate the need for custom middleware (for me at least) all together.

How can we reproduce this issue?

Create custom middleware based on 7.4 and update to 7.5. Try to log visit pages.

Additional context

No response

@evansims evansims self-assigned this Apr 10, 2023
@evansims
Copy link
Member

Hi @watercayman 👋 Sorry to hear your middleware was broken in the process. I'm not sure why any changes within our middleware would have that effect on your own, given that our middleware is final to permit internal API changes without risking breaking changes on developers.

If you were hooking into the deeper APIs of the SDK's Guard itself in your middleware, that would make sense, given we had to make some pretty big API changes there for Laravel 10 support. We do try to avoid any such changes, and thankfully, we wouldn't expect any other challenges that might cause boat-rocking in the foreseeable future.

To prevent breaking changes in future, would it be possible to add that setIntendedUrl() line into the SDK's middleware?

I am extremely hesitant to introduce this, simply because it is ripe with opportunity for inadvertently opening up redirect vulnerabilities in developer's applications.

Let's figure something out, though. Would you mind giving me an example workflow and breakdown of what your application is doing in your custom middleware, so I can better understand? Auth0 Passwordless should be fully functional with our current middleware, unless I'm misunderstanding what the emails you're referring means.

@evansims evansims added the Scope: Improvement Proposed improvements, or feature additions. label Apr 10, 2023
@evansims evansims changed the title Custom Middleware breaks Feature Request: Support for setIntendedUrl() Apr 10, 2023
@watercayman
Copy link
Author

Hi @evansims, many thanks for getting back to me. Yes, our middleware is a copy of the SDK's with that one additional line added as an else to make sure we trap the intended route before the user is logged in (so that that generator gives us the route in our app prior to authentication succeeding, rather than giving us /login)

Very very glad to hear you don't expect boat-rocking in future - still have shivers thinking about the effort to change from V6 to V7 ;)

I can understand your hesitancy over adding something like this to the SDK. For my better understanding, please can you clarify - you are worried that capturing the intended url prior to authenticating would produce a security vulnerability, or some type of unintended loop, or? I think this is expected behavior from Laravel's base auth natively, and thus not sure how this would be different from using the built-in mechanism.

For our apps, we have a number of times when we need to grab that intended url to make a mostly frictionless entry to users. Some require a token to match an email, some are general links. The key is that all users to enter any of our apps must have a user created within our app. A 'Laravel' user.

A typical example might be: A customer in our app (a user who has paid for x within the app) may wish to have 50 of their staff take training that exists within our app. This customer is able to forward a link that they purchase within our app to up to 50 emails. In those emails there is a link to a specific page(s) https://ourApp/token/area/id or similar. As the staff come in, our app tracks their progress by user and is able to display this back to the customer based on the user success of the training. We need the staff to be users within our system to track, thus need them to sign up with Auth0 to get in to our app, which then immediately creates a user and assigns permissions to the training and the lobby / other sales areas within the app. Same logic applies to a link sent to existing users - if they click the link and then sign in via Auth0, they won't end up on the intended page, rather they end up in the landing or whatever Auth0 connection we've set.

The ability to create custom middleware totally solves this, and works very well. The reason for the feature request is that the only reason we currently need that custom middleware is just to assign the intended url - if that were part of the SDK, or there is another method to do this, we wouldn't need to maintain the middleware.

@evansims
Copy link
Member

Hey @watercayman 👋 Thanks for clarifying! I misunderstood your initial request to mean you wanted to accept URLs as a parameter in the middleware to define an intended URL redirect from, which would have been a security concern. I get your intention here now, and it makes perfect sense.

I'm preparing a PR to add this feature now.

@watercayman
Copy link
Author

Hi @evansims, just confirming this is working as intended via 7.6.0. Nice work; this gets us out of having to maintain that custom middleware, and thus if changes are made in future, one less thing we depend on.

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Improvement Proposed improvements, or feature additions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants