fix: Security class sends cookies immediately

[kenjis]

Description
Fixes #5406

• move secure cookie check from Security to ResponseTrait
• Security class does not send cookie, just set it to Response
• CookieStore class does not send cookies, Response sends cookies
• Response::setCookie() can get Cookie object now
• deprecated
  • CookieStore::dispatch()
  • CookieStore::setRawCookie()
  • CookieStore::setCookie()
  • Security::sendCookie()
  • Security::doSendCookie()

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

This adds a new dependency (Cookie on Security), which is fine but it must be defined in depfile.yaml.

[lonnieezell]

Does this need to handle the setRawCookie (?) method from the security class also?

[paulbalandan]

Looking at the previous dependency addition warning, I think the SecurityException should not be inside the CookieStore class. IMO, CookieStore should be clean from outside dependencies and just dispatch the cookies in its collection. No outside interference.

The addition of setCookieStore() method in Response looks good to me. Reading on the related issue, my suggestion would be:

1. On sending the CSRF cookie, Security gets the saved instance of the CookieStore from the Response then save to it the CSRF cookie.
2. Security still retains the secure checks on the cookie.
3. Security puts the new CookieStore instance to the Response using $response->setCookieStore($cookieStore);
4. Response, after saving the new cookie store instance, invokes sendCookies() via the send() method. This calls $cookieStore->dispatch()
5. CookieStore will send all the cookies in its store, including the CSRF cookie.

[kenjis]

@lonnieezell

Does this need to handle the setRawCookie (?) method from the security class also?

What do you mean?

[kenjis]

@paulbalandan

I think it is weird that CookieStore dispatches cookies.
Response should dispatch cookies.
But if do so, it seems breaking changes.

Your opinion is just moving the secure checks on the cookie back to Security, right?

[kenjis]

Response should dispatch cookies.
But if do so, it seems breaking changes.

I thought that. But it seems I could go.
I moved dispatching cookie from CookieStore to Respose.

[MGatner]

It looks like @paulbalandan has a better grasp on this so I will not review. Thanks for the work though, I assume this fixes the test case issues.

[paulbalandan]

I've looked at how other frameworks send cookies. Particularly, for CakePHP, they send cookies using the ResponseEmitter class so I think this is good. In the future, or future versions, I think we can also modularize class responsibilities.

[kenjis]

I rebased for just signing commits.

[kenjis]

I will add the documentation.

[mostafakhudair]

Any point of storing response service in a variable?

Suggested change

	/** @var Response $response */
	$response = Services::response();
	$response->setCookie($this->cookie);
	Services::response()->setCookie($this->cookie);

[kenjis]

To explicitly indicate dependency.
This is also to avoid disabling the deptrac check.

You may like the short code, but it hides the dependency Response.
deptrac can't detect it.

Services::response()->setCookie($this->cookie);

[kenjis]

I added the documentation.
Can someone review?

[MGatner]

💪