Project

General

Profile

Actions

Bug #91993

closed

Official documentation and actual implementation of AuthenticationService is fatally mismatching

Added by Stefan P over 3 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Authentication
Target version:
-
Start date:
2020-08-13
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
10
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

The documentation states:

a value of 0 or a value of 100 or more indicates that the authentication has failed, but that further services should keep trying.

But the actual code treats 0 like totally failed, to NOT try further (it checks the return code for greater 0 not greater--or-equal 0):

foreach ($this->getAuthServices($subType, $loginData, $authInfo) as $serviceObj) {
    if (($ret = $serviceObj->authUser($tempuser)) > 0) {
        // If the service returns >=200 then no more checking is needed - useful for IP checking without password
        if ((int)$ret >= 200) {
            $authenticated = true;
            break;
        }
        if ((int)$ret >= 100) {
        } else {
            $authenticated = true;
        }
    } else {
        $authenticated = false;
        break;
    }
}

if ($authenticated) {
    // Leave foreach() because a user is authenticated
    break;
}

Luckily we did not trust the docs blindly in one of our projects but checked the core code first - else we had run in severe problems.

This must be fixed one way or the other.

IMHO, the docs should be correct. It must be possible to override a failed authentication by custom code. This is not possible if the service chain just ends hard.

Currently overriding failed core auth is only possible if an unknown hash algorithm is used.

Our use cas ist this:
  • If a user is not "blocked" (check in a custom auth service with priortity >50), then just run core auth, else fail hard (negative return).
  • if not blocked and core auth fails (wrong password given) we increase a failed login attempt counter and eventually block the user (custom auth service with priority <50)
  • when core auth succeeds we unblock the user in the custom auth with priotity <50

This is not possible currently.


Files

auth.png (6.73 KB) auth.png Sybille Peters, 2020-08-13 18:52
Actions #1

Updated by Sybille Peters over 3 years ago

Adding some links for convenience:

Actions #2

Updated by Sybille Peters over 3 years ago

  • I would fix the docs to match the core - because I do not know if this will get changed in the core and how long that may take. It is in any case a change that may break things.
  • I also pointing out the previous mismatch between docs + core in the PR. I also added a table which - for me - makes it easier to see the differences.

  • I also checked in the core, if this has been changed previously (e.g. see "git blame"). Does not look so.
  • It seems to me you might have a point.
Actions #3

Updated by Stefan P over 3 years ago

I would fix the docs to match the core

But then it's never possible to do follow-up core authentication (only for non-existing hash algorithms).

Imagine this:
  • A user enters correct password and the core auth should upgrade the hash to latest standards (I want to have this feature)
  • the follow-up auth service should still check for actual authentication (sometimes it's not enough to simply have a correct password, but maybe a wait-time to be able to login)

If the core always says "correct password = authenticated, period" all my PRE-core auth services HAVE TO check the password as well (which is error prone and code duplication).

I just want to leave the password check (with hash upgrading mechaisms and everything) to the core and do my additional non-password-related checks after this.

Actions #4

Updated by Sybille Peters over 3 years ago

I did not want to argue, that you are not right.

I just wanted to have the docs up to date and in sync with the core until this is resolved somehow. (As you said, this cost you some time until it got figured out).

The change in the docs has now been merged:

This is what it used to say:

a negative value indicates that the authentication has definitely failed and that no other “auth” service should be called up.
a positive value smaller than 100 indicates that the authentication was successful, but that further services should also perform their own authentication.
a value of 0 or a value of 100 or more indicates that the authentication has failed, but that further services should keep trying.
a value of 200 or more indicates that the authentication was successful and that no further tries should be made by other services down the chain.

Actions #5

Updated by Stefan P over 2 years ago

  • TYPO3 Version changed from 9 to 10
Actions #6

Updated by Sybille Peters over 1 year ago

  • Status changed from New to Closed

@Stefan P - if you still think core behaviour should get changed, please handle as separate issue and link to this as related.

Actions

Also available in: Atom PDF