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

Stop full object exposure on the front end #8803

Merged
merged 17 commits into from Mar 21, 2018

Conversation

@eternoendless
Copy link
Member

commented Feb 23, 2018

Questions Answers
Branch? 1.7.3.x
Description? This PR introduces object filtering through property whitelisting. This filtering is now applied to:
1. Objects being sent to the front office through the "prestashop" object,
2. Objects being sent after adding a product to the cart,
3. Objects being sent in response to search.

It also introduces two hooks:
1. actionBuildFrontEndObject - Allows to add elements to the "prestashop" object included in javascript
2. actionFrontControllerAfterInit - Launched after the "init" method on all front controllers
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-4509
How to test? See ticket

This change is Reviewable

}
}
return $products;
$filter = (new CollectionFilter())

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 24, 2018

Member

you have access to service container in controllers, take a look at config/services/services/front folder :)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 26, 2018

Author Member

Good to know! But how would modules override that?

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

You have information in docs you asked me to do :)

tldr: write a yml file and a class which implement the same interface in the module

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 27, 2018

Author Member

Ohh, that could be the answer to the extensibility issue I'm struggling with!

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

<3 <3 Always happy to help <3<3

interface FilterInterface
{
public function filter($subject);

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 24, 2018

Member

phpdoc please so you can declare {@inheritdoc} everywhere: wdyt? :)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 26, 2018

Author Member

I'll add the phpdoc but I don't think I will inheritdoc everywhere since each concrete class has its own parameter and return type.

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

fair enough 👍

{
public function __construct()
{
$whitelist = array(

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 24, 2018

Member

hum... looks like a configurable object... maybe a factory instead of multiples classes?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 26, 2018

Author Member

maybe a factory

Like this?

class ConfigurationFilterFactory()
{
    public function build()
    {
        return new HashMapWhitelistFilter(array(...));
    }
}

instead of multiples classes?

🤔 what do you mean? A single factory with multiple factory methods, one for each type?

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

maybe like this

$factory->build($name, $whitelist) // return instance of filter interface

I want to extract the configuration from objects... if we do that we can see that we dont really need them, isn't it?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 27, 2018

Author Member

I want to extract the configuration from objects

Oh that's already the case:

(new HashMapWhitelistFilter())->whitelist($whitelist)

My original idea was that you can mix & match generic filters in order to filter more complex structures in a more-or-less semantic way. What value would a factory add here?

Specific classes like ProductFilter are simply an extended filter class with a hardcoded whitelist. They serve a dual purpose:

  1. Provide better semantics for mixed & nested filters
  2. Encourage reusability and extensibility

I'll explain more in a bit.

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

you're right, I'm frustrated to not have a computer here to play a little bit with your pull request :)

@PrestaEdit

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

As I understand, only the properties set inside the white list array will be exposed ? And this list can't be modified by a third party module, isn't it ?

If yes, I think it will be good to rethink this to a black list (and not white one) or allow the third party modules to alter the white list array.

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

@PrestaEdit if every filter is called using the service container you'll be able to override them by overriding the service definitions in the module (feature available in 1.7.3+ only). So implementing the right interface will give you full powers and responsabilities.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

@mickaelandrieu what about multiple modules? does one respect another modifications?

@PrestaEdit

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

You want no more override and you mention... overriding feature. It's kind of no logic, there, no ? :)

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

I must agree with @PrestaEdit - we need to extend, not override.

return $products;
$filter = (new CollectionFilter())
->queue([
(new HashMapWhitelistFilter())

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 24, 2018

Member

Shouldnt be ProductFilter for consistency?

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 24, 2018

Member

If not, how about create a specific and overiddable class/object/service instead? I feel we should not write configuration in controller :(

This comment has been minimized.

Copy link
@eternoendless

eternoendless Feb 27, 2018

Author Member

Yeah you're right, I'll make a specific class for this.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

When I have a global look on this PR, I wonder why these lists are not stored near, or with the $definition properties of our object Model, this could avoid for instance several definition of the Product Filter list (i.e in ProductListingFrontController.php and ProductFilter.php.

Furthermore, we could chose between a whitelist & a blacklist, your product list would be shorter. :)

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

@PrestaEdit I don't want people to override a 800 lines controller to add a key in an array called "$filters" ^^

@kpodemski with or without 1.7, when 2 modules override the same things, there is a conflict: this is not something we can change. But with Symfony and services, you could have a module A that override the filter A and a module B that override the filter B.

In 1.6 "dev like", the module A override the complete controller and if you want to alter anything in it, you can't.

I'm not in favor of extension here, because theses filters shouldnt be used by modules (for me this should be final classes). Why? Because we need to limit the number of classes from Core you use directly in your modules, and instead make you implement the right interfaces: better modules and easier maintenance for both and you :-)

It's my personal opinion ;)

@PrestaEdit

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

Even for one or two lines, overriding is overrinding. There is not others words about that.

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

@Quetzacoalt91 too much responsabilities for object model already if you ask me :D it's a domain model and an active record orm and a table gateway mapper and an Api-like mapper xD

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

@PrestaEdit yes, you're right but I never said I'm against of the override of Symfony services available in PrestaShop: this is the main reason we provide them ;)

But you know what happens in 1.6: people override a class AND extends the original one at the same time. And when we make updates of Core, they cry a lot: this is what " i " want to avoid. Providing overridable services definitions that rely on final classes and implement interfaces force module developers to alter only what they need, and to not depend on Core.

This way we limit conflicts between modules (not all, but a lot more than before, you'll agree on that) and you don't need to "guess" from witch "override" folder the class you try to debug came from (painful work if you ask me)

@PrestaEdit

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

Bad people do that, yes. :)

You don't need to adapt to them, they need to adapt to what they need to do !

Somes modules use an object properties inside somes hooks to alter some others properties, and somes modules add new properties.

It's why I think the better thing is to allow a module to alter this array, as it.

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

@PrestaEdit I'm not against new hooks here, in addition to services: there is nothing wrong with the old good "hooks-way" to do :)

In the end, it's @eternoendless decision :D

@PrestaEdit

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

So, it's good. It's just to have maybe new hooks there, that I have take the voice here ! :D

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

@PrestaEdit I'm getting old, I need time to understand it ahahaha

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Thinking about how we can provide Hook(s) , what do you suggest @PrestaEdit?

"siret",
"website",
);

This comment has been minimized.

Copy link
@kpodemski

kpodemski Feb 27, 2018

Contributor

@mickaelandrieu

what about:
$whitelist = \Hook::exec('actionCustomerFieldsWhiteListModifier', ['whitelist' => &$whitelist]);

?

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Feb 27, 2018

Member

well, I prefer we include the hook dispatcher as dependency and do:

$whitelist = $this->hookDispatcher->dispatch('actionXxx', ['whitelist' => ... ]);

no more global static used, and easier to unit test 👍

This comment has been minimized.

Copy link
@kpodemski

kpodemski Feb 27, 2018

Contributor

@mickaelandrieu yeah sure, i forgot about hookDispatcher, you get the idea tho :)

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2018

Before anything, thank you all for pitching in! That means that you care about what we are doing 🙂

About whitelist vs blacklist

[@PrestaEdit]: I think it will be good to rethink this to a black list (and not white one) or allow the third party modules to alter the white list array.

[@Quetzacoalt91]: Furthermore, we could chose between a whitelist & a blacklist, your product list would be shorter. :)

Let's get this said:

Blacklisting Is A Bad Idea.

Why? Because it's unsafe by default. You don't know which properties will be added in the future, and you absolutely don't want to obliviously let them leak to the outside world.

Incomplete blacklisting is the very reason I had to make this PR: some people added properties with sensitive data to the Product model on their shop, and found them leaking to the front office!

Blacklisting relies on two principles that are incompatible with PrestaShop:

  1. It needs a stable model. You can only blacklist if you know what needs to be filtered out. An extensible system means that, by definition, you cannot know what you'll have in it in advance.

  2. It requires Full knowledge of what has to be done in order to have it working properly (aka the "Don't forget that..." syndrome). If you need to remember, you'll probably forget. Forgot to add your data to the blacklist? You won't realize until you data is already exposed and the damage is done. It's what actually happened.

Why is whitelisting better?

  • It's safe by default. You have to explicitly add it to the list to expose it.
  • It won't bite you in the back if you forget about it. Your data won't be available if you forget to whitelist it. If you actually need it, you'll notice right away.

Of course, it has the disadvantage of being "a little bit more work" because the list is longer. But I think it's worth it, by a long stretch.

About override vs extending

[@kpodemski]: I must agree with @PrestaEdit - we need to extend, not override.

I agree with the need to extend. Override may be useful in certain contexts, so we need to keep that possibility open, but extensibility should always be preferred.

I had already taken the overridable part into account (isolating the filtering part on the controller so it would be easily overridable), as well as some degree of extensibility (generic filters, whitelist() allowing to add or replace elements on the whitelist).

I think I can improve on the extensibility part by declaring filters as services, therefore allowing modules to alter them using the service container. For example if you needed to whitelist another field on your Product, instead of overriding the whole ProductFilter, you'll do it like this:

$this->get('product_front_end_filter')->whitelist(array('myCustomProperty'));

... with the advantage of all the modules being able to add items to the whitelist without overriding each other!

About the division of responsibilities

[@Quetzacoalt91]: I wonder why these lists are not stored near, or with the $definition properties of our object Model

For the same reason that I made this separate from the Presenters. The way I see it, there are three different levels of responsibility:

  1. Models - Source data, persistance
  2. Presenters - Enriched data, to be used by the back end (eg. templates)
  3. Filters - Whitelisted data to be exposed to the front end

That's why they have to be kept separate, even if it means duplicating things.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

$this->get('product_front_end_filter')->whitelist(array('myCustomProperty'));

I love that idea. Overall, i'm all in for having more and more PrestaShop layers as a service, this would be great and add so many possibilities...

@eternoendless eternoendless added this to the 1.7.3.1 milestone Feb 28, 2018

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Reviewed 8 of 14 files at r1, 1 of 1 files at r2, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


src/Core/Filter/HashMapWhitelistFilter.php, line 140 at r3 (raw file):


    /**
     * Returns the ested filters, indexed by $keyToKeep

typo in "nested"


src/Core/Filter/FrontEndObject/CustomerFilter.php, line 59 at r1 (raw file):

Previously, kpodemski (Krystian Podemski) wrote…

@mickaelandrieu yeah sure, i forgot about hookDispatcher, you get the idea tho :)

I agree with this suggestion


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions.


src/Core/Filter/FrontEndObject/CustomerFilter.php, line 41 at r3 (raw file):

        $whitelist = array(
            "addresses",
            "ape",

Did you mean "age" ?


Comments from Reviewable

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

@LittleBigDev SIRET/APE, not age ;)

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Oh 😅

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions.


src/Core/Filter/HashMapWhitelistFilter.php, line 140 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

typo in "nested"

👍


src/Core/Filter/FrontEndObject/CustomerFilter.php, line 59 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I agree with this suggestion

There's no need for a specific hook as you can get the service instance and add/remove items to/from the whitelist. You only need a hook that's launched at the right time (before FrontControllerCore::buildFrontEndObject()). I can't find a proper hook for that though, so I'm going to add it.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

:lgtm:


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Thank you @eternoendless

Maybe a second review should be done to double check the hooks part

@eternoendless eternoendless dismissed stale reviews from mickaelandrieu and LittleBigDev via 6236dff Mar 13, 2018

@eternoendless eternoendless force-pushed the eternoendless:stop-leaks branch from 42fe005 to 6236dff Mar 13, 2018

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

:lgtm:


Reviewed 8 of 8 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

👍

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

@mickaelandrieu mickaelandrieu merged commit 4fdb526 into PrestaShop:1.7.3.x Mar 21, 2018

2 of 3 checks passed

code-review/reviewable 5 discussions left (kpodemski, mickaelandrieu)
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

Thank you @eternoendless ! And thanks everyone for reviews

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

docs

remember about the docs

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

@kpodemski feel free to help us, you can start from hugo branch of docs repository 👍

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2018

I'm writing an article for build, devdocs will follow

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

@mickaelandrieu yeah, i need to get familiar with this Hugo, every year some fancy new stuff for docs ;p

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

@kpodemski in the end it's only "hugo serve" to render the docs in local and markdown :)
If you need help, I'll be available on gitter and I'm working on docs today ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.