Hi @sharidas are there any news on that PR?
cc @phil-davis fyi

@micbar what happens to old PRs like this?

@janackermann @JammingBen Can you do a rebase?

@janackermann @JammingBen Can you do a rebase?

Yes we can, but this code doesn't work proper, we need to make changes to the core aswell to make it work

We digged into this and created a new branch: https://github.com/owncloud/encryption/compare/revisit-symfony-events. The issues we have encountered so far:

• We have to emit 2 additional calls in the core. Otherwise the events user.aftercreate and user.aftersetpassphrase wont be fired.
• The user.afterdelete behaves a little bit strange during the tests. Let me try to explain this:

Some tests do a tearDown after they have finished. This tearDown calls $user->delete(); at some point, which ends up emitting an event: $this->emitter->emit('\OC\User', 'postDelete', [$this]);. Currently this does not trigger the UserHook defined in this app during tests (which is the correct behaviour). But after implementing the Symfony hooks, this hook will be fired which causes the tests to fail.

I am unsure about how to fix this properly because it may affect other scenarios. Possible solutions that come into my mind:

• Renaming the event in $this->emitter->emit('\OC\User', 'postDelete', [$this]);
• Renaming the Symfony-event in the UserHook.php. This solution would also require renaming the firing event emitter the core Server.php file

Maybe @micbar or @DeepDiver1975 could give us a hint to find an appropriate solution here?

cc @janackermann

Edit: After some more debugging this behaviour seems to be directly and only related to the tests. The Symfony event listeners of UserHook.php seem to listen during the whole test run, so the corresponding events get fired. This was not the case with the previous event system.

Edit2: We resolved the issue

All committers have signed the CLA.

@JammingBen mind to sign the cla? it is just a click away in the link provided by the cla assistant 😄

Resolved the above mentioned issues in the meantime, still need to fix the acceptance tests though.

@JammingBen can you also do a rebase?

The pending PR became obsolete as I found a way to use the existing event emitters in core.

However, there is still one acceptance test failing: tests/acceptance/features/apiShareManagementToRoot/acceptShares.feature:682 (https://drone.owncloud.com/owncloud/encryption/1550/33/16). After some debugging it looks like the user is missing all his skeleton files. The shared files are available though. Also, the problem only occurs if the encryption type is set to MasterKey. @phil-davis Any idea what is happening here?

The last nightly run of tests with encryption on had a list of fails in tags scenarios: https://drone.owncloud.com/owncloud/encryption/1552/120/17 - those happened with both master and user key encryption. I will follow that up in the morning.

https://drone.owncloud.com/owncloud/encryption/1552/120/17 - the test fails also with user key encryption.

That test is supposed to have the sequence:

• an admin creates user David, and David has never accessed his account yet
• Alice shares something to David that matches what is also "coming" in the skeleton
• David does his first action ever, tries to access his files/folders (at this point the code to create the skeleton should happen)
• David should see his skeleton files, except that he will see the share from Alice that has already "arrived" before the skeleton creation.

The last nightly run of tests with encryption on had a list of fails in tags scenarios: https://drone.owncloud.com/owncloud/encryption/1552/120/17 - those happened with both master and user key encryption. I will follow that up in the morning.

That is perhaps related to owncloud/core#38197.

https://drone.owncloud.com/owncloud/encryption/1552/120/17 - the test fails also with user key encryption.

Ahh you're right, it also happens with user key encryption :/ Maybe the user hooks still act different than the old ones... I will do some more tests on this.

Phew... finally found the issue here. The Symfony Event Dispatcher during loginWithPassword sits after the prepareUserLogin-call (see https://github.com/owncloud/core/blob/master/lib/private/User/Session.php#L541). Whereas the original hook is located before that (see https://github.com/owncloud/core/blob/master/lib/private/User/Session.php#L537). That means the filesystem is now being set up before the login event is called.

When I move the event dispatcher a few lines up to where it was before, the tests work fine. @phil-davis Do you know the intention behind this? The question now is, do we change the position of the event emitter, or do we adjust this test scenario...

The scenario is intended to test that a user can be created by the admin, and between the time that the user is created and the time that the user actually receives their password and logs in (might be 24 hours later or...) people share things with the new user. Those shares should work, and be seen by the user when they login, even if the received shared resource happens to have a name that matches a skeleton file or folder.

The scenario Then steps check what is the current behaviour, because that is what passes. But actually I don't think that the "requirement" for that is defined anywhere. For example, if there is a folder docs in the skeleton, and some other user shares a folder something/sub/docs then the new user should have a docs folder and a docs (2) folder. But it really does not matter which one is from the skeleton and which one is the share.

The test could be re-factored so that it allows either way around.

But it would be nice if the behaviour in core without encryption is the same is with encryption enabled. Otherwise someone will get confused about it some day.

But it would be nice if the behaviour in core without encryption is the same is with encryption enabled. Otherwise someone will get confused about it some day.

I second that.

So in summary: the problem is the position of the dispatched user.afterlogin-event. It fires after prepareUserLogin() which requires the encryption to be already set up. Listing to the user.beforelogin-event on the other hand also won't work.

Is there a reason why the user.afterlogin-event is fired after prepareUserLogin() when logging in with password? This isn't the case in similar methods:

• https://github.com/owncloud/core/blob/master/lib/private/User/Session.php#L603 (login with token)
• https://github.com/owncloud/core/blob/master/lib/private/User/Session.php#L698 (login with apache)

Seems to be very inconsistent... Maybe @DeepDiver1975 @jvillafanez you guys know something about this decision?

Seems to be very inconsistent... Maybe @DeepDiver1975 @jvillafanez you guys know something about this decision?

I don't know. It should be consistent. If there is no comment explaining and the commit introducing the change doesn't say anything about it, for me it's a bug.

Once owncloud/core#38289 goes through, this one might finally be ready (to review).

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information