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

Ah, so it’s just “by coincidence” that the realm is equal to the provider name prefix in the example. That was leading me in the wrong direction.

The core issue is not the name IMO but the 1:1 mapping

Indeed.

I would say the realm would default to be equal to the provider name, as mentioned above:

So in the example configuration, both providers would belong to the realm Acme.SomePackage:Default , but I could have made it a bit clearer, for example:

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

BTW: I’ll try to tackle this for the next major together with:

1 Like

As we are already working in this section. What would you think to rework the Impersonante PR from Dominque. The would be also an awesome feature for the next Major version and people would love that I guess :slight_smile:

https://github.com/neos/neos-development-collection/pull/2269

I just had a great talk with @sorenmalling since we plan to work on this in the upcoming days and weeks.

I still think that the approach discussed above would work out and provide a much cleaner foundation for future extensions (like the account impersonation @markusguenther mentioned).

There are (at least) two challenges we currently face though:

1. Account::getRoles() currently returns an array of Role objects including their parent roles.

When creating account instances, this is cumbersome, error prone and “expensive” because you’ll always have to use the PolicyService to create the Role instances:

$roles = array_map([$this->policyService, 'getRole'], $roleIds);

Changing that to only return a flat array of role ids would be a hell of a breaking change though.

We decided we would rename the getter in the new interface instead to:

interface AccountInterface
{

    //...

    public function getRoleIdentifiers(): RoleIdentifiers;

}

The Account implementation could then do the conversion lazily like:

class Account implements AccountInterface {
    public function getRoles(): array
    {
        return array_map([$this->policyService, 'getRole'], iterator_to_array($this->getRoleIdentifiers()));
    }
}

2. We’ll lose information about the authentication provider

Account::getAuthenticationProviderName() would no longer be able to work reliably since we’d only persist the Realm as described above.

I’d say that we could just do

/* @deprecated use PersistedAccount or AccountInterface */
class Account {
  /**
   * @deprecated bla bla, this will return the Realm, use getRealm() instead
   */
  public function getAuthenticationProviderName(): string
  {
     return (string)$this->getRealm();
  }
}

Next steps

  • Rebase the existing PR to current master
  • Introduce the new interfaces and classes as described above
  • Split neos_flow_security_account table:
    • rename authenticationprovidername => realm
    • extract credentialssource to new table (e.g. neos_flow_security_usernameandpassword as suggested above – note: these are credentials but specific ones for username & password authentication, so that name makes sense to me)
    • extract lastsuccessfulauthenticationdate and failedauthenticationcount into a separate table (potentially into a separate package even… to be discussed)
    • provide corresponding doctrine migrations for migrating structure and data
  • Revert some changes on the Account entity to make it backwards-compatible again, add deprecation annotations and implement “adapter” methods like getRoles() and getCredentialSources() as described above
  • Remove all/most setters from Account since something like setCredentialsSource() can’t work any longer as before
  • Introduce PersistedAccountRepository (as suggested replacement for to be deprecated AccountRepository) and UsernameAndPasswordRepository (to be used in Account:: getCredentialsSource() and in Username / Password providers)
3 Likes

I would like to thank you for all the ideas and effort you put in that part of flow.

So far I can only agree and I like what you do.
The things with the Impersonate feature have to be adjusted again anyway :slight_smile:

2 Likes

I know I’m pretty late to the party but I somehow missed this thread.
I agree with most of the things but just wanted to give an example about how we use this part in Passcreator where it makes sense I think (also I’m open to hearing that we did something stupid if that should be the case :wink: ).
We use the Party package to create a User object. That object can have multiple accounts attached to it. One account is using the default provider (UsernamePassword e.g.) and other accounts are API keys using a different provider for example.
The reason why I think this makes sense is that you can have a user and one account that is used to authenticate via the web interface. Then the user can create an number of access tokens for the API (e.g. integration one, integration two, dev etc.) and also remove these keys if needed. E.g. a user would want to remove the dev key at some point in time.
Then there’s some other thing we du currently: when you log in using our Redemption app, we authenticate a user using Basic Auth. This authentication uses the default account of a user.
Now that we know that this is a login from the app, we automatically generate a new access token (= Flow account attached to the user) and assign that account only the permissions that are needed for the app, even if the user is allowed to do more.
The rationale there is simply to minimize attack surface in case someone get’s ahold of credentials stored in the app. We currently don’t use OAuth so things might be a bit different there but in general I think what’s pretty great there is that you can have one user with different access “models” and assign these cases only the permissions that are needed. And you can also remove access pretty easily without breaking everything where access tokens may be used.

Thanks for the use-case description :slight_smile: I do think it makes a lot of sense and using multiple accounts was probably the only method to achieve this. But with the suggested change, this should still be possible and even easier to achieve AFAIS (having that use-case at hand might help though).

Did you run into the issue Bastian mentioned?

What permissions do you currently have if two accounts are authenticated that belong to the same party but have different roles?

No. We’ve built it in a way that in the Frontend you can only use a default account which means there’s only one “stateful account”. Everywhere where permissions may differ are only used in stateless transactions (e.g. our app creates a record that a pass was scanned or a user-generated API key is used to create a pass). Obviously if there would be two accounts that can have a session this might be an issue but we’ve solved it by not allowing that.

Thanks for the interesting scenario.

As @aberl wrote, you should be able to keep your code more or less with 7.0+ (if everything works out as planned).
However, with proper TransientAccount s in place and the username & password separated from the account model, you could also do something like this:

in a custom provider:

$accountCredentials = $this->usernameAndPasswordRepository->findOneByUsernameAndRealm($username, $realm);
if (!$this->hashService->validatePassword($password, $accountCredentials->getHashedPassword())) {
    // error
}

$dynamicRoleIdentifiers = // ...

$account = TransientAccount::create($accountCredentials->getAccountIdentifier(), $realm, $dynamicRoleIdentifiers);

I.e. it would be a single (transient) account with a dynamic set of roles, depending on the context/authentication method