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

dispatch BeforeUserLoggedInEvent #36883

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Feb 27, 2023

legacy code ...

This fix an issue with gss+saml (regreation post migration to IEventDispatcher as login event is not detected anymore).
The idea is to copy the normal process available in https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L615-L621 and implement it on login process on SSO

@ArtificialOwl ArtificialOwl force-pushed the fix/noid/gs-saml-pre-login-event branch from ef0e48a to e1a65ca Compare February 27, 2023 22:47
@ArtificialOwl ArtificialOwl requested a review from blizzz February 28, 2023 01:05
@ArtificialOwl
Copy link
Member Author

/backport to stable25

@come-nc
Copy link
Contributor

come-nc commented Feb 28, 2023

Why is the IApacheBackend needed?
Is it really specific to Apache web server?
Can you link to the code using that?

@blizzz
Copy link
Member

blizzz commented Feb 28, 2023

It used to be that legacy hooks were listened to in the server, and modern style ones fired in those cases. Here it would be a listener against `OC_Hook::emit("OC_User", "pre_login", ["run" => &$run, "uid" => $uid, 'backend' => $backend]);'. Is it not the case (anymore)?

@ArtificialOwl
Copy link
Member Author

example of IApacheBackend: https://github.com/nextcloud/user_saml/blob/master/lib/UserBackend.php
No idea if there is still in use elsewhere.

Currently, the modern style hook were not triggered during the sso auth because it was missing.
The globalsiteselector from master needs to interrupt the process of sso and generate session on the slave, not on the master. Because of the missing event, the session was created on master

@come-nc
Copy link
Contributor

come-nc commented Feb 28, 2023

It used to be that legacy hooks were listened to in the server, and modern style ones fired in those cases. Here it would be a listener against `OC_Hook::emit("OC_User", "pre_login", ["run" => &$run, "uid" => $uid, 'backend' => $backend]);'. Is it not the case (anymore)?

It’s weird actually there is https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L615-L621

But it’s the wrong way around, it listens to \OC\User::preLogin and emits both OC_User::pre_login and BeforeUserLoggedInEvent.
And it does not have the backend parameter.

Not sure what the clean fix is here, events are a mess.

@ArtificialOwl
Copy link
Member Author

ArtificialOwl commented Feb 28, 2023

It’s weird actually there is https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L615-L621

Yes, my first thought was to replace the current OC_Hook::emit("OC_User", "pre_login", [... to use the code you linked, but I think it was safer to add the call to IEventDispatcher directly in the code as it was already available later in the function.

This event will be catched by nextcloud/globalsiteselector#89

@ArtificialOwl ArtificialOwl force-pushed the fix/noid/gs-saml-pre-login-event branch from f870326 to 980e8e2 Compare March 1, 2023 16:33
@ArtificialOwl
Copy link
Member Author

/backport to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants