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

Make login hooks public #22743

Closed
MorrisJobke opened this issue Feb 29, 2016 · 7 comments
Closed

Make login hooks public #22743

MorrisJobke opened this issue Feb 29, 2016 · 7 comments

Comments

@MorrisJobke
Copy link
Contributor

MorrisJobke commented Feb 29, 2016

This is about a 8.1 instance - maybe different for 8.2 or master, but needs to be checked:

  • listen to post_login hook

My answer:

I just tested the hooks. Following hooks work here for WebDAV login on a stable8.1 instance:

\OCP\Util::connectHook('OC_User', 'pre_login', '\OCA\App\Hooks\Handlers', 'pre_login');
\OCP\Util::connectHook('OC_User', 'post_login', '\OCA\App\Hooks\Handlers', 'post_login');

But the essential part is following in the info.xml:

<types>
 <logging/>
</types>

This will register the app early in the request cycle and allows the handling of login. Otherwise apps are loaded after the login happened.

From #18910 (comment):

I've been using the newer $userManager->listen style with a callable as in https://doc.owncloud.com/server/8.1/developer_manual/app/hooks.html.

I can't find the docs referring to using the Util::connectHook method. From what I can see in the code they don't overlap at all, one is not a wrapper for the other. So it looks like I need to provide a function name to connectHook as a string rather than a variable holding a closure style function. Is that correct?

I'm guessing this is what Morris was referring to when he said this is fixed in 8.2. If the newer style works fine in 8.2 I'll wait until then rather than implementing the older style.

I'd not specified any type in my info.xml so that would have been part of my problem too. Even with that set to logging or prelogin it doesn't work with the listen style of connecting to hooks.

cc @gig13 and @scolebrook

@MorrisJobke
Copy link
Contributor Author

The OCP\Util methods are in the public namespace, and the only way to connect to hooks afaik.

@icewind1991 @PVince81 Are those hooks somehow connected to the events that are emitted via the emitterTrait?

To be more precise - it's that event: https://github.com/owncloud/core/blob/stable8.1/lib/private/user/session.php#L223

@PVince81
Copy link
Contributor

PVince81 commented Mar 1, 2016

@MorrisJobke not all private hooks are forwarded to the public API part (or weren't). They need to be explicitly forwarded.
For example in the case of users, it seems that we actually trigger public hooks and then forward them to private ones: https://github.com/owncloud/core/blob/v8.2.2/lib/private/server.php#L160

In any case, there is no implicit/automatic forwarding in place.

From what I see on master, only the private hook is triggered: https://github.com/owncloud/core/blob/v8.2.2/lib/private/user.php#L291

@PVince81
Copy link
Contributor

PVince81 commented Mar 1, 2016

@MorrisJobke if this is only about login hooks, then the ticket title should be "make login hooks public".
If it's about more than that, we'll need a list of hooks to be made public.

Tagging as enhancement.

@PVince81 PVince81 added this to the backlog milestone Mar 1, 2016
@PVince81
Copy link
Contributor

PVince81 commented Mar 1, 2016

CC @LukasReschke for security implications of having login hooks made public (one can intercept passwords, but actually they could also use the private ones...)

@MorrisJobke MorrisJobke changed the title Revisiting hooks Make login hooks public Mar 2, 2016
@MorrisJobke
Copy link
Contributor Author

For example in the case of users, it seems that we actually trigger public hooks and then forward them to private ones: https://github.com/owncloud/core/blob/v8.2.2/lib/private/server.php#L160

But we need this vice versa too, or am I wrong?

@PVince81
Copy link
Contributor

PVince81 commented Mar 2, 2016

But we need this vice versa too, or am I wrong?

Ideally we'd want to get rid of the private hooks altogether. So the goal would be to have the login code trigger the new-style public hook. Then have a "legacy adapter" that will listen to the new-style hook and also trigger the old-style hook again.

@PVince81
Copy link
Contributor

and make the login hooks cancellable: #13814

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

No branches or pull requests

4 participants