Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

!!! FEATURE: Introduce Account Interface #1538

Open
wants to merge 17 commits into
base: master
from

Conversation

@sorenmalling
Copy link
Contributor

commented Apr 6, 2019

Based upon this discuss.neos.io thread

https://discuss.neos.io/t/could-we-build-a-version-of-flow-without-doctrine-orm-database-requirement/2187/16

This change introduces a AccountInterface to serve for handling
with non-persisted Account during authentication as well as
the possibility of a base interface to implement authentication
classes.

Checklist

  • AccountIdentifier class
  • AccountProviderName class
  • CredentialsSource class
  • Roles collection
  • Replace usage of getAuthenticationProviderName?
  • Replace usage of getIdentifier?
  • Replace/adjust relations to the Account entity with AccountInterface
  • ... ?
  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch

Breaking change

This is a breaking change, since the getCredentialsSource of the AccountInterface class no longer return mixed, but a exact object. This is shown in the PersistedUsernamePasswordProvider class, where the password is cated to string, taking a __toString method into effect.

Related #1077

sorenmalling added some commits Apr 6, 2019

[!!!][FEATURE] Introduce Account Interface
This change introduces a AccountInterface to serve for handling
with non-persisted Account during authentication as well as
the possibility of a base interface to implement authentication
classes.
@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

It’s this that is failing

$this->mockAccount->expects($this->once())->method('getCredentialsSource')->will($this->returnValue('8bf0abbb93000e2e47f0e0a80721e834,80f117a78cff75f3f73793fd02aa9086'));
since it returns a object (CredentialsSource ) - I either create a getCredentialsSource() method inside the CredentialsSource class (kind of double-double-naming) or cast it to string (somehow)? What way should I go?

@daniellienert daniellienert changed the title [!!!][FEATURE] Introduce Account Interface !!! FEATURE: Introduce Account Interface Apr 7, 2019

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Not being remotely into how Unit test and object mocking works, I would some feedback on how to get the Unit tests to pass.

Inside Neos\Flow\Tests\Unit\Security\Authentication\Provider\PersistedUsernamePasswordProviderTest the methods authenticatingAnUsernamePasswordTokenChecksIfTheGivenClearTextPasswordMatchesThePersistedHashedPassword and authenticationFailsWithWrongCredentialsInAnUsernamePasswordToken contains the following line

$this->mockAccount->expects($this->once())->method('getCredentialsSource')->will($this->returnValue('8bf0abbb93000e2e47f0e0a80721e834,80f117a78cff75f3f73793fd02aa9086'));

But since the getCredentialsSource method now returns a Object, how should I go around this? I either create a getCredentialsSource() method inside the CredentialsSource class (kind of double-double-naming) or cast it to string (somehow)? What way should I go? Or a third way to adjust these Unit tests?

@daniellienert

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

About the tests:

Just replace:

$this->mockAccount->expects($this->once())->method('getCredentialsSource')->will($this->returnValue('8bf0abbb93000e2e47f0e0a80721e834,80f117a78cff75f3f73793fd02aa9086'));

with

$this->mockAccount->expects($this->once())->method('getCredentialsSource')->will($this->returnValue(new CredentialSource('8bf0abbb93000e2e47f0e0a80721e834,80f117a78cff75f3f73793fd02aa9086')));

Didn't have a look where your value objects are used in the code, but please at a (string) cast everywhere where they are used as implicit casting will fail once we declare strict typing in these classes.

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

All checks are green 🎉

Neos.Flow/Classes/Security/Account.php Outdated Show resolved Hide resolved
@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

I suggest that the task

Replace/adjust relations to the Account entity with AccountInterface

Is being a follow-up task, once we got the interface in and I will create a separate issue for that

@bwaidelich
Copy link
Member

left a comment

@sorenmalling Great initiative, thanks for starting this!
I added a few (partly nitpicky) comments (and if you agree, I can take care of those myself).
Apart from that, shouldn't AccountInterface has getRoles(): AccountRoles and hasRole(Role $role): bool as well?

Neos.Flow/Classes/Security/AccountIdentifier.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/AccountIdentifier.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/AccountIdentifier.php Outdated Show resolved Hide resolved
public function __construct(string $identifier)
{
$this->identifier = $identifier;
}

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Apr 8, 2019

Member

I would suggest to keep the constructor private and add a

public static fromString(string $identifier): self
{
    return new static($identifier);
}

That will allow us to add more constructors lateron

This comment has been minimized.

Copy link
@sorenmalling

sorenmalling Apr 10, 2019

Author Contributor

But only for this class? And not for the others (AuthenticationPRoviderName, CredentialsSource)?

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Aug 9, 2019

Member

Yes, your're right. lets do it for all new VOs

@daniellienert

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Hey @sorenmalling,
IMHO that looks much better now.
I added some more nitpicks.

It would further be great if you would squash your commits into one with a good description what has been changed.

And then, we need a PR in the Neos Repo also, changing the usages of the new methods.

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Thanks for the feedback - new versions coming up this evening :-)

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@bwaidelich I get to a point, where my Unit testing knowledge and Securirty framework is "not there yet".
A number of tests fails, due to change from a array of Role objects to the new Roles collection class.

Can you help me adjust the tests to match the new format?

I more or less copy/pasted the Roles class and added ArrayAccess to the implementation.

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

ping @bwaidelich

This is my current output when running the tests

https://gist.github.com/sorenmalling/c3f599a4ebc0f2ee53058cea18c41f18

@albe albe force-pushed the sorenmalling:feature/account-interface branch from e1fd747 to b2ac43a May 3, 2019

@albe

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Sorry, had to force push after a messed up merge and push beforehand. Did a couple of fixes to the code that prevented tests from running. Still WIP, but getting there.

albe added some commits May 3, 2019

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Trying to go over the failing tests, I can't wrap my head around the issue and how to solve it.

Is this feature considered something that should go in to Flow 6, then I will fight for it (also for my own projects sake).

If not, I will like to request some help after the Flow 6 release.

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Is this feature considered something that should go in to Flow 6 [...]

Depending on the potential "breakiness" I would love to have this in Flow 6.

Let me know if you need help with the tests

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Is this feature considered something that should go in to Flow 6 [...]

Depending on the potential "breakiness" I would love to have this in Flow 6.

Well, a little bit breaky, since we add some collections classes instead of arrays.

Let me know if you need help with the tests

I do need help ;-) I havent been writing tests before and the concept of mocking etc is very new and never-touched ground.

@albe

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

IMO definitely something for 6.0, otherwise this needs to be postponed to 7.0

PS: I currently lack the time to help further with this, as I have a couple of other open PRs, mainly the PSR move one. So if Bastian can jump on in for fixing the tests, that would be awesome

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I do need help ;-)

@sorenmalling Sorry, I missed your previous comments somehow.. Up for a quick chat/call tomorrow to go through the remaining tasks?

@sorenmalling

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

I do need help ;-)

@sorenmalling Sorry, I missed your previous comments somehow.. Up for a quick chat/call tomorrow to go through the remaining tasks?

@bwaidelich I will be offline till sunday, but will give it a try with fresh eyes, and ping you next week with lasting issues. I will love to get this in and planned time for it :) Thanks for helping me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.