Session improvements #316

2020-05-15T21:05:57Z

matthijskooijman commented

2020-05-15 21:05:57 +00:00

(Migrated from github.com)

This PR improves the way sessions are handled. In particular:

It gracefully handles session start failures due to permission problems
It only creates a session for users that actually need it (i.e. not for regular visitors).
The session cookie is now better separated and secured
The CSRF token is moved from the session to a cookie (so not everyone will need a session anyway). This also means visitors that take a long time to type a comment are no longer met with a CSRF error.
The static Hypha class (that contained HyphaFiles) is merged into RequestContext (the former was a nice idea, but predated the latter and the latter is probably a better place).
A new HyphaSession class encapsulates all session access.
Some more $_POST accesses were converted to use HyphaRequest instead.
Plus some more small things.

The first two points where what I originally set out to achieve, the other improvements and refactorings were either on the path those, were made possible by these refactorings, or did I just think of while working on this code.

This PR improves the way sessions are handled. In particular: - It gracefully handles session start failures due to permission problems - It only creates a session for users that actually need it (i.e. not for regular visitors). - The session cookie is now better separated and secured - The CSRF token is moved from the session to a cookie (so not everyone will need a session anyway). This also means visitors that take a long time to type a comment are no longer met with a CSRF error. - The static `Hypha` class (that contained `HyphaFile`s) is merged into `RequestContext` (the former was a nice idea, but predated the latter and the latter is probably a better place). - A new `HyphaSession` class encapsulates all session access. - Some more `$_POST` accesses were converted to use `HyphaRequest` instead. - Plus some more small things. The first two points where what I originally set out to achieve, the other improvements and refactorings were either on the path those, were made possible by these refactorings, or did I just think of while working on this code.

RoukePouw (Migrated from github.com) reviewed

2020-05-15 21:05:57 +00:00

matthijskooijman commented

2020-05-19 09:02:13 +00:00

(Migrated from github.com)

One of the changes this PR made was to not write the session on every request if it was not changed. However, I realized that this could actually cause the session to expire (session file deleted by the PHP session GC), because that checks for modified time rather than access time. This would mean the session lifetime was counted from login, rather than last access, which did not seem a good idea. So I removed that change and instead documented that the writing is intentional instead.

One of the changes this PR made was to *not* write the session on every request if it was not changed. However, I realized that this could actually cause the session to expire (session file deleted by the PHP session GC), because that checks for modified time rather than access time. This would mean the session lifetime was counted from login, rather than last access, which did not seem a good idea. So I removed that change and instead documented that the writing is intentional instead.

matthijskooijman commented

2020-07-14 09:50:55 +00:00

(Migrated from github.com)

I just rebased this on top of master, now #331 is merged (which removes a few commits from this PR).

laurensmartina (Migrated from github.com) requested changes

2020-07-14 11:46:11 +00:00

system/core/HyphaSession.php

					
				@ -0,0 +5,4 @@

					 * It should always be used instead of direct access to

					 * $_SESSION.

					 */

					class HyphaSession {

To ensure the lock it would be better to apply the singleton pattern;

class HyphaSession {
    private static $instance;
    public static function getInstance() {
        if (!self::$instance instanceof HyphaSession) {
            self::$instance = new HyphaSession;
        }

        return self::$instance;
    }

    private function __construct() {
        // ....
    }
}

$hyphaSession = HyphaSession::getInstance();

To ensure the lock it would be better to apply the singleton pattern; ``` class HyphaSession { private static $instance; public static function getInstance() { if (!self::$instance instanceof HyphaSession) { self::$instance = new HyphaSession; } return self::$instance; } private function __construct() { // .... } } $hyphaSession = HyphaSession::getInstance(); ```

system/core/RequestContext.php

The */ should be on the next line when it comes to comment blocks.

The `*/` should be on the next line when it comes to comment blocks.

system/core/base.php

This should be themeHtml in stead of html; $O_O->data->themeHtml->read();

This should be `themeHtml` in stead of `html`; `$O_O->data->themeHtml->read();`

system/core/base.php

This should be themeHtml in stead of html; $O_O->data->themeHtml->writeWithLock($contents);

This should be `themeHtml` in stead of `html`; `$O_O->data->themeHtml->writeWithLock($contents);`

This should be themeCss in stead of css; $O_O->data->css->read();

This should be `themeCss` in stead of `css`; `$O_O->data->css->read();`

system/core/base.php

This should be themeCss in stead of css; $O_O->data->css->writeWithLock($contents);

This should be `themeCss` in stead of `css`; `$O_O->data->css->writeWithLock($contents);`