user_saml icon indicating copy to clipboard operation
user_saml copied to clipboard

Add some user related events

Open dermalikmann opened this issue 1 year ago • 12 comments

This issue initially popped up when looking into https://github.com/stjosh/auto_groups/issues/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.

dermalikmann avatar May 13 '24 12:05 dermalikmann

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!

github-actions[bot] avatar May 28 '24 02:05 github-actions[bot]

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?

elainabialkowski avatar Sep 13 '24 16:09 elainabialkowski

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

blizzz avatar Oct 02 '24 13:10 blizzz

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

blizzz avatar Oct 02 '24 13:10 blizzz

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 avatar Oct 02 '24 13:10 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).

dermalikmann avatar Oct 02 '24 14:10 dermalikmann

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 avatar Oct 04 '24 18:10 blizzz

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

blizzz avatar Oct 04 '24 18:10 blizzz

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.

dermalikmann avatar Oct 06 '24 11:10 dermalikmann

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!

blizzz avatar Oct 08 '24 09:10 blizzz

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.)

github-actions[bot] avatar Oct 09 '24 02:10 github-actions[bot]

@dermalikmann any updates on this?

blizzz avatar May 02 '25 08:05 blizzz