RFC: AccountInterface and Authentication rework

Continuing the discussion from Could we build a version of Flow without doctrine orm / database requirement:

I would like to suggest a major conceptual change in the whole authentication implementation in Flow & Neos.
tl;dr I suggest a different mental model, code-wise the change is not that big, see code examples below

Lets start with the problem space and collect some issues we currently have:

  • The security framework is bound to the Account entity thus requires persistence & a database to be setup (the main reason for the original thread to be posted)
  • It’s not possible to authenticate the same account with different auth types (e.g. username/password & basic auth) leading to quirky work-arounds and bugs
  • When implementing 3rd party authentication providers you have to create an Account entity instance that must not be persisted (which is not only weird but error-prone)

IMO it was a bad idea to bind the account to its auth provider. And as a follow-up to have the Party (i.e. Neos Users) be related to multiple accounts potentially.
Especially the latter doesn’t really make sense if you think about it:
What permissions do you currently have if two accounts are authenticated that belong to the same party but have different roles?

I think we should built upon @sorenmalling’s great work on the AccountIdentifier but also change some fundamental behavior by extracting credentials from the account.
And instead of binding an account to an Authentication Provider we could introduce the notion of an Authentication Realm (e.g. “Neos Backend” or “Some custom FE login”, …).

As a result the AccountInterface would become as simple as:

interface AccountInterface
{

    public function getIdentifier(): AccountIdentifier;

    public function getRealm(): AuthenticationRealm;

    public function getRoles(): Roles;

}

And there would be a corresponding entity, for example:

/**
 * @Flow\Entity
 */
final class PersistedAccount implements AccountInterface
{
    // ...
}

…and an implementation for “3rd party accounts” (SSO etc):

final class TransientAccount implements AccountInterface
{
    // ...
}

For the default username/password authentication we would have one more entity that encapsulates what we currently have as accountIdentifier and credentialsSource, for example:

/**
 * @Flow\Entity
 */
final class UsernameAndPassword
{
    public function getAccountIdentifier(): AccountIdentifier
    {
        // ...
    }

    public function getRealm(): AuthenticationRealm
    {
        // ...
    }

    public function getUsername(): Username
    {
        // ...
    }

    public function getHashedPassword(): HashedPassword
    {
        // ...
    }
}

For backwards compatibility we could keep the current Account entity and turn it into an adapter, like:

/**
 * @Flow\Entity
 * @deprecated
 */
class Account implements AccountInterface
{

    public function getCredentialsSource(): ?string
    {
        $credentials = $this->usernameAndPasswordRepository->findOneByAccountIdentifier($this->accountIdentifier);
        return $credentials ? $credentials->getHashedPassword()->toString() : null;
    }
    
    // and so on
}

The PersistedUsernamePasswordProvider::authenticate() would change from (simplified):

$account = $this->accountRepository->findActiveByAccountIdentifierAndAuthenticationProviderName($username, $this->name);

$authenticationToken->setAuthenticationStatus(TokenInterface::WRONG_CREDENTIALS);

if ($account === null) {
    // error
}

if (!$this->hashService->validatePassword($password, $account->getCredentialsSource())) {
    // error
}

to (simplified):

$realm = $this->options['realm'] ?? $this->name;
$accountCredentials = $this->usernameAndPasswordRepository->findOneByUsernameAndRealm($username, $realm);
if (!$this->hashService->validatePassword($password, $accountCredentials->getHashedPassword())) {
    // error
}
$account = $this->persistedAccountRepository->findActiveByAccountIdentifierAndRealm($credentials->getAccountIdentifier(), $realm);

In order to implement parallel authentication mechanisms we’d no longer need one account per provider (which doesn’t make sense as described above). Instead we could specify the “realm” in the corresponding provider (potentially with the provider name as default value for b/c and easier configuration):

Neos:
  Flow:
    security:
      authentication:
        providers:
          'Acme.SomePackage:Default':
            provider: PersistedUsernamePasswordProvider
            token: UsernamePassword
          'Acme.SomePackage:Default.HttpBasic':
            provider: PersistedUsernamePasswordProvider
            providerOptions:
              realm: 'Acme.SomePackage:Default'
            token: UsernamePasswordHttpBasic
            entryPoint: HttpBasic

And finally, in Neos, we’d change User::getAccounts(): Account[] to User::getAccount(): AccountInterface

1 Like

nobody interested? :no_mouth:

This is a really good suggestion, I definitely like it. “Realm” also instantly makes sense to me and the change seems to have a good migration path.

I like this so much that I actually would like to join the effort and help you out, if you like :wink:

2 Likes

Somehow missed this post, or at least didn’t get around reading into it enough to form an opinion :slight_smile:

The union of the permissions = roles of all accounts I would say intuitively. Hard to compute in the brain for debugging and ofc has potential for flakey behaviour based on authentication ordering or conflicting role permissions.

IMO it was a bad idea to bind the account to its auth provider.

I always considered the configured provider name (the setting key, not the provider implementation, i.e. UsernamePassword*!) to be something that roughly matches your concept of a “Realm” now (as far as I understood it). At the core maybe a naming issue :slight_smile: At least in combination with a RequestPattern. You’d have a NeosBackend, SomeCustomFrontend etc. in your security.authentication.providers settings and then you’d need to query the current account based on the provider. So by connecting the account to the “providerName”, you effectively encode which accounts are applicable in which “realm” (i.e. this is a “NeosBackend” account).
The issue with that I guess is that you easily end up with multiple providers being active for one request (because you need to specify request patterns for each configured provider name, and what if you want to have multiple providers for one “realm”, i.e. multi-factor auth). Hence permissions potentially don’t match the expected (due to the above mentioned ambiguity).

That’s btw something I don’t yet see solved in your proposal - how do you bind a “Realm” to some request patterns? And how is the “Realm” Acme.SomePackage:Default connected to the configured provider Acme.SomePackage:Default? By the name prefix?

Otherwise, if I understand correctly, the basic change is that Account::providerName is renamed to “realm” and made part of the new AccountInterface? Approve :white_check_mark:

Thanks for your feedback, it’s highly appreciated!

Right, that probably makes the most sense. But it could add quite a lot of complexity and intransparent behavior, especially when using GRANT and DENY privileges…

The core issue is not the name IMO but the 1:1 mapping:
A Realm would not just be a renamed provider but an abstraction on top. You’ll ask the security context getAuthenticatedAccountByRealm($realm) for example and it would return the first authenticated account that matches the realm, independently on the provider that was used.

Or, to put it differently: It allows you to fetch the authenticated Neos Backend user, no matter whether it was authenticated via username/password or LDAP or authorization header or …

That’s true, but IMO not a big deal in reality since you should not care usually. And if you care about the provider, you can still use getAccountByAuthenticationProviderName() and/or use RequestPatterns.

By the new realm option:

Neos:
  Flow:
    security:
      authentication:
        providers:
          'Acme.SomePackage:Default.HttpBasic':
            provider: PersistedUsernamePasswordProvider
            providerOptions:
              realm: 'Acme.SomePackage:Default'

If that’s not specified we could fall back to realm name <=> provider name, as suggested.

Right, but they are not the same thing. The realm defines the “domain” this account belongs to, while the provider was a technical concept describing the authentication mechanism