security icon indicating copy to clipboard operation
security copied to clipboard

Create interface for User

Open foxycode opened this issue 7 years ago • 17 comments

  • bug report? no
  • feature request? yes
  • version: 3.0

Few times I wanted to use own User implementation, bt there is no simple way how to force Nette to use it. I'd like to have IUser same way as IUserStorage to be able to use my own implementation in framework.

foxycode avatar Jan 18 '17 19:01 foxycode

User is registered in DIC as a service with the name therefore it should be possible to replace it with your own implementation similarly like in this thread.

mrtnzlml avatar Jan 18 '17 19:01 mrtnzlml

Problem is, there is User class used directly in Nette code. If not in code, definitelly in comments which isn't good for IDE.

foxycode avatar Jan 18 '17 20:01 foxycode

My tip: don't use the User class at all, operate directly on IUserStorage. It has almost the same API.

JanTvrdik avatar Jan 18 '17 20:01 JanTvrdik

@JanTvrdik Thanks for the tip, but this doesn't solve issue in framework. User is somehow hardcoded and it would be great if it wasn't. I'm happy to make PR (or at least try it), but I want to know others opinions first.

foxycode avatar Jan 18 '17 20:01 foxycode

@foxycode Actually the User class is problematic in quite a few use-cases so in advanced systems it's better to not use it at all. Rather than a PR it's better to just make your own library like I did.

enumag avatar Jan 18 '17 20:01 enumag

@enumag Again, why not solve it directly in framework? Yes, I can make my own library. I actually already did.

I believe, if we put our heads together, we can solve it directly in framework. I train new programmers in Nette and it's confusing for them. I can't simply tell them to write own library :)

foxycode avatar Jan 18 '17 20:01 foxycode

@foxycode It would become a new package separate from nette/security anyway (because of BC). In which case it doesn't matter if it is in nette namespace or not. Can't speak for @dg but I don't think he wants to add something completely new into nette.

enumag avatar Jan 18 '17 21:01 enumag

@enumag I think we can afford BC breaks in 3.0 and this is only about adding interface. Possibly this wouldn't need BC at all. I'd like to know what @dg thinks too. Can you tell me which problems do you see with User?

foxycode avatar Jan 18 '17 21:01 foxycode

this is only about adding interface

Yes, but it is an interface of a class with bad name and bad design. The goal should be to remove (or somehow fix?) the class instead of making even more stuff dependent on in.

JanTvrdik avatar Jan 18 '17 21:01 JanTvrdik

@JanTvrdik Ok, I agree with it :) I'd like to try, but need to know what others don't like about User class. Can you tell your opinion?

foxycode avatar Jan 18 '17 21:01 foxycode

Few random things that are bad about the User class.

  1. The name – User is usually an entity in application. The responsibility of this class is not to represent a user but to serve as some kind of manager of active user session.
  2. It has way to many responsibilities (that's why it cannot have a good name). It is better to use directly IAuthenticator, IUserStorage and some alternative of IAuthorizator.
  3. It relies on bad interface IAuthorizator (see this RFC on forum)

JanTvrdik avatar Jan 18 '17 22:01 JanTvrdik

God, I just wanted to post the same as @foxycode.

I am upgrading a larger codebase from nette v 2.4 and it's just not possible, since we are using an extended version of User class, bud suddenly some methods are final 😖. What is even worse, Nette\Application\UI\Presenter class won't accept any other than Nette\Security\User object and the getUser method is (yet again) final.

So... how do we extend the functionality of User object?? Please, this is a framework. Why do we constantly need to hit private and final stuff? We want to extend and bend a framework. This is where Nette gets in way. I do not wish to copypaste 1400 lines of Presenter code just to be able to use my own User implementation.

In particular, in my case I have some custom logic in User::getIdentity method, why it is final, I have no idea.

dakujem avatar Mar 20 '20 19:03 dakujem

Hi, any updates on this topic? It's quite old, but still it is a issue when upgrading large codebase from v2

sommcz avatar Oct 26 '20 17:10 sommcz

I've just came accross this. I want NOT to use Nette\Security\User, but I can't because if it's baked into Presenter, UserPanel etc.

While I agree that User class has bad design, the thing is, that it's still in Nette and it will be probably for some time. So I think one of these solutions should be applied until the whole class is rethought:

  • make Nette\Application\UI\Presenter#getUser() not final – that's the easiest one. You can not replace User, but at least you can ensure that no one is going to use it in presenters. But e.g. User panel in Tracy still gets information from User class.
  • make Nette\Security\User an interface – User keeps working, but making it an interface, there is place for custom implementations. I think it could be actually done without BC breaks.

@dg Could you please write some thoughts to this old issue? At least for me, a big blocker NOT to make PRs is the fact, that I'll never know if the whole stuff wouldn't be rejected. And I don't want to/can't waste my time. If you say "well, yes, let's do it this way, would someone do it?", I believe more people would contribute.

Edit: just realized that non final getUser() solves it for presenters only, which is not enough in my opinion. So I would add myself to the call for interface. 🙂

dakur avatar May 05 '22 14:05 dakur

In case the interface is too much to ask or simply not desired, I suggest also removing the return type hint, so that the overriding methods can return anything, not descendants of the User object only.

// Presenter.php

    /**
     * @return Nette\Security\User
     */
    public function getUser() //: Nette\Security\User
    {
        if (!$this->user) {
            throw new Nette\InvalidStateException('Service User has not been set.');
        }

        return $this->user;
    }

This is a backwards-compatible <10 minute fix.

dakujem avatar May 05 '22 15:05 dakujem

Simple, stop using it at all. Only hard which is to how to remove it from DI, so nobody can wire it. I have approached it using this fake factory overriding original registration from nette/security di-extension.

services:
	security.user:
		factory: CampApp\App\Infrastructure\NoNetteUserFactory::throwErrorWhenAccessed()
		autowired: false
final class NoNetteUserFactory
{
	public static function throwErrorWhenAccessed(): User
	{
		throw new UsageException('Hey! Nette\User is disabled in this project (as interface does not match use-cases needed in this project). Use <YourClass> instead.');
	}
}

jkuchar avatar May 06 '22 13:05 jkuchar

Wouldn't it then be better to completely remove it from Presenter, extension etc.? It gets in the way. Why not just enable overriding, which is simpler and backwards-compatible?

dakujem avatar Jun 15 '22 19:06 dakujem