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_accounttable:- rename
authenticationprovidername=>realm - extract
credentialssourceto new table (e.g.neos_flow_security_usernameandpasswordas suggested above – note: these are credentials but specific ones for username & password authentication, so that name makes sense to me) - extract
lastsuccessfulauthenticationdateandfailedauthenticationcountinto a separate table (potentially into a separate package even… to be discussed) - provide corresponding doctrine migrations for migrating structure and data
- rename
- Revert some changes on the
Accountentity to make it backwards-compatible again, add deprecation annotations and implement “adapter” methods likegetRoles()andgetCredentialSources()as described above - Remove all/most setters from
Accountsince something likesetCredentialsSource()can’t work any longer as before - Introduce
PersistedAccountRepository(as suggested replacement for to be deprecatedAccountRepository) andUsernameAndPasswordRepository(to be used inAccount:: getCredentialsSource()and in Username / Password providers)