Skip to content

Conversation

@konradoboza
Copy link
Contributor

@konradoboza konradoboza commented May 14, 2024

🎫 Issue IBX-8140

The idea is to engage Symfony mechanisms to check permissions. Otherwise, we end-up in the BO-user-protected templates which cannot perform any operations on null user which is anonymous in the new Symfony security approach.

Removal of the Ibexa\Contracts\AdminUi\Controller\Controller definition is done due to duplication, ref: https://github.com/ibexa/admin-ui/blob/main/src/bundle/Resources/config/services.yaml#L49.

Related PRs:

Description:

For QA:

Documentation:

@alongosz alongosz requested a review from a team May 21, 2024 10:30
@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from bfb4f1e to c2b9db3 Compare May 21, 2024 12:37
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@konradoboza konradoboza requested a review from a team May 22, 2024 07:05

Ibexa\Bundle\AdminUi\Controller\ContentViewController: ~
Ibexa\Bundle\AdminUi\Controller\ContentViewController:
parent: Ibexa\Contracts\AdminUi\Controller\Controller
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really immediately see how setting this as parent definition helps. Especially since this definition lies in a completely different file it seems.

Have you considered using _instanceof to prevent forgetting that this parent has to be added to each controller of this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I haven't. Til this point we have followed adding parent controller to all of the admin-ui-related ones. I only aimed to fix the ones that slipped between cracks and which I tracked down while working on enabling new security layer.

I would say that if this approach needs revisiting we should work on that separately as it's a bit out of scope of security-related changes.

Unless you had something else in mind.

@konradoboza konradoboza merged commit a340f40 into main May 23, 2024
@konradoboza konradoboza deleted the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch May 23, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature New feature request Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants