Use LaravelSessionStore in the SessionStateHandler.

[nstapelbroek]

👋

This is an implementation of the approach I pitched in #125 . Let me know what you think 😄

What has been done

1. Binded the LaravelSesisonStorage and SessionStateHandler to the container.
   a. I did this so I could re-use it when generating a paswordless state token without rebuilding what the SDK already offers for me with the callback handling :)
   b. In addition, by binding it on the StoreInterface you could now potentially overwrite the driver for this session storage 🎉
2. Moved some constructor initialisation logic out of the Auth0Service
   a. Since some dependencies are now bound to the app container. Why not pass them as params? :)
   b. Having less logic in the constructor makes it a bit less error prone and more testable.

Notes

• Sadly, I changed the constructor method signature of the Auth0Service which is public. So these changes are backwards incompatible and require a major version bump.
• I thought about moving more magic methods out of the class or cleaning it up a bit more. I opted for only changing the stuff that I directly touched.
• Let me know if you need anything changed, happy to help 😄

References

#125

Testing

Can you help me with this? I was unable to find a testing setup. Any way to test this and proof that it works? I did remove some stuff that is unrelated to the issue just because my IDE hinted that it was dead code. 🤔

[x] This change has been tested on the latest version Laravel

Checklist

[x] I have read the Auth0 general contribution guidelines

[x] I have read the Auth0 Code of Conduct

[nstapelbroek]

Some touch ups 👷

[joshcanhelp]

@nstapelbroek - Thank you for this! I have a few other changes that would require a major so let me find some time in an upcoming sprint to take a look at this.

[nstapelbroek]

@joshcanhelp awesome! Let me know if there is anything I can do to help :)

[xaoseric]

Any updates on this? Session is still broken with laravel

[joshcanhelp]

@xaoseric - Apologies for the delay here. I'll take a look next week to see if we can do this without any breaking changes and, if not, start a branch for a new major. Thank you for your patience!

[joshcanhelp]

Alright, so .... this PR looks great but definitely introduces breaking changes. We're working on a major in the PHP SDK that this plugin relies on, which will likely require some additional changes in this library. I'd like to hold off on releasing a major for this plugin before that one is done. I made a 7.0.0-dev branch to track any breaking changes as they are made.

For this PR ... could we make the arguments in that constructor default to null and set a value if nothing is passed in? Not pretty but it would make the fix available. If it's not null, use it. If it is null, set a value that works properly. I'm not seeing anything in the modified \Auth0\Login\LoginServiceProvider::register() that would break. Something like this in the constructor:

public function __construct(
    array $auth0Config = null,
    StoreInterface $sessionStorage = null,
    SessionStateHandler $sessionStateHandler = null
)
{
    if ( is_null( $auth0Config ) ) {
        $auth0Config = config('laravel-auth0');
    }

    if ( is_null( $sessionStorage ) ) {
        $sessionStorage = new LaravelSessionStore();
    }

    if ( is_null( $sessionStateHandler ) ) {
        $sessionStateHandler = new SessionStateHandler($sessionStorage);
    }

    $auth0Config['store'] = $sessionStorage;
    $auth0Config['state_handler'] = $sessionStateHandler;
    $this->auth0 = new Auth0($auth0Config);
}

All that said ... when I try out your branch locally, I get an invalid state error on login. Debugging a bit, I see that Session::get() called in LaravelSessionStore is not returning anything. Any ideas there?

[nstapelbroek]

Hi @joshcanhelp, thank you for your feedback!

If it's okay with you I would like to:

1. See if I can reproduce those invalid state errors. I only tested this setup with a password less login so I'll make sure to check this
2. Make sure the Auth0Service is BC compatible in this PR so we could release the fix
3. Since I have the hunch that testing will be easier if we move some logic out of the constructor, I'd like to recreate the BC break in a new PR against the 7.0.0-dev branch if thats okay with you.

[joshcanhelp]

See if I can reproduce those invalid state errors. I only tested this setup with a password less login so I'll make sure to check this

The invalid state errors happen outside of the login method you're using. In general, a state value is generated and stored in session by the SDK, that value is added to the link to login at Auth0, it's then added as a query parameter to the callback URL on successful login (again, regardless of method), and checked against the value stored before the redirect. When I tested, the Session::get() portion was not returning a value with this change, telling me it was either not set correctly or not retrieved correctly. Could very well be a configuration issue on my end but this is a default app with minimal changes from laravel new. Happy to help test if you give me some guidance on what to do differently.

Make sure the Auth0Service is BC compatible in this PR so we could release the fix

Would love to get this out in a minor, if possible, as we're at least a couple months away from a major here.

I'd like to recreate the BC break in a new PR against the 7.0.0-dev branch if thats okay with you.

I don't think that would help much. That branch is no different from master currently. I just pushed that up to start collecting any breaking changes (which we don't have yet).

Thank you for sticking with this! I'll monitor this thread and provide feedback as needed 👍

[nstapelbroek]

Heya 👋 , a quick follow up on the authentication flow not working when using the LaravelSessionStorage. I managed to reproduce this on my local setup.

My theory is that this happens because the Session::put call does not write the value to the storage instantly. This is handled by the framework when returning a response.
On my local setup the /login route handler ends up using the SDK to start the login. The SDK then decides to set a redirect header and exit the php execution, preventing the framework from persisting the session and writing any set-cookie headers.

I'll look into finding a fix in this package as this tends to be more of a framework specific problem. Thanks for responding and helping. I'll keep you posted!

[joshcanhelp]

@nstapelbroek - Thank you for the update! Is there anything I can do to help this move along, like testing or explaining anything? I'd like to get this released along with the Laravel 6 update.

[nstapelbroek]

@joshcanhelp As I'm slowly returning from holiday my time is becoming a bit more limited in the last couple of days, sorry about that. I'll make sure to solve the feedback in the PR before the end if the weekend. If you need it faster: feel free to make changes on the branch :)

The challenge right now is that I'm not really happy with the solution that we have in the LaravelSessionStorage (persisting the cookie storage on write). It's a bit inexplicit and by working around the framework this PR causes more edge cases than it solves. My next try would be to find a way to simply extend the Auth0 class so I can override the exit; behaviour and let the framework return a response a bit more Laravel friendly.

Happy to hear your thoughts 😄 !

[joshcanhelp]

@nstapelbroek - I appreciate your work so far!

So you're saying this is a problem with exit in the PHP SDK \Auth0\SDK\Auth0::login() method? We're going to remove that in the upcoming major but that could cause a big security issue if the exit is not added so I don't want to take that out without sufficient warning.

To be clear ... you're worried about the Session::save() call in the set() method only being needed for state but used for everything? Worst case, we could only save if we're using SessionStateHandler::STATE_NAME, that would only use it in the specific case we needed (with a TODO to change once the major is out). Or, we could do a little more work in \Auth0\Login\Auth0Service::login() to handle the login redirection in a more Laravel-friendly way?

Happy to push a change for either one if it seems like a reasonable adjustment here.

[nstapelbroek]

Good morning! 🎉

I just pushed out the requested changes. I also found a way to get this working without some of the inexplicit workarounds in this PR. Since those changes do break backwards compatibility I've submitted them in a separate PR in #140 so we can move forward here and talk about your valid raised concerns in a different PR.

[joshcanhelp]

@nstapelbroek - I did a bit of work on this and realized that we would have to make a few session-handling changes in #140 to make it work with this new method so I'm combining it all here. If this all makes sense then we can close #140.

Using the method added in auth0/auth0-PHP#371, I switched the login() method on the service to to all the auth param transforms that were there previously and use Illuminate\Http\RedirectResponse to do the actual redirect. When I do that, the Session::save() is not needed in the Laravel state handler.

I also rebased the Laravel 6 addition and made the PHP SDK minimum 5.6.0 (not yet released). If you want to test this, you'll need to pull the branch from the linked PR along with this branch.

If this looks good and works on your end, I'll merge the SDK PR, release it, approve this, and get everything released together.

Again and again ... thank you for your work on this!

[joshcanhelp]

@nstapelbroek - Sorry to bother you on this but I'd like to get this and the SDK released tomorrow. If you can take a look and make sure this looks good and works on your end, I'd appreciate it!

@xaoseric - Can you pull this branch down and see if it solves your issue?

[nstapelbroek]

Hey @joshcanhelp, Thanks for taking care! I'll do a quick test to confirm the PR in it's current state with a manual SDK of 5.6.0 works and then we can roll this out :)

[nstapelbroek]

tested this on my local setup using the normal flow and a passwordless authentication. Works here! 👍

I did notice an invalid state error but I'm a bit convinced that this is caused by me clearing cookies and removing the Laravel storage in between requests.

If I can help with anything else, let me know :)

[joshcanhelp]

PHP SDK is released, this package to follow!