codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.37k stars 1.9k forks source link

Feature Request: Centralized loggedInUser before RC #2055

Closed MGatner closed 4 years ago

MGatner commented 5 years ago

The decision not to ship CI4 with an authentication library leaves a big gap for developers wanting to interact with "the current user", as how that is defined may vary drastically between libraries. What they should all have in common is storing a logged-in user in the session.

I proposed (https://forum.codeigniter.com/thread-73257.html) a framework-sanctioned location for this so different libraries could all depend on the same key (e.g. Myth:Auth uses $_SESSION['logged_in']). This could be set directly, with a service, or triggered by a listener for a "login" event, and likewise accessed via a service or directly from session().

For the sake of circumventing a myriad of different ways of solving this I would love to see this feature implemented prior to RC1. I am glad to code it if the team decides the policy and guiding principles.

chistel commented 5 years ago

This would be a great idea. But year it has to be flexible like laravel's auth to support multiple authentication

lonnieezell commented 5 years ago

One problem I see with your code example is that it doesn't work for stateless authentication, like JWT or other common API auth patterns.

Also - adding a user() method to the Session, while convenient, doesn't apply well with the Single Responsibility Pattern since grabbing a current user really doesn't have anything to do with the session.

Unfortunately, I don't think there's a catch-all solution here due to stateless authentication. Perhaps a better pattern would be to have all libraries accept a user instance, say, though a setUser() method. While it's not as convenient, it removes the need for the libraries to know anything about the authentication system used, which is a good thing....

MGatner commented 5 years ago

I chose to put it in Session because I believe Session will need to be involved no matter what, being the only place where variables 1) persist across app calls, and 2) are user-specific. Stateless does make it a bit weird to use Session but it would still function, right? I think the alternative (and my original idea) gets more complex: having a User class and service that looks for a local property first, then falls back to Session (conditionally).

The problem with user dependency injection for third-party libraries is it precludes zero-conf packages as it requires the developer to initialize the library explicitly. Defining the "current user" is the only main hurdle I see to enabling zero-conf packages and why I keep pushing for it (alternative to a fully-implemented authentication class, of course).

Paradinight commented 5 years ago

I'm against it. One user class is not enought. Most application need more than isLoggedIn :)

I do not see any benefit.

MGatner commented 5 years ago

Just to be clear it's more than boolean isLoggedIn, it's who is logged in. Applications will definitely still need full auth-entication/-orization but this fills a crucial gap. An example:

CI4 has amazing module support. You could build a WidgetFactory package that a developer could install with composer require widgetland/widgetfactory and would be ready to go with routes, controllers, its own database tables and helpers - a compete sub-application ready for users at https://example.com/widgets except that there is no way to link users to their widgets without knowing which authentication library the developer is using.

Currently in modules I right I handle this with a config file, which is fine, and since I'll be using Myth:Auth I can pre-seed it with the appropriate variable $_SESSION['logged_in']. But for the sake of supporting CI4's choice not to include authentication I would like to see this solution as a middle ground for developers of auth libraries and modules alike.

Paradinight commented 5 years ago

I am interested in this feature too, because I am creating a sub-application for Codeigniter 4 and this requires an authentication system. Currently I do not know how to solve it cleanly.

MGatner commented 5 years ago

Glad to hear the feedback! My belief is this will be a larger issue if it is not addressed before RC-1, because people who have been waiting for the beta to finish will start looking at available addins and the mismatch of authentication & modules will become much more apparent.

Just out of curiosity, if you're creating a whole sub-application why not bundle the auth library with it?

Paradinight commented 5 years ago

I am creating a queuedashboard for ci4. It should use the login from the application. One login page is better than two :)

MGatner commented 5 years ago

After processing feedback from here and https://github.com/codeigniter4/CodeIgniter4/pull/2058 and thinking it through more, I think a better solution is a User(s) class, very basic with set and get methods, and the let injunction on re-calling (as needed) on each page load lie with either the authentication library or the implementing developer.

I'll send over a different PR with a concept implementation, but I'm curious to hear thoughts from this thread.

MGatner commented 4 years ago

The farthest we decided to go was documentation establishing some guidelines for authentication modules: https://codeigniter4.github.io/CodeIgniter4/extending/authentication.html