Bug #29488

AuthenticationManager::authenticate() does not throw Exception for invalid credentials

Added by Carsten Bleicker about 10 years ago. Updated almost 10 years ago.

Status:
Resolved
Priority:
Should have
Category:
Security
Start date:
2011-09-05
Due date:
% Done:

100%

Estimated time:
PHP Version:
Has patch:
Complexity:

Description

if i send empty login data the result of authentication is allways true here.
can somebody reproduce this? exception is also not thrown.
talking about this part:

public function authenticateAction() {

    $authenticated = FALSE;
    try {
        $this->authenticationManager->authenticate();
        $authenticated = TRUE;
    } catch (\TYPO3\FLOW3\Security\Exception\AuthenticationRequiredException $exception) {
        // No Exception is thrown if user sends empty form?
        var_dump($exception);
    }

    /**
     * At this point $authenticated is allways true if user sends empty form?
     */ 
    var_dump($authenticated);
    die();

    if ($authenticated) {
    ........
}

#1

Updated by Karsten Dambekalns about 10 years ago

  • Status changed from New to Needs Feedback

Andi, can you shed some light on this?

#2

Updated by Bastian Waidelich about 10 years ago

  • Subject changed from authenticationAction allways True? to AuthenticationManager::authenticate() does not throw Exception for invalid credentials
  • Assignee set to Andreas Förthner
  • Priority changed from Should have to Must have

I can reproduce this with the default setup.
To fix this you can change the authentication strategy. for instance like this:

TYPO3:
  FLOW3:
    security:
      authentication:
        authenticationStrategy: atLeastOneToken

But if I get it right, the behavior of the default strategy (anyToken) is not correct.

#3

Updated by Karsten Dambekalns about 10 years ago

This would be "expected" behavior for the anyToken authentication strategy - authenticated doesn't mean authorized. If anonymous authentication is not to be allowed, change the authentication strategy to atLeastOneToken.

But is that what "people" would expect?

#4

Updated by Bastian Waidelich about 10 years ago

Bastian Waidelich wrote:

But if I get it right, the behavior of the default strategy (anyToken) is not correct.

..I didn't get it right obviously. The behavior of anyToken is described with "Authenticate as many tokens as possible but do not require an authenticated token (e.g. for guest users with role Everybody)."

Still, I think we should either change the default strategy or make it easier to check the authentication status.
Currently we'd have to do

$activeTokens = $this->securityContext->getAuthenticationTokens();
foreach ($activeTokens as $token) {
    if ($token->isAuthenticated()) {
        // is authenticated
    }
}

Another option might be to be able to override the strategy by a parameter in the authenticate() method?

#5

Updated by Andreas Förthner about 10 years ago

As already discussed, the behaviour is correct. But maybe we really should change the default strategy. On the other hand, anyToken ist the strategy with less errors being thown. So it might be a good choice, as it doesn't break something if you do nothing...

#6

Updated by Bastian Waidelich about 10 years ago

Andreas Förthner wrote:

On the other hand, anyToken [...] doesn't break something if you do nothing...

Except for your security if you happen to forget to reconfigure the strategy ;)
I think, the default use case is standard username/password authentication so we should probably have the default configuration "safe" - but we can discuss that again at T3CON!

#7

Updated by Bastian Waidelich about 10 years ago

  • Status changed from Needs Feedback to Accepted
  • Assignee changed from Andreas Förthner to Bastian Waidelich
  • Priority changed from Must have to Should have
  • Target version set to 1230

As discussed at T3CON11 we should change the default strategy from "anyToken" to "atLeastOneToken" as that will fit the most common use case of authenticating via username & password.

#8

Updated by Mr. Hudson about 10 years ago

  • Status changed from Accepted to Under Review

Patch set 1 of change Ia3d15f8e5e900ccc3b5be1b22b668d5ddadad7c8 has been pushed to the review server.
It is available at http://review.typo3.org/5750

#9

Updated by Mr. Hudson about 10 years ago

Patch set 2 of change Ia3d15f8e5e900ccc3b5be1b22b668d5ddadad7c8 has been pushed to the review server.
It is available at http://review.typo3.org/5750

#10

Updated by Bastian Waidelich about 10 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#11

Updated by Karsten Dambekalns almost 10 years ago

  • Target version changed from 1230 to 1.0.0

Also available in: Atom PDF