scholrly / neo4django

Drop-in Neo4j/Django integration.
GNU General Public License v3.0
357 stars 83 forks source link

Migrate to the new graph_auth directory #215

Closed pirhoo closed 10 years ago

mhluongo commented 10 years ago

Thanks for catching that @Pirhoo!

pirhoo commented 10 years ago

You're very welcome!

tonjo commented 10 years ago

Great job guys! A couple of questions:

  1. What's the purpose of USERNAME_FIELD? In the backend authentication is done with username in any case, am I wrong? (unless I write my own email authenticator).
  2. Is there still the "wrong type" problem? Inheriting from graph_auth.user should I keep doing the from_model trick?
tonjo commented 10 years ago

I'm answering myself on 2nd question, and I hope I'm giving useful suggestions.

Reading the docs, we could go with Django 1.4 with caveats, and with 1.5 without problems. Actually, some problems arose. I have a CustomUser(neo4django.graph_auth.models.User) . Something strange (for me) happens: the inherited Manager is not neo4django.graph_auth.models.UserManager, but django.db.models.manager.Manager.

So create_user is not present, and operations like get won't work, I have to use the graph_auth.models.User manager and do the from_model trick.

tonjo commented 10 years ago

Ok, intended behavior, from Django Docs:
Managers defined on non-abstract base classes are not inherited by child classes. If you want to reuse a manager from a non-abstract base, redeclare it explicitly on the child class.

So, hints for a best-practice? I redefine the UserManager from graph_auth?
(which is "short" ;) ).

I suggest that should be mentioned in auth.rst.

mhluongo commented 10 years ago

Re: your first point - you're absolutely right about the backend authentication- it needs to refer to USERNAME_FIELD. In fact, it might be the case that it's no longer necessary to use it in 1.5, but I'll need to do some tests to find out.

Re: the user manager- I would just add the manager to your subclass, eg

from neo4django.graph_auth.models import User, UserManager

class MyUser(User):
    objects = UserManager()
    ...

It should definitely be mentioned in the docs.

tonjo commented 10 years ago

I think USERNAME_FIELD it's still used, because if I change it with email, for example, a validation fails because it does not have unique=True. So a question arise: I cannot use that email for authentication.
I could still do 2 things:

I think I know the answer, but what if User was left more "undefined" in order to ease customization? Or maybe add an additional abstract base class with no predefined authentication fields?
In django tests there's a CustomUser example, which derive from AbstractUser, a base class with no username nor email fields.
Otherwise I will copy and customize graph_auth ;)