Bug #46636

Authentication does not work any longer without redirects

Added by Marco Falkenberg about 8 years ago. Updated about 8 years ago.

Status:
Resolved
Priority:
Must have
Category:
Security
Start date:
2013-03-25
Due date:
% Done:

100%

Estimated time:
PHP Version:
Has patch:
No
Complexity:

Description

After applying the patch #46352 authentication via HTTP-Basic (PersistedUsernamePasswordProvider & UsernamePasswordHttpBasic-Token) throws

#1222268609: Access denied (0 denied, 0 granted, 1 abstained)

#1

Updated by Marco Falkenberg about 8 years ago

After some debugging I could locate the problem. The thing is that the Security\Context never will be informed about a new authenticated token, and under some circumstances Security\Context->getRoles() will be called before the token is authenticated.

This happens e.g. when you use a PersistedUsernamePasswordProvider. It tries to fetch a user from persistence. Then the PersistenceQueryRewritingAspect hooks in which calls Security\Context->getRoles() and... bam: The default roles Everybody and Anonymous are locked. And when UsernamePasswordHttpBasic-authentication is used, the authentication and access to the protected resource afterwards occurs in one request.

#2

Updated by Marco Falkenberg about 8 years ago

A solution would be to write a new slot which unsets the locked roles and connect it with the authenticatedToken signal of the AuthenticationProviderManager.

#3

Updated by Bastian Waidelich about 8 years ago

  • Subject changed from Broken Authentication via HTTP-Basic to Authentication does not work any longer without redirects
  • Priority changed from Should have to Must have
  • Target version set to 2.0

Marco, this one cost me some hours too..
The issue is not only true for HTTP authentication but for most authentication providers! Usually the issue doesn't matter though, because of a redirection to some other action.

The problem is that the PersistenceQueryRewritingAspect calls SecurityContext::getRoles() for all Query::execute() invokations. That means that the following code

$account = $this->accountRepository->findActiveByAccountIdentifierAndAuthenticationProviderName($credentials['username'], $this->name);

in PersistedUsernamePasswordProvider implicitly initializes the security context with the roles Everybody & Anonymous

#4

Updated by Bastian Waidelich about 8 years ago

  • Status changed from New to Accepted
  • Assignee set to Bastian Waidelich
#5

Updated by Gerrit Code Review about 8 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#6

Updated by Christopher Hlubek about 8 years ago

What about using some context to disable the PersistenceQueryRewritingAspect in the AccountRepository? This is a feature we would also need for policy enforcement. This would allow to execute queries (or call methods) without security in an explicit way. This is also useful for example in commands where usually no account is authenticated or in low-level tasks.

Pseudocode:


$this->securityContext->withoutAuthorization(function() use ($accountRepository, $credentials, &$account) {
    $account = $accountRepository->findActiveByAccountIdentifierAndAuthenticationProviderName($credentials['username'], $this->name);
});

The aspect could check for a global flag in Security\Context and have an early return.

Bastian Waidelich wrote:

The problem is that the PersistenceQueryRewritingAspect calls SecurityContext::getRoles() for all Query::execute() invokations.

#7

Updated by Karsten Dambekalns about 8 years ago

IIRC we ignored security in the aspect if the SecurityContext cannot be initialized - that used to work fine in the past. Hm.

#8

Updated by Bastian Waidelich about 8 years ago

Karsten Dambekalns wrote:

IIRC we ignored security in the aspect if the SecurityContext cannot be initialized - that used to work fine in the past. Hm.

There's even a test rewriteQomQueryDoesNotRewriteQueryIfSecurityContextCannotBeInitialized in PersistenceQueryRewritingAspectTest ;)

#9

Updated by Bastian Waidelich about 8 years ago

Bastian Waidelich wrote:

Karsten Dambekalns wrote:

IIRC we ignored security in the aspect if the SecurityContext cannot be initialized - that used to work fine in the past. Hm.

..But this has been changed with the aim to "enforce Query Rewriting more reliably": https://review.typo3.org/#/c/16106/

So maybe Christophers suggestion would be the way to go

#10

Updated by Bastian Waidelich about 8 years ago

Bastian Waidelich wrote:

So maybe Christophers suggestion would be the way to go

I just gave this a quick try and it seems to work fine! I'll push a WIP

#11

Updated by Gerrit Code Review about 8 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#12

Updated by Gerrit Code Review about 8 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#13

Updated by Gerrit Code Review about 8 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#14

Updated by Gerrit Code Review about 8 years ago

Patch set 5 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#15

Updated by Gerrit Code Review about 8 years ago

Patch set 6 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/20346

#16

Updated by Gerrit Code Review about 8 years ago

Patch set 1 for branch 2.0 has been pushed to the review server.
It is available at https://review.typo3.org/20586

#17

Updated by Bastian Waidelich about 8 years ago

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

Also available in: Atom PDF