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

Require a logged in user to resolve an authorization request #196

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

ajgarlag
Copy link
Contributor

@ajgarlag ajgarlag commented Aug 16, 2024

Introduces BC breaks

Fix #195

@ajgarlag ajgarlag changed the title Require a logged in user to resolve an authoration request Require a logged in user to resolve an authorization request Aug 16, 2024
@ajgarlag
Copy link
Contributor Author

@chalasr Can your review this? Thanks!

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Let's merge for now and reconsider in case the BC break appears to be too annoying.

@chalasr
Copy link
Member

chalasr commented Aug 26, 2024

Thank you @ajgarlag.

@chalasr chalasr merged commit 44272ff into thephpleague:master Aug 26, 2024
20 checks passed
@ajgarlag
Copy link
Contributor Author

@ajgarlag ref #200 :) im wondering if the runtimeexception is reasonable

I was unsure too. Do you think a logicexception is more reasonable here?

@ro0NL
Copy link

ro0NL commented Oct 15, 2024

@ajgarlag figured the config is required

i was more thinking about throwing UnauthorizedHttpException to avoid the 500 response

but then the issue may not be noticed in logs 🤔

@ajgarlag
Copy link
Contributor Author

IMO the UnauthorizedHttpException is not a good option.

Since 0.9.0 a logged-in user is required to authorize the request, so the error must be explicit and force the modification of the security config.

@Mika56
Copy link

Mika56 commented Nov 15, 2024

Hello,
Before this change, it was possible to store information from authorization request in session before redirecting to the login form manually:

<?php
#[AsEventListener(OAuth2Events::AUTHORIZATION_REQUEST_RESOLVE, method: 'onAuthorizationRequestResolve')]
class AuthorizationRequestResolve
{

    public function __construct(
        private readonly RequestStack $requestStack,
        private readonly UrlGeneratorInterface $urlGenerator
    )
    {
    }

    public function onAuthorizationRequestResolve(AuthorizationRequestResolveEvent $event): void
    {
        if (null !== $event->getUser()) {
            $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED);

            return;
        }

        $request = $this->requestStack->getMainRequest();
        $session = $request->getSession();
        $session->set('_security.api_authorize.client_id', $event->getClient()->getIdentifier());
        $session->set('_security.api_authorize.target_path', $request->getUri());

        $event->setResponse(new RedirectResponse($this->urlGenerator->generate('api_authorize_login')));
    }

}

This is now impossible because the authorization_request_resolve event is not triggered anymore, since the RuntimeException breaks the flow and stops the event from being created.

Is this an unwanted side-event, or was it not supported?

My use case is not clear in the code, but we're customizing the login form depending on the client_id.

@Mika56
Copy link

Mika56 commented Nov 15, 2024

Please disregard my last comment, it looks like it can be handled nicely with a custom entry point: https://symfony.com/doc/current/security/access_denied_handler.html#customize-the-unauthorized-response

@chalasr
Copy link
Member

chalasr commented Nov 16, 2024

Thanks for sharing your alternative @Mika56, looks good 👍

@ajgarlag ajgarlag deleted the require-user branch November 22, 2024 09:12
@Payn
Copy link

Payn commented Nov 22, 2024

Hello,
I'm using this bundle for communication between multiple backends, triggered by external requests. There is no logged in user in those cases.
Is there any way to make it work after 0.9?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require an UserInterface instance before resolving an AuthorizationRequest
5 participants