Make sure we do not generate a new state id

[xaoseric]

If we already have state id, we should not be generating one, can lead to Invalid state error in laravel.

Changes

• SessionStateHandler class - In the issue method, we check if there is already one created to avoid an invalid state exception in laravel.

References

auth0/laravel-auth0#141 (comment)

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

This can be validated by following the tutorial on laravel integration and adding the fix.

[ ] This change adds test coverage

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

Checklist

[x] I have read the Auth0 general contribution guidelines.

[x] I have read the Auth0 Code of Conduct.

[x] All existing and new tests complete without errors.

[x] The correct base branch is being used.

[xaoseric]

@joshcanhelp if you wanted to go further with it, you could add a $force = false to the method and if force is not false generate a new uniqid. ex:

$state = $this->store->get(self::STATE_NAME);
if ($state === null || $force) {
     $state = uniqid('', true);
     $this->store($state);
}

[joshcanhelp]

@xaoseric - Thanks for this! It looks like this is still in motion ... should I wait to review?

[xaoseric]

@joshcanhelp - It is finished, allowed for it to be forced by adding true, it is false by default and will only generate if it doesn’t already exist in the session. Also added a getSession and a getSessionHandler method so third party developers can add a try catch and manually issue a new uniqid if desired.

[joshcanhelp]

Thanks again for the contribution here!

I'll respond to your thread in the Laravel repo but, in speaking just about the SDK here ... the "id" that we're generating here is a one-time use string. I'm not totally sure why we'd ever need to keep that around.

[joshcanhelp]

The master branch here targets 5.5, so no return type hinting. We are working on a major in the 7.0.0-dev branch but if you want these methods out before then, they'll have to be 5.5 compatible.

[xaoseric]

removed the return type hinting

[joshcanhelp]

When would a developer need to use $force?

[xaoseric]

if they want to implement a try catch for the getUser method, and want to redo the login if the user is null, that would be a good use case.

[xaoseric]

I'll respond to your thread in the Laravel repo but, in speaking just about the SDK here ... the "id" that we're generating here is a one-time use string. I'm not totally sure why we'd ever need to keep that around.

In the getLoginUrl function you have

        if (empty( $auth_params['state'] )) {
            $auth_params['state'] = $this->stateHandler->issue();
        } else {
            $this->stateHandler->store($auth_params['state']);
        }

In the exchange method you have

        $state = $this->getState();
        if (! $this->stateHandler->validate($state)) {
            throw new CoreException('Invalid state');
        }

and the getState method is

protected function getState()
    {
        $state = null;
        if ($this->responseMode === 'query' && isset($_GET['state'])) {
            $state = $_GET['state'];
        } else if ($this->responseMode === 'form_post' && isset($_POST['state'])) {
            $state = $_POST['state'];
        }
        return $state;
    }

/**
     * Perform validation of the returned state with the previously generated state.
     *
     * @param string $state
     *
     * @return boolean
     */
    public function validate($state)
    {
        $valid = $this->store->get(self::STATE_NAME) == $state;
        $this->store->delete(self::STATE_NAME);
        return $valid;
    }

If your not storing the state then why are you validating it against the stored state?

[joshcanhelp]

@xaoseric - Sorry, I don't think my question was clear. We do need to store that for the round-trip to Auth0 to log in. But the generated state value is more-or-less a nonce, not a value we need if we never use it.

I understand that this might be solving an issue in the Laravel library but I can't think of a reason why we need to re-use (as in, return it from issue()) rather than re-generate.

[joshcanhelp]

@xaoseric - Just checking back in here. You mentioned in a few threads that this change solved an issue for you but I'd like to figure out the root cause there before we merge this change in. The issue() method is only called when an authorize URL is generated and the store is updated at the same time, I'm not sure why this would change anything. I'm hesitant to make a change to existing code that's working when we're not clear on what the fix is.

[joshcanhelp]

@xaoseric - Sorry for all the back and forth here.

Since this is going into the 5.x version and how this is handled changes in 7.x, I'm ok with merging this in. Two things:

• Why are we exposing the store and state hanlder?
• Need tests for the changes to issue()

[joshcanhelp]

Why is this included as part of this changeset? Would rather not expose internals if it's not necessary.

[xaoseric]

The storeinterface and the statehandler getters? or the debugger? The debugger one was there before.

[joshcanhelp]

Just asking about the code you're adding

[joshcanhelp]

No activity.

[torbentschechne]

@joshcanhelp What's the status here - this has not been merged and released yet. Why not? I receive "Invalid State" Exception is all of my Laravel Applications and look forward to have this issue solved.

[joshcanhelp]

@Elbsurfer - See my comments above as to why this approach was not merged. We need to figure out the core reason why your application is not able to validate state before we make a change to this library. Also, since you're mentioning Laravel, it might be an issue with that module or this one, hard to say without a root cause.

That said ... we're working on a major release for the Laravel plugin that pulls in the latest version of this SDK and will include changes to how state is handled. Once this PR gets merged:

auth0/laravel-auth0#162

... you can try out the 7.0.0-dev branch there and see if it solves your problem.

Otherwise, we'll need some way to reproduce your state issue in a way we can dig into the problem. If you have a sample repo of some kind that we can pull down, spin up and test, that would be helpful.

[torbentschechne]

@joshcanhelp

I am coming back to this issue... We still facing these issues, even with a new version. Can I contact you via e-Mail to give you access to an example repo?

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.