communityii / yii2-user

Configurable Yii 2 user management module with social authentication and various controls
https://groups.google.com/d/forum/communityii
Other
50 stars 9 forks source link

Design Discussion Thread #1

Closed schmunk42 closed 10 years ago

schmunk42 commented 10 years ago

EDIT: kartik-v: Changed title to Design Discussion Thread, original title was Data Model

I think one thing which should be always done carefully in the beginning of an extension is the data model, so here's a very rough first draft: https://gist.github.com/schmunk42/9839772

kartik-v commented 10 years ago

Thanks. Few initial suggestions...

  1. The user table ideally should be lean with only the necessary fields - since this table data will be used and queried really often. Your example is good - we may recheck each field - and find any user details not needed for basic user auth ... can be relegated to a profile table or similar (which we need to figure out - this will have a one-to-one relation).
  2. I feel the auth_client and auth_key should lie in a detail table with foreign key to id in the user table. This way an user can be authenticated with multiple social logins if needed.

I maybe will try to work out and share a db structure for suggestions and also try getting some info from all the other extensions.

evercode1 commented 10 years ago

I ran into a disambiguation problem with the advanced template because of the field name status, when I tried to have a method getStatus that pointed to another table named status, which I was using to hold the names of the statuses. I preferred to have it all in DB instead of using constants.

So, first suggestion is using status_id as field name instead of status, and also role_id would be required if we were sticking with what comes out of the box in the advanced template, since there is a field for role. We could point role_id to a role table and status_id to a status table, which would hold the names of each. This will make the ui for user management easy to build and manage.

robregonm commented 10 years ago

@kartik-v Agreed. If a user needs more "user fields" I use to create more tables for that purpose. A couple tables: ProfileField and ProfileFieldValue, that way the admin user can create several custom fields. This is what I'm talking about

schmunk42 commented 10 years ago

@robregonm While I like the proposal and used the same way of creating profiles many times in yii-user. I'd like to raise a few points here.

Couldn't this be even a separate extensions? Because what we're doing here is a using a dynamic Form-and-Model-Builder with a WebUI, a rather complex task. If you think about creating dynamic validation rules, etc... I know it can and has be done, but I'd also like to see a very flexible solution for profile data.

Some years ago, one of our first Yii extensions was a user module where you could "plug-in" another profile model. This kept our user module pretty slim. Have a look at this 4 year old code just to get the idea.

Can we create an interface for the profile data, it could be either a dynamic model or a normal model (with i.e. partial forms).

@evercode1 Why do you have the status-(definitions) in the Db, aren't they closely linked to the module logic? Do you have a use-case for custom statuses?

About having a role in the user table I'd like to ask @samdark and @cebe why they added it. I'd have said that this should be done by an rights/auth module, but maybe this just a default role, fallback or simple implementation.

robregonm commented 10 years ago

Sounds good and reasonable. Agreed! That could make the extension even more powerful. KISS

kartik-v commented 10 years ago

@schmunk42 @samdark @cebe @robregonm @evercode1 @nineinchnick

The collaboration organization is renamed to communityii

evercode1 commented 10 years ago

@schmunk42 I realize I didn't have to do it this way, but it made building the ui for the role and status crud very easy since I could just use Gii. It gave me a way to add a role or a status in the future that I haven't anticipated yet, maybe didn't need, but was very flexible. I could use that for any type of site.

As for the structure of the user table including status and role, I found that to be very intuitive. It only took one method name change in the backend site controller to point to and one new method on LoginForm named loginAdmin:

https://gist.github.com/evercode1/9847778

So now, with hardly any coding, I was able to restrict backend users to an admin level. And with the ui I created for users and roles, I could easily update a user to admin. Again, I'm just a beginner, so this is not a complete rbac solution obviously, but it was really easy to work with.

I had read the book for Yii 1 on rbac and wanted to shoot myself. I wasn't planning on going in that direction for rbac and I know the old rbac had simply been ported over, so I was going to do something different, just didn't get that far, since I realized I was stuck on how to integrate the social logins, which was more important to me.

Yii 2 has been much easier to work with than Yii 1 for me. Other than the disambiguation problem I had, I was amazed at how quickly I was able to put it all together. My hope is that the new user module can use the user model on the existing advanced template, it is easy to use and easy to understand.

I was going to make a suggestion for the social table structure, but I think @kartik-v will get it right. It needs to support mutlitple social logins, so the table should hold a record for each social network auth id/key/whatever that the user wants to associate with their yii-user account. I figured there would be at least one additional table to hold the names of the social networks that were being referenced.

With the right data structure, we could anticipate reports that we might build. What is the average number of social networks the site's users have associated with their accounts? What are the most popular social logins? Etc.

I really don't see why the user table has to change at all since the social table will most likely have a field for user_id, tying it all together, but that is for greater minds than me to decide :)

kartik-v commented 10 years ago

Folks ... I have attempted to put in design thoughts after some quick study.

Please refer the proposed design skeleton for design discussion and planned features. Let know your thoughts.

evercode1 commented 10 years ago

Sorry if this is an obvious question, but on the adm_user_profile table, would this support multiple social networks? A lot of fields seem like they would be one-time entries, email, gender, etc. whereas the provider field is something we would need for each network? Also, email is currently in the user model, would it make more sense to work with the existing structure of advanced template? Also, if we used the role field on the advanced template, we would not need the pivot table.

Obviously I've never built a user module before so forgive me if these are obvious questions...

kartik-v commented 10 years ago

Yes that's the idea to support multiple social networks... the provider field will be one of FACEBOOK, GOOGLE etc. The id field will be unique auth_client_key and profile_id is the displayed id like social email or social username. The other fields will be autopopulated from the provider using the OAuth API. If we maintain only one instance of user name, avatar etc... these will be overwritten once you sign in with another social protocol. The user may have only certain fields (like first_name, last_name) for example updated in facebook and certain other fields in google etc... Hence, the initial thought is to maintain these fields for all protocols the user has signed in as ... and then let the user choose which one he/she wants to set as the active one by setting is_active to true.

In a practical scenario, user may not sign in with more than 2 odd protocols. But the module can be made extensible to support all and choose, which of the protocol fields will update the profile.

We can get a user's first_name, last_name by just query on is_active flag in user_profile (only one of the provider records can be made to be the active through an internal validation).

AN ALTERNATIVE METHOD CAN BE TO MAINTAIN JUST ONE INSTANCE OF FIRST_NAME, LAST_NAME etc per user... the only thing is the profile details will be not available readily as query from db in this case. The OAuth API needs to be called for picking these details each time to refresh or set active one profile.

nineinchnick commented 10 years ago

In my module I don't force any data model. I simply require an interface in the UserIdentity class to interact with it. That way the dev can create db tables in any form he wants or use existing ones and adjust the provided UserIdentity example class.

We could agree on an example data model and still don't force it on everybody.

As for the additional profile fields I did the same, I just require a few methods in the UserIdentity to pass them as an array, my ProfileForm accepts a behavior that adds attributes to it and the view can be overriden in a theme. That way you can add complex UI controls for a simple extra column in the user table.

@robregonm description looks like EAV which is considered an anti-pattern for SQL databases.

As for the social part did anyone actually looked at my data model?

kartik-v commented 10 years ago

@nineinchnick good inputs... we can think to use a simplified structure like yours... but can you elaborate a bit where would you get or store the additional fields from the provider? Something like the avatar image, email, first_name, last_name, profile_username are always needed by most use cases - which is available with most social providers.

Will these need to be fetched separately each time from the respective provider on each request?

Or should we store it in the DB - which can be fetched in one query?

nineinchnick commented 10 years ago

I think just the main profile should be updated, simulating what the user would do. Only one of those profiles would be used anyway. We can always refetch a profile from a different provider if needed, having the user consent obtained when logging in for the first time.

There are some strategies to select on how to sync the local profile with a remote one. It can be done once on first association or on every login or manually. That either requires some practical usage feedback or be left to the devs to select from.

kartik-v commented 10 years ago

There are some strategies to select on how to sync the local profile with a remote one.

Great... let's think on these lines to devise a design for extensibility. The module can provide a few common options if we feel so. Also, let's agree on the fields we will store by default for the main profile - I feel the OAuth API does specify a set of data which is derived from providers. My attempt was to list most of them... should we use that as a ref? It makes sense because - social auth integration is assumed to be integrated in this module.

nineinchnick commented 10 years ago

We should follow OAuth as far as grouping fields, because when asking for user consent to share data you need to select such groups like:

There are some extra ones specific to providers. I think we should keep this at minimum, that is only ask for profile and email by default. The dev should be able to extend the local user profile and ask for more if needed.

Again, in my module, I left the process of updating local profile using a remote profile to the UserIdentity implementation, so it can be easily extended.

Edit: let's not try to solve all problems at once. A basic user module doesn't need to store user address, it just need to identify the user, so a login, email and a display name (firstname/lastname) is sufficient.

kartik-v commented 10 years ago

Makes sense. Would you be able to share an english translation of your demo site?

nineinchnick commented 10 years ago

There already is one, there's a language picker in the menu. Also, try switching to the Bootstrap theme.

kartik-v commented 10 years ago

I feel, we may need to still keep an option in the implementation to turn on/off the local/remote profile updates through some module configuration.

nineinchnick commented 10 years ago

I agree. Also, I was supposed to finish managment of remote identities in user profile this weekend. After that I'll update my Yii 2 version.

kartik-v commented 10 years ago

Have updated the proposed design skeleton v1.1 based on the last discussion. Have a relook and update.

schmunk42 commented 10 years ago

Guys, I still have to read through everything, but I'd add the following:

kartik-v commented 10 years ago

@schmunk42 - thanks... so we can then skip that and it can be plugged in if needed by anyone into this module (we may need to check if there are issues in ensuring that on the first release). I am removing them from the design then. Here is proposed design skeleton v1.2

kartik-v commented 10 years ago

If a user needs more "user fields" I use to create more tables for that purpose. A couple tables: ProfileField and ProfileFieldValue, that way the admin user can create several custom fields.

@robregonm , @schmunk42 , @nineinchnick I think we can entirely simplify this (I have mentioned this in the design skeleton as well). We start with a basic adm_user_profile table - and allow that table to be modified and extended (with docs on how to do that). Developers can reuse the built in UserProfile Model or extend it as per their newly modified table. This will offer much better control for developers I suppose than making it the Profile & ProfileFields way like the old yii-user.

Any comments.

My idea is to give options to developers to totally customize the UI the way they want. This can be done by allowing them specify their own views and layouts in the configuration (refer this in the design feature map).

An additional option is to create a few widgets like LoginForm, RegistrationForm, UserProfile (these can be then plugged in wherever the developers want to on their pages) - thus offering control over the various pages and styling them.

Views please...

cebe commented 10 years ago

About having a role in the user table I'd like to ask @samdark and @cebe why they added it.

@schmunk42 where did we add that? What exactly is the question about?

schmunk42 commented 10 years ago

@cebe here: https://github.com/yiisoft/yii2/blob/master/apps/advanced/console/migrations/m130524_201442_init.php#L21

cebe commented 10 years ago

okay, thats not mine, @samdark might know why that is added.

evercode1 commented 10 years ago

@kartik-v thank you for clarifying. The proposed design skeleton v1.2 seems very intuitive and your idea about offering widgets for login etc would be really great. @cebe, @schmunk42, I thought adding role to the user table was part of a master plan to be brilliant :) I have no clue actually, but it did make it easy for a beginner like me to restrict access to the backend...

cebe commented 10 years ago

adding role in user table is just that you allow a user to have only one role. That might be sufficient for most use cases but sometimes you may want to assign multiple roles. Having role in user table you do not need assignment table of RBAC.

schmunk42 commented 10 years ago

I agree, that it is an intuitive way of settings permissions when you have a role column in the user table.

But I see two problems here, first: you're doing the job of an auth manager, which should be handled by RBAC and second: it is a pretty limited solution. In almost all Yii 1 extensions for user management and authentication we had to subclass the user class from a class from the extension, which was a bad side effect of mixing user management and authentication. I think in Yii 2 it will be much easier to build a clean implementation, but I haven't looked at it in detail.

I thought about the following, what's about having very basic PHP-based user identities for the root account and maybe one or two additional accounts - just to keep it easy if you don't much RBAC. But don't include roles to the user management directly. As yii2-user can deal with different identities anyway, we can read them from a PHP config file.

If you need it, I think you can either create rules for assigning roles by PHP or you just a full-featured RBAC manager.

evercode1 commented 10 years ago

@cebe, @schmunk42 thanks for clarifying guys. I'll wait for further direction from you on role and rbac.

kartik-v commented 10 years ago

Added inactive status to design.

@schmunk42 on your topic on roles -- could we have a yii2-user module configuration option as:

simpleRBAC => true

and add the role field and related rules in... only if the above is set to true... if it is false all rules from the custom rbac module would be used. It may enable the module to be used for most use cases even though we are encroaching a wee bit into RBAC.

schmunk42 commented 10 years ago

How about the following:

evercode1 commented 10 years ago

@kartik-v I think the config idea for rbac is very intuitive. If I'm understanding this correctly, it's a simple way to decide if you want to use only the user module or user with the rbac extenstion.

kartik-v commented 10 years ago

@schmunk42 sounds good. On the third part for yii\rbac\DbManager - we may allow the user to set a widget class as a parameter . By default if not set, it would use the widget class from communityii/yii2-rbac extension. We may still add the option to read simpleRBAC config if authmanager is not set - which will read a role from user table... else skip it.

schmunk42 commented 10 years ago

btw: Samdark mentioned, that the core team will create a RBAC UI.

If you really need an option, which reads the role from the user table, I'd suggest to create a yii\rbac\SimpleDbManager which does that.

But we have to make clear how things work, because you may get confused when setting the role in the user table and nothing happens, because you have a RBAC manager configured which works differently.

kartik-v commented 10 years ago

I think we will discuss this part... I am trying out a work segregation and would update in the internals repo.

samdark commented 10 years ago

I'm not sure if we'll do it soon enough though.

kartik-v commented 10 years ago

Closing this thread. We can open it up in internals repo if needed.