Refactor login / ban management #1008

virtualtam · 2017-10-27T16:49:46Z

Relates to #324

Added:

Add the LoginManager class to manage logins and bans

Changed:

Refactor IP ban management
Simplify logic
Avoid using globals, inject dependencies

Fixed:

Use ban_duration instead of ban_after when setting a new ban

ArthurHoaro · 2017-10-28T10:41:46Z

Nice one 👍

Note that you can get rid of that GLOBAL array, and just store a simple serialized array, now that everything is in the same scope.

nodiscc

Nice

virtualtam · 2017-10-31T18:59:31Z

@ArthurHoaro getting rid of globals was planned for #587, I'm tempted to address it in this PR though as it makes more sense to refactor the whole feature at once

@nodiscc this is still a WIP, could you un-approve the PR? (not to say you should disapprove of it ^^)

work in progress

ArthurHoaro · 2017-11-03T16:57:43Z

Oh right, I forgot about this one. I'll move its milestone to get it addressed sooner, if not in this PR.

virtualtam · 2017-11-27T20:38:33Z

application/LoginManager.php

+    function handleSuccessfulLogin($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+        // FIXME unban when behind a trusted proxy?


I'm not sure what to do for this specific point; according to my understanding of the login / ban mechanism, I'd expect this method to be on par with handleFailedLogin() as far as proxy handling is concerned:

the IP is not behind a trusted proxy:

current, expected: reset the ban counter for this IP

the IP is behind a trusted proxy but is not forwarded:

expected: do nothing

current: reset the ban counter for the proxy's IP

the IP is behind a trusted proxy and forwarded:

expected: reset the ban counter for the forwarded IP

current: reset the ban counter for the proxy's IP

ping @ArthurHoaro @nodiscc

virtualtam · 2017-11-27T20:53:30Z

This part of the refactoring is up for reviewing :)
I'll submit a separate PR to address #587

ArthurHoaro · 2017-12-02T14:06:54Z

application/LoginManager.php

+            }
+        }
+
+        $ipBans = &$this->globals['IPBANS'];


Is this for readability only? I would tend to say that it is a bad practice as you then don't see at first glance that you're manipulating a class attribute.

Is this for readability only?

Yup

you then don't see at first glance that you're manipulating a class attribute

Didn't think of it, that's a very valid point :)

ArthurHoaro

Very nice, especially regarding test coverage 👍

ArthurHoaro · 2017-12-02T14:09:37Z

application/LoginManager.php

+    public function handleSuccessfulLogin($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+        // FIXME unban when behind a trusted proxy?


It would make more sense in canLogin() as it's not possible to have a successful login while being banned.

Argh, my comment on this section was hidden after the last rebase... re-posting:

I'm not sure what to do for this specific point; according to my understanding of the login / ban mechanism, I'd expect this method to be on par with handleFailedLogin() as far as proxy handling is concerned:

the IP is not behind a trusted proxy:

current, expected: reset the ban counter for this IP

the IP is behind a trusted proxy but is not forwarded:

expected: do nothing

current: reset the ban counter for the proxy's IP

the IP is behind a trusted proxy and forwarded:

expected: reset the ban counter for the forwarded IP

current: reset the ban counter for the proxy's IP

Oh I didn't remember that. So, if I understand correctly, currently the ban mechanism is pretty useless if Shaarli is behind a reverse proxy, as the REMOTE_ADDR IP is always the proxy's one? It may be a good idea to expand a bit this comment and open a new issue?

the ban mechanism is pretty useless if Shaarli is behind a reverse proxy

it seems so, unless the user has:

properly configured the proxy / proxy chain to forward client information

configured the trusted proxy list in Shaarli settings

I think this point is not clearly mentioned in the docs, and I completely missed it while working on Docker images too 😿

https://shaarli.readthedocs.io/en/master/Shaarli-configuration/#security

expand a bit this comment and open a new issue

=> #1032

ArthurHoaro · 2017-12-02T14:12:42Z

index.php

-    if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.'));
+    if (! $loginManager->canLogin($_SERVER)) {
+        die(t('I said: NO. You are banned for the moment. Go away.'));
+    }
    if (isset($_POST['password'])
        && $sessionManager->checkToken($_POST['token'])
        && (check_auth($_POST['login'], $_POST['password'], $conf))


Shouldn't check_auth be a part of LoginManager?

... and so do isLoggedIn() and logout(), I'm doing this one step at a time :)

virtualtam · 2018-01-03T21:05:24Z

Rebased, let me know if you see other points to be addressed in this PR (and are not covered by #587 nor #1032) :)

Relates to shaarli#324 Added: - Add the `LoginManager` class to manage logins and bans Changed: - Refactor IP ban management - Simplify logic - Avoid using globals, inject dependencies Fixed: - Use `ban_duration` instead of `ban_after` when setting a new ban Signed-off-by: VirtualTam <[email protected]>

virtualtam · 2018-02-05T17:16:45Z

Rebased and merged, thanks for the reviews :)

Introduced by shaarli#1008 Signed-off-by: VirtualTam <[email protected]>

virtualtam added cleanup code cleanup and refactoring enhancement in progress security labels Oct 27, 2017

virtualtam added this to the 0.10.0 milestone Oct 27, 2017

virtualtam self-assigned this Oct 27, 2017

virtualtam requested review from ArthurHoaro and nodiscc October 27, 2017 16:49

virtualtam mentioned this pull request Oct 27, 2017

Memo: code refactoring #324

Closed

43 tasks

virtualtam force-pushed the refactor/authentication branch 2 times, most recently from ae90c6c to ff7fb9e Compare October 27, 2017 17:07

nodiscc previously approved these changes Oct 30, 2017

View reviewed changes

virtualtam force-pushed the refactor/authentication branch 2 times, most recently from 9fc5e93 to 897f57d Compare October 31, 2017 18:25

virtualtam added the template HTML rendering label Oct 31, 2017

virtualtam force-pushed the refactor/authentication branch 2 times, most recently from d509d54 to 48da9b5 Compare November 8, 2017 22:54

virtualtam force-pushed the refactor/authentication branch 4 times, most recently from f95593f to 041f8c4 Compare November 27, 2017 20:33

virtualtam commented Nov 27, 2017

View reviewed changes

virtualtam added in review and removed in progress labels Nov 27, 2017

virtualtam changed the title ~~[WIP] Refactor login / ban management~~ Refactor login / ban management Nov 27, 2017

virtualtam force-pushed the refactor/authentication branch from 041f8c4 to 47d6007 Compare November 27, 2017 20:57

ArthurHoaro reviewed Dec 2, 2017

View reviewed changes

virtualtam force-pushed the refactor/authentication branch from 47d6007 to f6f09f7 Compare December 2, 2017 21:16

virtualtam mentioned this pull request Dec 3, 2017

Rework and document IP ban management when behind a reverse proxy #1032

Closed

virtualtam force-pushed the refactor/authentication branch from f6f09f7 to 93fa075 Compare January 3, 2018 21:03

ArthurHoaro approved these changes Feb 2, 2018

View reviewed changes

virtualtam force-pushed the refactor/authentication branch from 93fa075 to 44acf70 Compare February 5, 2018 17:12

virtualtam removed the in review label Feb 5, 2018

virtualtam merged commit 91f17fc into shaarli:master Feb 5, 2018

virtualtam deleted the refactor/authentication branch February 5, 2018 17:16

virtualtam mentioned this pull request Feb 18, 2018

Refactor user login and session management #1086

Merged

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Mar 24, 2018

fix: IP ban check for the Vintage theme

adf4097

Introduced by shaarli#1008 Signed-off-by: VirtualTam <[email protected]>

virtualtam mentioned this pull request Mar 24, 2018

fix: IP ban check for the Vintage theme #1108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor login / ban management #1008

Refactor login / ban management #1008

virtualtam commented Oct 27, 2017

ArthurHoaro commented Oct 28, 2017 •

edited

Loading

nodiscc left a comment

virtualtam commented Oct 31, 2017

ArthurHoaro commented Nov 3, 2017

virtualtam Nov 27, 2017

virtualtam commented Nov 27, 2017

ArthurHoaro Dec 2, 2017

virtualtam Dec 2, 2017

ArthurHoaro left a comment

ArthurHoaro Dec 2, 2017

virtualtam Dec 2, 2017

ArthurHoaro Dec 3, 2017

virtualtam Dec 3, 2017

ArthurHoaro Dec 2, 2017 •

edited

Loading

virtualtam Dec 2, 2017

virtualtam commented Jan 3, 2018

virtualtam commented Feb 5, 2018

Refactor login / ban management #1008

Refactor login / ban management #1008

Conversation

virtualtam commented Oct 27, 2017

ArthurHoaro commented Oct 28, 2017 • edited Loading

nodiscc left a comment

Choose a reason for hiding this comment

virtualtam commented Oct 31, 2017

ArthurHoaro commented Nov 3, 2017

Choose a reason for hiding this comment

virtualtam commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurHoaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurHoaro Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virtualtam commented Jan 3, 2018

virtualtam commented Feb 5, 2018

ArthurHoaro commented Oct 28, 2017 •

edited

Loading

ArthurHoaro Dec 2, 2017 •

edited

Loading