Abuse of Global Scope Within Session and Auth Adapters

mikegreiling commented 12 years ago

Description of the issue.

Lithium does a great job of avoiding the use of superglobals in favor of the Request object, and relegating all output functionality to the Response object's render() method. All other objects defer to these two in all input/output functions. This enhances testability by reducing the side effects of globally scoped variables and output functions, essentially leaving the global I/O immutable outside of these privelaged objects. I think the term for this behavior is referential transparency, and you guys talk a great deal about it in your FAQs.

Lithium, however falls short in one area. The adapters for \lithium\storage\Session and \lithium\security\Auth appear to have a unique exclusion to the rule that the rest of the library abides by. They both take liberties with header(), setcookie(), and session_start() (which send output to the client), and read data directly from $_COOKIE (and $_GET/$_POST if you include session_start() when session.use_only_cookies is false).

Prescription

I think the separation of concerns regarding client I/O is important, and should be consistant. I would propose the following:

Add handling of the $_COOKIE superglobal to \lithium\action\Request (it is part of the request after all). It could be accessed alongside $request->data and $request->query as $request->cookie or something similar.
Add functionality to set cookies in \lithium\action\Response, to be rendered only in Response::render(). Just as you can currently set arbitrary headers with $response->headers(), setting cookies will store them in some temporary location along with TTL and scope data to await rendering.
Update \lithium\storage\Session to include two adapter methods for Request injection and Response manipulation. I'd call these something like Session::prime($request) and Session::apply($response), and they would be filtered into Dispatcher::run() like so:
```
Dispatcher::applyFilter('run', function($self, $params, $chain) {
   Session::prime($params['request']);

   if (($result = $chain->next($self, $params, $chain)) instanceof Response) {
       Session::apply($result);
   }
   return $result;
});
```
Note the prime method should not actually do anything other than collect the $request object and store it for later use. This is because due to the ordering of Dispatcher filters, the Environment::set() may not yet have been called.
Update \lithium\security\Auth similarly, with prime() and apply() adapter methods. The prime() method could be ignored if you intend to pass the request into Auth::check() every time. The apply() method would have no use in any of the current Auth adapters, but it would have applications if someone wanted to create an adapter which saves an authentication token as a cookie upon login or logout (implementing "keep me logged in for xxx days" functionality). These methods could also be wrapped around Dispatcher::run()
Update all Auth and Session adapters, removing all access to superglobals, header(), and setcookie(), and neutering session_start() with ini_set("session.use_cookies", false) and setting the id manually with session_id($key).
Update all unit tests to utilize mock Request/Response objects rather than testing superglobals and headers_list().

Concerns

Cons:

This requires an update to framework as well as lithium.
This will break some backwards compatibility, but growing pains can be minimized with liberal use of descriptive exceptions (i.e. when Session::read() is called prior to Session::prime()). Plus, if needed, taking out and plugging in the old session class in place of the new one is an easy task due to Li3's flexible nature.

Pros:

Giving authors of custom Session and Auth adapters access to Request and Response objects will make development of advanced functionality much easier.
Relegating all I/O functions out of the global scope will make testing much more ideal as all side effects to the global scope are confined and filtered through two objects.
It's just more ideologically consistent.

Implementation

I can help write tests, update the classes, and do whatever is necessary to make this happen, but I want some tentative approval from the powers that be before I put forth all of that effort. And of course, I would welcome any help.

I had brought this up in the #li3-core chatroom about a year and a half ago and @gwoo seemed to be on board. Unfortunately I did not have the free time back then to implement it.

mikegreiling commented 12 years ago

Since this does break some BC and requires changes to framework in addition to lithium, this should probably live in a branch and wait for the next milestone release (1.0 or 0.11).

nateabele commented 12 years ago

This is an interesting proposal and I'm not opposed to it. I just need to think on it for a while.

tonyeung commented 12 years ago

I'm not sure if this is appropriate, but if its ok, I'd like to add my vote for this, and see it implemented in the current master.

mikegreiling commented 12 years ago

Thanks @nateabele, I'm interested to hear your considered opinion, and I'd love to get feedback from anyone else with 2¢ to add to the conversation as well.

Ciaro commented 12 years ago

There are more issues than this with the Session functionality:

configurations seem to eat each other (try combining cookie and session adapters)
session encryption doesn't play along nicely with the flash_message plugin
session encryption writes two cookies instead of one (one encrypted and one with normal data)

Might as well look at them together?

nateabele commented 12 years ago

@Ciaro The reason they seem to eat each other is because I had a design idea originally where you could read or write to multiple ones in a single call. That ended up just not working well, so yeah, that's definitely something we can address as well.

The issue with session encryption and flash messages is that li3_flash_message always uses its own session storage. This is obviously a problem, which I'm trying to address here: https://github.com/uor/li3_flash_message (fork and contribute if you feel so inclined).

Ciaro commented 12 years ago

@nateabele Thanks for the update!

Atm, I'm quite swamped with a new project. I probably cannot invest time looking into your fork (in detail). I'm more than happy to test it out on one or two projects once it's finished, though...

mikegreiling commented 12 years ago

It's been a few weeks. Any thoughts on these or other changes to the session/auth adapters?

nateabele commented 12 years ago

@pixelcog I just had a kid, so to me it's still only been a few days. ;-)

mikegreiling commented 12 years ago

@nateabele I'd say that's a valid excuse :)

UnionOfRAD / lithium