Add extension for unsafe autologin #364
Conversation
| <?php | ||
| declare(strict_types=1); | ||
|
|
||
| class FreshExtension_auth_Controller extends FreshRSS_auth_Controller { |
There was a problem hiding this comment.
I am worried about the maintenance of this approach based on copying and overriding.
Would you be able to find another approach based on an extension hook (a new one)?
This way, it would both made this part more realistic to maintain and maybe also enable other custom login types
There was a problem hiding this comment.
maybe also enable other custom login types
This isn't really a login type, but an extension of form login.
I'm aware of this TODO:
Maybe some sort of hook should be put here?
There was a problem hiding this comment.
This isn't really a login type, but an extension of form login
Indeed. But maybe what about making it a new extension-defined login type?
Edit: and which could fall back to the native logins (e.g. Web form) if it does not have the required parameters to log in
There was a problem hiding this comment.
Maybe some sort of hook should be put here?
By the way I meant somewhere above not specifically in the POST if condition :p
Indeed. But maybe what about making it a new extension-defined login type?
Edit: and which could forward to the Web form login if it does not have the required parameters to log in
That would be a little more complicated than the current implementation.
I have considered this though
There was a problem hiding this comment.
Maybe be a hook around:
https://github.com/FreshRSS/FreshRSS/blob/128c375fc9de5119b1963d92dc9cd3e423053111/app/Models/Auth.php#L57
either inside or outside the form case
There was a problem hiding this comment.
I looked into it and I don't think a hook is a viable solution, at least not if we want to keep full compatibility.
accessControl() gets called only if giveAccess() fails, which is not desirable in this case since unsafe autologin could be used for account switching, and that is not possible with this hook if the user is already logged in.
I don't think it makes sense to put a hook in loginAction() either, since Minz already has an option for extending the controller.
If the auth controller ever gets removed then it would be enough to just change the extended class from FreshRSS_auth_Controller to FreshRSS_ActionController and the extension would keep working.
There was a problem hiding this comment.
If we wanted to implement any other authentication method (one that doesn't extend form auth), then a hook in accessControl() would be fine though.
edit: it would be best to avoid placing a hook in giveAccess() too, aside from the default case
2 participants
See: FreshRSS/FreshRSS#7958