Add some user related events

[dermalikmann]

This issue initially popped up when looking into stjosh/auto_groups#74

For now only added:

• UserLoggedInEvent
• UserLoggedOutEvent
• UserDeletedEvent

UserCreatedEvent should be added as well, but the ClassConstructor expects the users password, and I would need to investigate what possible options we have at this stage, as the user's real password is not known.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

[elainabialkowski]

Is there any update on this PR? I've noticed the checks having been sitting at Expected for a hot second. Is there anything I can do to help move this forward?

[blizzz]

I am the bottle neck, I am afraid. Thank you for your contribution, @dermalikmann!

[blizzz]

Could we also get in a test? There was one good attempt there that probably can be taken over, with adjusted classes: https://github.com/nextcloud/user_saml/pull/870/files#diff-de828f9df04940c610cecf388767da55bd9e27446a31c25ea1766c3d796d098e

[blizzz]

@dermalikmann apologies for the late response and review. Are you still up for this?

[dermalikmann]

I just read through #870
i could try to implement a test, but just as the other person, im not too familiar with nextcloud dev, so i need to take a look first. but you can expect something in a couple of hours.

[dermalikmann]

@blizzz I copied over the test @x7airworker did in their PR.
I wanted to test for the other events as well, but neither the logout nor the user delete procedures have any tests yet, and I am not familiar enough with phpunit and this addon to implement even stubs for those tests. sorry 😅

Feel free to merge, resolve the conflict by using the incoming changes (mine).

[blizzz]

I wanted to test for the other events as well, but neither the logout nor the user delete procedures have any tests yet, and I am not familiar enough with phpunit and this addon to implement even stubs for those tests. sorry 😅

Yes, and it is a bit nightmarish. We have integration tests, but those treat it as a black box. We can implicitly assume that they are covered. While not asserted, the call is simple and there is no complex logic behind, so it should get through. Not as good as an explicit test, but acceptable.

Rebased, resolved the conflict, and added minor fixes and style corrections.

[blizzz]

… oh but this tests keeps failing now, needs another closer look.

[dermalikmann]

Ok, so I took another look at this, and it turns out, blindly copying someone else's code is not always the best idea. The test where I copied the check into (testLoginWithEnvVariable) is testing the 'environment-variable' branch of lib/SAMLController.php::login()
This execution-path does not pass through lib/SAMLController.php::assertionConsumerService(), which is where the login event gets dispatched, as it's the step in the SAML login process. (it either redirects to the original call URL, the home page, or an error page in case of a failed login).

The easiest solution to make the test pass, would be to chuck another event dispatch into the login() function here which would be also an awesome place to add a "UserFirstTimeLoggedInEvent", but for now I added only the normal UserLoggedInEvent.

[blizzz]

The easiest solution to make the test pass, would be to chuck another event dispatch into the login() function here which would be also an awesome place to add a "UserFirstTimeLoggedInEvent", but for now I added only the normal UserLoggedInEvent.

The first UserFirstTimeLoggedInEvent fits there, while the regular one should be outside of the condition of course, but i agree!

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)