Clean Up POST Parameters Code and OAuth2PKCEClient Code

[brianfreytag]

From what I'm seeing, PR #463 cleans up a deprecation in the Request class (https://symfony.com/blog/new-in-symfony-7-4-request-class-improvements) by swapping $this->getCurrentRequest()->get('state'|'code') with $request->query->get('state'|'code'). This, in turn, removed the ability for the state to be passed via POST. This became an issue for @benndt in #468 who patched it in #469.

He was using the "fix" in #463 that defined $request at the top of getAccessToken() and passing the object as an argument in getRequestParameter() method. We can just define $request in that method.

We can also use a ternary operator rather than an if block to clean this up a bit.

The changes in OAuth2PKCEClient are similar improvements. There is no reason to inject the RequestStack into both OAuth2Client and OAuth2PKCEClient to get the session when the session exists in the parent. Just needed to update the getSession in OAuth2Client to protected. It will handle the test to make sure that the request exists and that the session exists in the parent.

[bocharsky-bw]

Great improvement, thank you!

I'm just not sure about that getSession(), before we had 2 different error messages but if we share it the way you did it - we will have only one error message, which will be confusing to see in the OAuth2PKCEClient because we don't work with state but with pkce_code_verifier there. So, I would better some duplications as it was before in favor of clarity, unless we have any other solutions to have those separate error messages for different cases

[brianfreytag]

@bocharsky-bw I don't disagree and will admit it gave me initial pause, so you're not the only one thinking it.

I definitely don't like the idea of the need to inject the RequestStack into Oauth2PKCEClient simply to have a different error message, though. While RequestStack is lighter than it used to be, the redundancy could be a minor performance hinderance.

A try/catch block and return the same exception with a different message in the PKCE client? Or passing an optional boolean argument into getSession() that modifies the message? Let me think about that for a minute and I'll push up a change.

[brianfreytag]

@bocharsky-bw Force pushed the updates. Please review. I went with the optional boolean argument in getSession() and cleaned up getAccessToken() in the PKCE client to define the session at the top since it's used three times throughout the method and will be easier to read than passing the argument each time $this->getSession() is called.

[brianfreytag]

@bocharsky-bw I just realized something as I was making the changes... I didn't technically need to make these changes:

getCurrentRequest in OAuth2Client already throws a \LogicException that has the There is no "current request", and it is needed to perform this action. OAuth2PKCEClient injected the RequestStack then did $this->requestStack->getRequest() and if the request didn't exist, it threw the exact same exception that the getCurrentRequest on the parent already does. If the Request exists, then it simply returns the session without checking that the session existed to error out there:

 /**
     * @return SessionInterface
     *
     * @throws \LogicException          When there is no current request
     * @throws SessionNotFoundException When session is not set properly [thrown by Request::getSession()]
     */
    protected function getSession()
    {
        $request = $this->requestStack->getCurrentRequest();

        if (!$request) {
            throw new \LogicException('There is no "current request", and it is needed to perform this action');
        }

        return $request->getSession();
    }

This getSession() replicates what its parent getCurrentRequest does already:

/**
     * @return Request
     */
    private function getCurrentRequest()
    {
        $request = $this->requestStack->getCurrentRequest();

        if (!$request) {
            throw new \LogicException('There is no "current request", and it is needed to perform this action');
        }

        return $request;
    }

I think this is why my pause on the OAuth2Client::getSession() didn't stop me. That error message already exists in the case it was used in the parent, and it already uses $this->getCurrentRequest() and would throw that exception there:

/**
     * @return SessionInterface
     */
    private function getSession()
    {
        if (!$this->getCurrentRequest()->hasSession()) {
            throw new \LogicException('In order to use "state", you must have a session. Set the OAuth2Client to stateless to avoid state');
        }

        return $this->getCurrentRequest()->getSession();
    }

I think it may be advisable to leave the original getSession() as it is in main but make a more generic, but helpful, error message.

Perhaps instead of In order to use "state", you must have a session. Set the OAuth2Client to stateless to avoid state we could do something like, A session is required but is not found. If you are attempting to use a stateless request, use OAuth2Client::setAsStateless()?

TL;DR: The "different error messages" concern doesn't actually exist since OAuth2Client::getCurrentRequest() throws the exact same message if the Request doesn't exist just as it does in OAuth2PKCEClient::getSession() but the PKCE client doesn't even check if the session exists before returning it which is a different issue altogether. Maybe we can just change the error message on OAuth2Client::getSession() to something that can work for both Clients.

Thoughts?

[brianfreytag]

As I think about it more, it would make sense to

A) Check that a request exists and throw an exception if it doesn't in every situation
B) Provide specific error messages for the two clients

I'll push something up shortly.

[brianfreytag]

@bocharsky-bw This should handle everything.

[bocharsky-bw]

Nice code improvements, Brian!

And thank you for polishing it with several iterations :)

[brianfreytag]

@bocharsky-bw My pleasure. Thanks for all of the incredible suggestions.