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

IBX-8290: Reworked REST authentication to comply with the new Symfony authenticator mechanism under separate firewall #98

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

konradoboza
Copy link
Contributor

@konradoboza konradoboza commented May 28, 2024

🎫 Issue IBX-8290

Related PRs:

Description:

This PR is about reworking REST API authorization to comply with the new Symfony authenticator mechanism. Below I describe my reasoning and technical details under the hood.

1. How was it done till today?

Lots of code around REST authorization orbits around concept that REST API requests are handled by the same ibexa_front firewall which is used for the regular authorization within Ibexa DXP. This doesn't seem to be necessary so basic idea is to move it to a dedicated firewall - ref: ibexa/recipes-dev#121.

2. What is the idea?

My main focus was to:
a) keep existing authorization calls (POST /user/sessions) in-tact,
b) switch to a separate firewall
c) use built-in authenticator (tried with json_login ref: https://symfony.com/doc/5.x/security.html#json-login) but it proved to not work correctly and limited us to use JSON-based credentials only.

Keeping all that in mind I decided to implement a custom authenticator (Ibexa\Rest\Security\Authenticator\RestAuthenticator) which from now on will be referenced in our security configuration. It tries to mimic the behavior coming from the old src/lib/Server/Security/RestAuthenticator.php but with some caveats.

3. Why removing the old authenticator?

There are handful of reasons:

  • the base for this class was Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface which doesn't seem to be needed at all given we switch to the Symfony way of performing authorization,
  • it manually dispatches InteractiveLoginEvent which doesn't seem to be needed - our custom authenticator implements Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface which does that under the hood, ref: https://symfony.com/doc/5.x/security.html#other-events,
  • it performs logout operation which is based on already deprecated Symfony\Component\Security\Http\Logout\SessionLogoutHandler - the new authenticator handles that properly.

4. How to hook into the new authorization system?

My idea is to have it triggered automatically only on requests to POST /user/sessions endpoint, like it was done before. It's implemented via public function supports(Request $request): ?bool to be invoked whenever proper _route attribute is present (ibexa.rest.create.session).

Since it's out own implementation, we have a full control over how it's triggered ergo we can change it to be more robust or put some extension point in place.

Other important changes:

  • I got rid of ibexa_rest_session: ~ firewall config key alongside src/bundle/DependencyInjection/Security/RestSessionBasedFactory.php since (to the best of my knowledge) its main purpose was to customize form login and allow REST authorization to reside on the same firewall as the regular authorization,
  • I removed deprecated methods coming from src/bundle/EventListener/CsrfListener.php,
  • I removed Ibexa\Rest\Server\Security\RestLogoutHandler based on the deprecated Symfony mechanism,
  • I improved strict typing whenever id made sense,
  • I got rid of most of the class_alias entries inside the classes affected by this PR,
  • I improved readability for tests/bundle/EventListener/EventListenerTest.php and its derivatives,
  • I removed unused tests for the removed classes,
  • lots of PHPStan baseline entries were fixed on miscellaneous occasions while working on this PR. 😉

Open questions:

  • should RestAuthenticator be triggered under some other conditions rather than by analyzing request headers? reworked to be triggered only on ibexa.rest.create_session matched route,
  • whenever wrong credentials are sent, the response code is now 500 with some generic Symfony message - should we adjust it to the previous 401 instead? we should (after team sync)

For QA:

Sanities on all the REST API endpoints might become necessary, especially on those around authorization except JWT - it will be re-implemented in the next steps.

Documentation:

@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch 5 times, most recently from 36ff001 to 9939569 Compare May 28, 2024 13:59
@konradoboza konradoboza changed the title IBX-8290: Reworked REST authentication to comply with the new Symfony authenticator mechanism under separate firewall [REMOVE-DEV-DEPENDENCY] IBX-8290: Reworked REST authentication to comply with the new Symfony authenticator mechanism under separate firewall May 28, 2024
@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch 8 times, most recently from 9cd39ca to 83ce0fc Compare June 3, 2024 13:28
@konradoboza konradoboza marked this pull request as ready for review June 3, 2024 13:28
@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch 2 times, most recently from 946ea79 to 363f36d Compare June 3, 2024 13:55
@konradoboza konradoboza added the Feature New feature request label Jun 4, 2024
@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch 3 times, most recently from 1e30962 to 292d7dc Compare June 4, 2024 08:15
@konradoboza konradoboza requested a review from a team June 4, 2024 12:15
@konradoboza konradoboza added the Doc needed The changes require some documentation label Jun 5, 2024
@konradoboza
Copy link
Contributor Author

After internal sync, I improved UnauthorizedException by introducing a dedicated class and fixing existing usage, ref: 283bef7.

@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch from 283bef7 to 9c8f8c2 Compare June 7, 2024 10:37
@konradoboza konradoboza force-pushed the ibx-8290-re-implement-rest-auth branch from 9c8f8c2 to b23cde0 Compare June 7, 2024 10:52
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

This overall looks very solid. Great work 💪

I have some insignificant/cosmetic remarks:

src/bundle/EventListener/CsrfListener.php Show resolved Hide resolved
src/bundle/EventListener/ResponseListener.php Show resolved Hide resolved
src/lib/Server/Controller/SessionController.php Outdated Show resolved Hide resolved
src/lib/Server/Controller/SessionController.php Outdated Show resolved Hide resolved
tests/bundle/Functional/SessionTest.php Outdated Show resolved Hide resolved
tests/bundle/Functional/SessionTest.php Outdated Show resolved Hide resolved
tests/bundle/Functional/SessionTest.php Outdated Show resolved Hide resolved
tests/bundle/Functional/SessionTest.php Outdated Show resolved Hide resolved
tests/bundle/Functional/UserTest.php Outdated Show resolved Hide resolved
@alongosz alongosz requested a review from a team June 7, 2024 17:28
Copy link
Contributor

@webhdx webhdx left a comment

Choose a reason for hiding this comment

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

I didn't find any major technical issues. The direction is in line with what we've discussed on the meetings. I only have some minor nitpicks and I spotted a few places where more PHP 8.3 features could be enforced (constructor promotion, readonly etc.).

tests/bundle/Functional/SessionTest.php Outdated Show resolved Hide resolved
@webhdx webhdx requested a review from a team June 12, 2024 12:58
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Errors in tests that occurred can be narrowed down to these issues:

UDW displays incomplete tree.
(Missing items: Users, Site skeletons, Corporate Account i Dashboards)
This can be reproduced in various places. E.g. when assigning items to Roles or Sections. When trying to Move items. When setting up File field in Form Builder etc.

New field types cannot be added (e.g. ezcountry, ezdate, eztext).
This can be reproduced when creating new Content Type or Product Type. Error occurs when new field type is dragged into the Field definitions section ("Not Found" error in UI).

Add to bookmarks doesn't work.
This can be reproduced by adding item to bookmarks and reloading the page (item is not marked as bookmarked)

Multi file upload doesn't work.
Either when using drag and drop or the "Upload file" button. File is being uploaded, progress bar is shown but error occurs at the end.

Several locations cannot be loaded in admin panel using the left side menu. UI shows error "Cannot read properties of undefined (reading 'subitems')".
Affects: Users, Corporate, Dashboards, Site skeletons.

@konradoboza
Copy link
Contributor Author

Reported issue should be resolved via ibexa/recipes-dev@a7190fb. Explanation: ibexa/recipes-dev#121 (comment). I will restart the CI here just to be sure nothing got broken, early testing looks promising.

@konradoboza konradoboza requested a review from micszo June 20, 2024 10:21
@konradoboza konradoboza changed the title [REMOVE-DEV-DEPENDENCY] IBX-8290: Reworked REST authentication to comply with the new Symfony authenticator mechanism under separate firewall IBX-8290: Reworked REST authentication to comply with the new Symfony authenticator mechanism under separate firewall Jun 20, 2024
Copy link

sonarcloud bot commented Jun 20, 2024

@konradoboza konradoboza merged commit 450c2f1 into main Jun 20, 2024
10 checks passed
@konradoboza konradoboza deleted the ibx-8290-re-implement-rest-auth branch June 20, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc needed The changes require some documentation Feature New feature request QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants