Require login by default & fast-fail if login is required but user isn't

[cpeel]

Note: this is intended for merging after this month's release.

Shift the logic such that login is required on all pages and make the few that don't require login to explicitly state that. This lets us do a few wise & performant things:

1. we can globally enforce login everywhere as a rule.
2. we can fast-fail to the login page if the requester doesn't have a DP session cookie -- this bypasses even connecting to the database. For "dumb" crawlers that aren't using cookies this dramatically reduces the per-page overhead.
3. we can fail to the login page if the user isn't logged in -- this bypasses loading the user record from the database, setting up gettext, etc.

The interesting changes in this PR are in:

• pinc/base.inc
• pinc/bootstrap.inc
• pinc/dpsession.inc

This will require matching changes in noncvs that currently call require_login() now that the function is deprecated and a no-op.

Sandbox: https://www.pgdp.org/~cpeel/c.branch/require-login-by-default/

[chrismiceli]

Seems logical to fail safely and avoid unnecessary db connections. Can you elaborate on where those db connections were setup for authenticated pages that were hit while unauthenticated? Just curious.

[cpeel]

Can you elaborate on where those db connections were setup for authenticated pages that were hit while unauthenticated? Just curious.

Sure. Since we store sessions in the database, all pages need a DB connection and we've written the code in base.inc and bootstrap.inc for that common path (bootstrap.inc connects to the database). And to confirm a user is authenticated we need to load their session from the database -- that hasn't changed. But if the user doesn't have the cookie to map to a session they can't be logged in and we don't need to check the database for it, we can just redirect them to the login page.

If the user does have a session cookie, we need to connect to the database and load the session to see if they're actually logged in, but we don't need to do other things like try to load the user's record, configure gettext, etc if they aren't actually logged in.

Letting base.inc & bootstrap.inc know if a page requires authentication lets us do this short-circuiting as quickly as we know the user isn't authenticated rather than after all of the bootstrapping code has been run.

[bpfoley]

Just an overall comment about this, that something feels slightly odd about the design to me, but it's largely because my mental model (and PHPStan's way of analyzing the entire repo) doesn't quite align with what the codebase actually is: namely a large group of disjoint scripts, one per .php file that happen to include_once()/require_once() some common files.

This is what caused me to have to rename some functions that had the same function name across different scripts, but different numbers of arguments: PHPStan assumed that the first definition of the function it saw (in one .php file) was the correct one, and the second one was a mismatch.

Imagine
acounts/activate.php

<?php
namespace Accounts\Activate;
$require_login = false;
include_once($relPath.'base.inc');

project.php

<?php
namespace Project;
include_once($relPath.'base.inc');

Now, when in base.inc we say

if (!@isset($require_login)) {
    $require_login = true;
}
...
if ($require_login) {
    require_login();
}

we're actually referring to different 'instances' of $require_login depending on which include_once the interpreter ran. One instance is 'really' the $require_login from acounts/activate.php and the other instance is defined 'on behalf of' project.php. Since PHP variables aren't namespaced, we can't call these \Accounts\Activate\$require_login and \Project\$require_login, but that's effectively what they are.

I guess what I'm describing in a round-about way is the caveat about global variables in PHP: they're really what programming language people call 'dynamically scoped' rather than 'lexically scoped'. And that's the reason they aren't namespaced, because namespacing is a lexical thing.

Just to be clear, I'm not objecting to this PR at all. These ramblings are just me internalising what it means.

[cpeel]

Numbers are fun, let's show some numbers. This is testing done using hey on TEST.

project.php requests with no DP cookie

This is what I suspect most all crawlers do based on my observations at pgdp.net:

Code in master:

Summary:
  Total:  22.8116 secs
  Slowest:  0.3127 secs
  Fastest:  0.0220 secs
  Average:  0.1132 secs
  Requests/sec: 438.3735


Response time histogram:
  0.022 [1] |
  0.051 [1] |
  0.080 [3] |
  0.109 [5058]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.138 [4340]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.167 [300] |■■
  0.196 [177] |■
  0.226 [78]  |■
  0.255 [16]  |
  0.284 [13]  |
  0.313 [13]  |


Latency distribution:
  10%% in 0.0998 secs
  25%% in 0.1041 secs
  50%% in 0.1091 secs
  75%% in 0.1155 secs
  90%% in 0.1256 secs
  95%% in 0.1449 secs
  99%% in 0.2003 secs

Code in this PR:

Summary:
  Total:    6.3496 secs
  Slowest:  0.2110 secs
  Fastest:  0.0015 secs
  Average:  0.0314 secs
  Requests/sec: 1574.8970


Response time histogram:
  0.001 [1] |
  0.022 [507]   |■■
  0.043 [9021]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.064 [328]   |■
  0.085 [61]    |
  0.106 [17]    |
  0.127 [20]    |
  0.148 [26]    |
  0.169 [10]    |
  0.190 [8] |
  0.211 [1] |


Latency distribution:
  10%% in 0.0260 secs
  25%% in 0.0278 secs
  50%% in 0.0299 secs
  75%% in 0.0327 secs
  90%% in 0.0370 secs
  95%% in 0.0425 secs
  99%% in 0.0729 secs

project.php requests with DP cookie but not logged in

Code in master:

Summary:
  Total:  22.3956 secs
  Slowest:  0.3289 secs
  Fastest:  0.0066 secs
  Average:  0.1116 secs
  Requests/sec: 446.5164


Response time histogram:
  0.007 [1] |
  0.039 [4] |
  0.071 [12]  |
  0.103 [1726]  |■■■■■■■■■
  0.135 [7959]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.168 [234] |■
  0.200 [27]  |
  0.232 [32]  |
  0.264 [1] |
  0.297 [2] |
  0.329 [2] |


Latency distribution:
  10%% in 0.1008 secs
  25%% in 0.1053 secs
  50%% in 0.1103 secs
  75%% in 0.1156 secs
  90%% in 0.1219 secs
  95%% in 0.1279 secs
  99%% in 0.1584 secs

Code in this PR is a slight improvement, but not a noticeable one:

Summary:
  Total:  18.4386 secs
  Slowest:  0.2572 secs
  Fastest:  0.0032 secs
  Average:  0.0918 secs
  Requests/sec: 542.3414


Response time histogram:
  0.003 [1] |
  0.029 [3] |
  0.054 [15]  |
  0.079 [494] |■■
  0.105 [8874]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.130 [481] |■■
  0.156 [65]  |
  0.181 [29]  |
  0.206 [15]  |
  0.232 [16]  |
  0.257 [7] |


Latency distribution:
  10%% in 0.0817 secs
  25%% in 0.0856 secs
  50%% in 0.0903 secs
  75%% in 0.0960 secs
  90%% in 0.1018 secs
  95%% in 0.1064 secs
  99%% in 0.1429 secs

[cpeel]

Just an overall comment about this, that something feels slightly odd about the design to me, but it's largely because my mental model (and PHPStan's way of analyzing the entire repo) doesn't quite align with what the codebase actually is: namely a large group of disjoint scripts, one per .php file that happen to include_once()/require_once() some common files.

Right. The DP codebase is not a "PHP application" it's a bunch of scripts in a trench coat pretending to be an application.

Given that as a constraint, is there a better mechanism to achieve this than a global variable? Would PHPStan appreciate a constant better? We should be able to achieve the same thing that way.

[bpfoley]

Just an overall comment about this, that something feels slightly odd about the design to me, but it's largely because my mental model (and PHPStan's way of analyzing the entire repo) doesn't quite align with what the codebase actually is: namely a large group of disjoint scripts, one per .php file that happen to include_once()/require_once() some common files.

Right. The DP codebase is not a "PHP application" it's a bunch of scripts in a trench coat pretending to be an application.

Given that as a constraint, is there a better mechanism to achieve this than a global variable? Would PHPStan appreciate a constant better? We should be able to achieve the same thing that way.

So... I don't think this is a constraint as such, just something I was trying to internalise. From PHPStan's point of view, all the different $require_logins are the same global variable, and that doesn't cause any type checking problems, because they're only ever checked for existence, or have boolean values assigned to them or are used as booleans.

So this is just my very roundabout way of saying LGTM!

[srjfoo]

I've made the assumption that everything that has had the require_login() function call removed is fine. But I have some questions about those where $require_login is set to false.

Some, I can understand (the front page, list_etexts), but others, I'm curious about:

• faq/prooffacehelp.php
• faq/site_progress_snapshot_legend.php
• translate.php
• quiz/index.php (which apparently does require login via another path?)
• stats/equilibria.php
• stats/percent_users_who_proof.php
• stats/pp_stage_goal.php
• stats/round_backlog.php and stats/round_backlog_days.php
• styles/design_philosophy.php
• styles/style_demo.php
• tools/proofers/ctrl_frame.php (which is not, in fact, accessible because other parameters are necessary)
• credits.php

Some, like translate.php I can kind of see leaving not requiring login, but prooffacehelp.php and site_progress_snapshot_legend just seemed weird.

As I said -- not objecting so much as just curious.

[cpeel]

I've made the assumption that everything that has had the require_login() function call removed is fine. But I have some questions about those where $require_login is set to false.

Some, I can understand (the front page, list_etexts), but others, I'm curious about:

https://github.com/DistributedProofreaders/dproofreaders/blob/master/SETUP/ci/check_require_login.php has some additional comments on why. I think the idea was that we wanted the faq accessible to everyone -- ditto the CSS design pages. The stats pages we embed in other places and maybe we wanted to be able to do that on pages that didn't also require login?

[srjfoo]

I've made the assumption that everything that has had the require_login() function call removed is fine. But I have some questions about those where $require_login is set to false.
Some, I can understand (the front page, list_etexts), but others, I'm curious about:

https://github.com/DistributedProofreaders/dproofreaders/blob/master/SETUP/ci/check_require_login.php has some additional comments on why. I think the idea was that we wanted the faq accessible to everyone -- ditto the CSS design pages. The stats pages we embed in other places and maybe we wanted to be able to do that on pages that didn't also require login?

Thanks -- that's about what I figured. Though without a proofing interface, I'm not sure prooffacehelp is any use, nor the snapshot legend without the snapshot. 😁