Migrate login and authentication in Back Office to Symfony #35983

jolelievre · 2024-04-22T08:42:16Z

Questions	Answers
Branch?	develop
Description?	Migrate login and authentication in Back Office to Symfony
Type?	new feature
Category?	BO
BC breaks?	yes
Deprecations?	yes
How to test?	CI tests and UI tests green Manual checks: - check login to BO - check logout from BO - check password reset feature - check permission are consistent with assigned profile - check profile edition
UI Tests	https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/8921326681
Fixed issue or discussion?	Fixes #36002
Related PRs	PrestaShop/ga.tests.ui.pr#67 GA UI test tool updated to remove the option about symfony layout that is no forced and the only possibility
Sponsor company	~

prestonBot · 2024-04-22T08:42:21Z

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
It can help trigger a discussion about the best implementation path before a single line of code is written.
It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

Hlavtox

Does not seem to reimplement PS_COOKIE_CHECKIP removed. This feature allows to enable/disable checking IP address of the session.

Correct me and remove the block if I am wrong. :-)

src/PrestaShopBundle/Security/Admin/AdminAuthenticationSuccessHandler.php

jolelievre · 2024-05-02T12:21:36Z

Does not seem to reimplement PS_COOKIE_CHECKIP removed. This feature allows to enable/disable checking IP address of the session.

Correct me and remove the block if I am wrong. :-)

I didn't know about this one, just like the cron URLs feature I added this in the epic #36001

The present PR is already big enough but it still passes all our autotests, a few left tasks are already identified and will be handled in smaller PRs, if you have other thoughts about missing things feel free to let us know so that we can add them into the epic

I don't think we should block the PR though considering it already handles maybe 90% of the whole login/authorization feature and I can guarantee that all the remaining tasks will be dealt with right after this PR is merged 😉

M0rgan01 · 2024-05-02T12:24:27Z

admin-dev/index.php

+$response = $kernel->handle($request, HttpKernelInterface::MAIN_REQUEST, true);
+$response->send();
+$kernel->terminate($request, $response);


Finally 👍

Hlavtox · 2024-05-02T12:40:20Z

@jolelievre No worries, I trust you. Let's merge this so the PR doesn't keep growing. I added the notes to #36001. ;-)

M0rgan01 · 2024-05-02T12:29:22Z

app/config/admin/services.yml

-  prestashop.adapter.security.admin:
-    class: "%AdapterSecurityAdminClass%"
-    arguments:
-      - "@prestashop.adapter.legacy.context"
-      - "@security.token_storage"
-      - "@prestashop.security.admin.provider"
-      - "@security.helper"
-    tags:
-      - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest }
-


M0rgan01 · 2024-05-02T12:30:23Z

app/config/admin/services.yml

+    autowire: true
+    autoconfigure: true


Not necessary due to default value

I note to remove it in the next PR 😉

M0rgan01 · 2024-05-02T12:33:35Z

app/config/parameters.yml.dist

+    cookie_key: ThisTokenIsNotSoSecretChangeIt
+    cookie_iv: ThisTokenIsNotSoSecretChangeIt


We could put this in environment variables

Yep for now they are default parameters, if we use env variables we can always use the env directive in the parameters, but currently we transform parameters in a PHP file so we can't really enjoy this feature To use this we should do a small refacto to actually use the parameters.yml file

I believe I had to add those default values here because they would make the SF console break when absent at some point, and it's more consistent to have all the required fields in the dist file anyway

M0rgan01 · 2024-05-02T12:44:06Z

src/PrestaShopBundle/Entity/Employee/Employee.php

+    /**
+     * @ORM\Column(name="email", type="string")
+     */
+    private string $email;


Is it interesting to add a unique constraint to this field?

Not for now, the Doctrine entity is not used in edition (not in the form nor in the CQRS commands), so they would be useless The should be added when we refacto the CQRS commands I guess

Argh my bad I misunderstood I thought you were talking about assertions for the entity validation Regarding the unicity of this column you're completely right But it's a change of the DB structure I prefer to handle it in another PR, we also discussed in another comment about removing the theme customizations field, I guess all this could be handled in a dedicated PR

M0rgan01 · 2024-05-02T12:45:20Z

src/PrestaShopBundle/Entity/Employee/Employee.php

+
+    /**
+     * @ORM\Column(name="bo_color", type="string", length=32, nullable=true)
+     */
+    private ?string $boColor;
+
+    /**
+     * @ORM\Column(name="bo_theme", type="string", length=32, nullable=true)
+     */
+    private ?string $boTheme;
+
+    /**
+     * @ORM\Column(name="bo_css", type="string", length=64, nullable=true)
+     */
+    private ?string $boCss;
+
+    /**
+     * @ORM\Column(name="bo_width", type="integer", options={"default": 0, "unsigned": true})
+     */
+    private int $boWidth;
+
+    /**
+     * @ORM\Column(name="bo_menu", type="boolean", options={"default": 1})
+     */
+    private bool $boMenu;


When I see that I tell myself that it could go to another entity

This is no longer used, maybe we can use this opportunity to remove this 💩?

bo_color
bo_width
bo_theme (there is only default)
bo_menu

Ping @kpodemski correct me if I miss something

I don't mind, for now I had to leave them in the Doctrine entity or the doctrine command would automatically delete these fields, so I had to sync the content of the entity with the db structure table

We could clean those, but in another PR then

paulnoelcholot · 2024-05-07T19:27:03Z

Hello @jolelievre,

I tested your PR and it's good for me! 🎉

Thanks!

gericfo · 2024-10-14T12:33:52Z

Hello @jolelievre
Please note that the “Needs autoupgrade PR” label has been automatically added to your Pull Request. This means that the changes made in your PR must also be reported in the Autoupgrade module, responsible for running the update process of PrestaShop stores.
When there's a modification to the db-structure.sql file or if a Doctrine entity has been modified, added, or deleted, it impacts the database structure. Modifying the list of hooks and feature flags has consequences on the update process as well. These changes can affect how the application interacts with the database and how update procedures are handled. It's important to consider these aspects when contributing to ensure a smooth update experience for users.
That's why this contribution should be backported, with another corresponding PR, on the Autoupgrade module's GitHub repository which is available here: link.
Don't forget to add a comment in this current thread with the link to this PR once it has been created.
Thank you.

gericfo · 2024-10-16T06:36:39Z

Related Autoupgrade PR - PrestaShop/autoupgrade#954

prestonBot added develop Branch Feature Type: New Feature BC break Type: Introduces a backwards-incompatible break labels Apr 22, 2024

jolelievre changed the title ~~Migrate login~~ Migrate login and authentication in Back Office Apr 22, 2024

jolelievre changed the title ~~Migrate login and authentication in Back Office~~ Migrate login and authentication in Back Office to Symfony Apr 22, 2024

jolelievre force-pushed the migrate-login branch 2 times, most recently from 5d4b466 to 46ff748 Compare April 22, 2024 10:26

jolelievre mentioned this pull request Apr 23, 2024

Migrate login form, logout and handle authorization via Symfony, remove Symfony layout feature flag #36002

Closed

jolelievre force-pushed the migrate-login branch 21 times, most recently from 9adc35e to 0665025 Compare April 29, 2024 19:14

Hlavtox requested changes May 2, 2024

View reviewed changes

ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label May 2, 2024

nicosomb reviewed May 2, 2024

View reviewed changes

src/PrestaShopBundle/Security/Admin/AdminAuthenticationSuccessHandler.php Show resolved Hide resolved

jolelievre mentioned this pull request May 2, 2024

Migrate Login backoffice page and authentication system to Symfony #36001

Closed

16 tasks

prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label May 2, 2024

M0rgan01 reviewed May 2, 2024

View reviewed changes

nicosomb approved these changes May 2, 2024

View reviewed changes

Hlavtox approved these changes May 2, 2024

View reviewed changes

ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label May 2, 2024

M0rgan01 reviewed May 2, 2024

View reviewed changes

M0rgan01 added the Needs autoupgrade PR label May 2, 2024

M0rgan01 added this to the 9.0.0 milestone May 2, 2024

M0rgan01 approved these changes May 2, 2024

View reviewed changes

paulnoelcholot approved these changes May 7, 2024

View reviewed changes

paulnoelcholot added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels May 7, 2024

nicosomb merged commit 8bad56c into PrestaShop:develop May 8, 2024
39 checks passed

ChillCode mentioned this pull request May 10, 2024

Error 302: ERR_TOO_MANY_REDIRECTS in Back Office #35751

Open

2 tasks

jolelievre deleted the migrate-login branch May 12, 2024 12:35

jolelievre mentioned this pull request May 27, 2024

Check IP when connected on BO #36243

Closed

kpodemski added the Documentation ✔️ Developer documentation is up-to-date label Jul 31, 2024

matks mentioned this pull request Sep 25, 2024

[SF Migration] Use Symfony components for authentication instead of legacy system #19124

Closed

gericfo added the PR Autoupgrade opened ✔️ The associated PR has been opened in the Autoupgrade module repository. label Oct 16, 2024

Hlavtox removed Needs autoupgrade PR PR Autoupgrade opened ✔️ The associated PR has been opened in the Autoupgrade module repository. labels Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate login and authentication in Back Office to Symfony #35983

Migrate login and authentication in Back Office to Symfony #35983

jolelievre commented Apr 22, 2024 •

edited

Loading

prestonBot commented Apr 22, 2024

Hlavtox left a comment

jolelievre commented May 2, 2024

M0rgan01 May 2, 2024

Hlavtox commented May 2, 2024

M0rgan01 May 2, 2024

M0rgan01 May 2, 2024

jolelievre May 2, 2024

M0rgan01 May 2, 2024

jolelievre May 2, 2024

M0rgan01 May 2, 2024

jolelievre May 2, 2024

jolelievre May 2, 2024

M0rgan01 May 2, 2024

M0rgan01 May 2, 2024

Hlavtox May 2, 2024

jolelievre May 2, 2024

paulnoelcholot commented May 7, 2024

gericfo commented Oct 14, 2024

gericfo commented Oct 16, 2024 •

edited

Loading

		cookie_key: ThisTokenIsNotSoSecretChangeIt
		cookie_iv: ThisTokenIsNotSoSecretChangeIt

Migrate login and authentication in Back Office to Symfony #35983

Migrate login and authentication in Back Office to Symfony #35983

Conversation

jolelievre commented Apr 22, 2024 • edited Loading

prestonBot commented Apr 22, 2024

About linked issues

Hlavtox left a comment

Choose a reason for hiding this comment

jolelievre commented May 2, 2024

Choose a reason for hiding this comment

Hlavtox commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulnoelcholot commented May 7, 2024

gericfo commented Oct 14, 2024

gericfo commented Oct 16, 2024 • edited Loading

jolelievre commented Apr 22, 2024 •

edited

Loading

gericfo commented Oct 16, 2024 •

edited

Loading