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

User Info in separate table or removed #53

Closed
ghost opened this issue Oct 26, 2014 · 6 comments
Closed

User Info in separate table or removed #53

ghost opened this issue Oct 26, 2014 · 6 comments

Comments

@ghost
Copy link

ghost commented Oct 26, 2014

Could these attributes be removed or added to its own table e.g. Profile

  ## User Info
  t.string :name
  t.string :nickname
  t.string :image

When the developer can decide on which User Info s/he want to have in the app. Most apps would likely have for User Info stored specific the app and by removing these attributes from User table, we can keep the User table clean.

@lynndylanhurley
Copy link
Owner

@UrbanViking - you're free to override this functionality. Here is the method that assigns these attributes during OAuth login/signup. You can extend the default controllers using these instructions.

Different applications will always have different needs. The current implementation covers 90% of the use cases that I've encountered. Adding a Profile association would increase complexity for a minority of users.

But it should be easy enough to extend the omniauth_callbacks_controller to suit your application's needs. Please post an issue if anything is preventing you from doing that.

@ghost
Copy link
Author

ghost commented Oct 27, 2014

@lynndylanhurley - I agree that I can easily override this. However, I would rather be able to use Devise-Token-Auth without making too many changes. You have done a great job with Ng-Token-Auth and Devise-Token-Auth.

But mixing partial user info into the user table and asking developers to customize the controllers to add additional user info, seems wrong to me. In the same way, it seems wrong to store email address in UID attribute when you have an email attribute within same table. I know this is a workaround to get an unique key per user whether they choose Email or OAuth.

Why not simple remove user info from user table and leave it up the developer how to implement this - then we would not have unused attributes in user table and controllers - most apps would need more user info anyway.

If email isn't used, perhaps this should removed - it would make it easier to understand the single authentication either Email or OAuth.

It would also be awesome, if email confirmation was optional - so we didn't have to exclude it. If the developer wants email confirmation, s/he can include instead. Then you would also be aligned with Devise.

@ghost
Copy link
Author

ghost commented Oct 27, 2014

@lynndylanhurley - I am not suggesting to remove the user table entirely, but removing the extra attributes that aren't necessary for authentication and let the developer decide how to implement user data.

I like the idea of single authentication as default. I just don't like
having attributes in the user table that aren't been used and/or replicated.

I'm apologize if I wasn't clear in my comments.

@lynndylanhurley
Copy link
Owner

@UrbanViking - thanks for this feedback. I'm sorry that I haven't explained my position on this very well - I'll try to elaborate here.

I've been rolling this issue around in my head for some time - it actually relates to the issue of multiple provider authentication (#23).

For my own purposes, and for 90% of use-cases, I would like to keep the data model as simple as possible. I would like the User model to accomplish the following:

  1. For simplicity, it should support both email and OAuth users using the same table. This has been the case for most (>95%) of the applications that I've encountered that use authentication. If you have an application where this behavior is causing problems, please post an issue about that specific use-case and I will try to solve for it. But I don't want to interfere with the simplicity of 95% of applications for a minority use-case.
  2. It should provide basic info without the need to query an associated table. Associations can become awkward, and often result in n+1 queries when fetching lists of records. I agree that more info may be required from the user, and in those cases I think an association table (Profile, etc.) would be appropriate. But I think that the very basic attributes (name, email, image, and nickname) should be available without the need for a join or includes query for both simplicity and performance reasons.
  3. User records should provide a uniform set of attributes. Displaying lists of users should be a simple task, but if the users' info lives in polymorphic Profile associations, where each record could potentially contain a different set of attributes, then displaying a simple list of users with their names and email addresses would be much more complicated. I chose the name, email, image, and nickname attributes because they are always returned by OmniAuth, and because they are the simplest means to identify a user.

The current implementation was built to support these considerations. I think that removing the name, email, image, and nickname fields may complicate some basic operations. Please correct me if I'm mistaken.

I agree that the email field should be optional. I'll investigate what needs to be done to support its omission.

@lynndylanhurley
Copy link
Owner

Ahh sorry - I accidentally hit the submit button before that last comment was finished, and I just saw that you replied to it. 😵

@ghost
Copy link
Author

ghost commented Oct 27, 2014

@lynndylanhurley - no worries. I understand your perspective.

  1. For simplicity, I suggest to have an attribute named EmailOrUid. This attribute and the attribute Provider both need to be mandatory and together form an unique key. This is solely used for authentication. The attribute Provider is set to "email", "facebook" etc. Currently, the attribute Provider is optional.
  2. It is very simple and zero performance to have a 1-1 relationship. It is recommended by most data modeling guide, you can find. If you are worry about performance, you should also worry about disk space and/or redundant data.
  3. I think that most apps can do with single association to user data (e.g. Profile), but the table would have more attributes than name, email, nickname and image. Since you are building an authentication gem rather than user management gem, I would recommend to omit these extra fields.

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

No branches or pull requests

1 participant