Admin module

[trainerbill]

I needed to make a small Admin module for a project I am working on. All I needed was to have it add/remove roles from users but it is a good start. Also I did not make any tests for the module. Sorry, I did not have the time. The module has view/list/update to users and pagination. Its a start if you want to build on it. Otherwise you can close the PR.

[ilanbiala]

@trainerbill we'll probably consider this for 0.4.x since it will add a new feature that we haven't tested much.

[rhutchison]

There is an issue with updating a user (modifies the user's password), but it is a good start. I started refactoring the routes and will push my changes this week.

[sielay]

@rhutchison Nice one. If you merge with User Refactor PR then you can drop list of users I added there, as you replace it perfectly. Would you be against, if I try later to merge it to CleanMean?

[trainerbill]

I cleaned up a jshint issue on the server controller. @rhutchison I don't know why but you are right. I am only merging user.firstName, user.lastName and user.roles before the user.save so I have no idea why the password is changing. I tried to delete user.password and user.salt before saving and it still changes. Someone with more knowledge of the stack is going to have to figure this one out.

If anyone wants anything else built for the AdminModule let me know. Willing to help out.

Thanks!

[rhutchison]

@sielay I will check out your PR today and see if there is anything to take from this and merge to your PR.

[pgrodrigues]

The problem is this hook in user model:

UserSchema.pre('save', function (next) {
    if(this.password && this.password.length > 6) {
        this.salt = crypto.randomBytes(16).toString('base64');
        this.password = this.hashPassword(this.password);
    }
    next();
});

Even if you only change the firstName, lastName and roles, once you call user.save(function ({...})) the user pre hook above will be called and will generate another salt and hash the password making it impossible to login again.

Not sure why the same doesn't happen when the user updates his profile. I'll investigate a bit more and then I will report here.

[pgrodrigues]

When a user updates his profile, the password is undefined which means that when the pre save hook middleware is called it will just pass through to the next middleware with next().

However when an admin updates the user profile here:

/**
 * Update a User
 */
exports.update = function(req, res) {
    var user = req.model;

    //For security purposes only merge these parameters
    user.firstName = req.body.firstName;
    user.lastName = req.body.lastName;
    user.roles = req.body.roles;

    user.save(function(err) { ... });

    ...
};

The password for user is in fact the hashed password instead of undefined, so when user.save() is called it will generate another salt and re-hash the already hashed password.

Using User.findById({...}, '-salt -password') fixes this I believe.

[rhutchison]

@pgrodrigues do you have the latest? resolved by Gym@6cedafb#diff-10f27f9a7495c672e5db142bf4a583abR78

[pgrodrigues]

Yeah I missed commit @6cedafb no problem ty anyway :)

[pgrodrigues]

@rhutchison I left a few comments in your commit Gym@6cedafb#diff-10f27f9a7495c672e5db142bf4a583abR78

[misugijunz]

Will this add capability to have different views for each users as well?

[mleanos]

Would it make more sense to create unique routes for the Admin module? For instance, we could define the routes here as '/api/admin/users'

If we define the same routes in different modules, this could get confusing. I understand that aren't any conflicts between the existing user routes, and these new routes. However, I think it might make more sense to use completely separate routes.

[rhutchison]

I don't think this makes more sense. What you are suggesting is that every time someone creates a new module, they should use unique api routes for that module. This can grow out of control and be even more confusing, especially if you are exposing your end-points to third parties.

[lirantal]

I understand what @mleanos is trying to say - we basically put all the admin functionalities in a central place, and when the admin capabilities will grow with time, as well as in our own personal projects it might create clutter to keep up to date or merge changes.

[mleanos]

@rhutchison I agree with you, for most cases. For the purposes of an Admin module, it seems that it may need additional consideration. Since there is very specific role requirements, in order to access these routes, it may be better to separate these routes; thus, isolating them from the rest of the api routes.

I'm not necessarily opposed to this change. Just thought I might express my considerations with these types of api design patterns.

@lirantal I was trying to convey those concerns. Thanks.

[lirantal]

@trainerbill @rhutchison the password-changing issue has been fixed already in a PR so we're on that account.

Before we can merge I need some LGTM so please comment on any changes required or otherwise confirm if you're up for it.

[rhutchison]

@lirantal There is some minor cleanup needed. I will submit a PR to @trainerbill's branch tomorrow night and then he should rebase before it's merged.

[lirantal]

@rhutchison thanks

[trainerbill]

I think it looks good to me. Quite a bit has changed since my original commits but the functionality seems to be there. The only thing I can think of is that the search currently searches the entire user model. We may want to restrict that to only a few of the fields like username, email, firstname, lastname. But that can be done at a later time. @rhutchison I am a noob when it comes to things outside of commit/add/pull/push so I will probably need some help with the rebase. We can take it to the chat when the time comes.

[lirantal]

Ok, I'll leave it for you guys @trainerbill @rhutchison to finalize this PR then. Please ping on this thread when you're done and we'll merge.

[mleanos]

@rhutchison Everything here looks really good to me. I'm still a little concerned about the route usage, that I commented about.

What would happen if there's a need to provide a list of users to a user that wasn't an admin? This is probably a one-off type of example. However, it may be hinting at problem with maintainability.

I'm not saying we should create new api routes for each new module. The Admin module seems like a very unique case. We want to enforce a very strict access policy for these routes. By completely isolating the Admin routes, from the rest of the api, we can ensure that there's no ambiguity between any of the other existing user routes. For me, I'd want to keep this an Admin module on complete lockdown.

Anyone else have feedback on this?

[rhutchison]

@mleanos are you on https://gitter.im/meanjs/mean? I would be open to discuss further.

I am hesitant to jump to any changes and think we can discuss further to come towards some reasonable solution. One thing I do not like about the current users routes is it doesn't feel very restful to me.

Weird:
GET /api/users/me
PUT /api/users

Alternative:
GET /api/account
PUT /api/account
PUT /api/account/password - not sure if separate route is needed.
DELETE /api/account/providers/:id
POST /api/account/picture

If I was to expose the same API to a third-party, i would restrict access to specific data based on their rights/roles, not end-points (you would still need to enforce the same access rules to protect the data).

Maybe instead of having a separate admin module, the admin functionality needs to ride along with the module.

angular.module('core.admin', []) - Adds the menu item in the topbar
angular.module('users.admin', []) - submenus, routes, controllers, views
angular.module('articles.admin', []) - submenus, routes, controllers, views
etc...

This might be much more maintainable from a route config and also being able to add/remove new modules easily.

POC: Gym@b7523e0

@trainerbill @lirantal @ilanbiala @simison @sielay @codydaig @pgrodrigues please weigh in

[mleanos]

@rhutchison Thanks for your response. I appreciate your willingness to have a discussion about this. I see you're pretty busy with all the movement in this repo. I thank you for that as well. It's much appreciated.

I also have had some reservations about the current users routes. Earlier today I had just about the same thought as you; except i was thinking of /api/me as an alternate route strategy. However, I think 'account' could be a bit clearer as to what it is.

I agree with your other point as well. If the same route strategy is used between the existing /api/users, and the new Admin module, it may be appropriate to put them in the same module.

I still have an inclination to completely isolate the Admin module from the rest of the application tho.

I'm looking forward to implementing the Admin feature into my app, so I've been giving it a bit of thought lately.

[trainerbill]

@rhutchison I actually really like the idea of admin going into the specific module. That way if someone builds a module that we want in the project they can extend the admin menu without modifying a core set of admin code. But this is a pretty large refactor. Could introduce the admin module functionality and get a larger community discussion going for 0.5.0 for refactoring into the modules.

[mleanos]

@rhutchison @trainerbill I think this is a pretty big change. I really like the idea, but it introduces some complexities that should be addressed. I'm wondering if makes sense to assume most dev users of this project, will want to implement the Admin module. If they don't have a need for it, then it would just add clutter to their other modules.

It's a very interesting design consideration. I definitely think it requires some discussion with larger group of this community.

[rhutchison]

@mleanos What are the complexities that concern you?

[mleanos]

@rhutchison What concerns me most is that the proposed design would couple the core (boiler plate) part of the application too closely with the new Admin module. The rest of the application will depend upon the Admin module being present. Whereas, I can see some dev user's not wanting the Admin module at all. I would rather treat the Admin module the same as Articles; which I have removed from my application with ease, and without any repercussions.

I really like a lot of the aspects of your POC; the use of the resolve in the routes, the separation of controllers, and the route strategy. However, I'd rather see all this go into an isolated module, that most application users won't be interacting with.

[rhutchison]

@mleanos as a boilerplate, I actually see this design easier to remove/create custom modules with.

I assume that anyone using users, would also want to be able to manage those users. Maybe they would like to expand further, but I see it being a good base. Where-as like you said, you removed articles with ease. Now lets say that MEAN.js community decided to take the admin module further and build functionality for articles or even chat. If we had a single admin module, then people would need to dig through admin to clean it up. If they didn't want chat or articles, they would simply remove the module and the admin feature would be gone with it.

If a developer wants to have their own user module that is so far outside of what this boilerplate offers they may also have made customization to the users module itself. At this point, I would assume that they could just as easily fork/maintain their own user module and replace their implementation with the boilerplate.

[trainerbill]

@mleanos I agree with @rhutchison. We talked at length in chat yesterday. By pushing this off to the modules themselves it actually makes it easier to remove certain modules. If we wanted a Manage Articles in the admin section that would allow an admin to remove/edit any article then we create the functionality in the articles module. So that if someone does not want the articles module then they can remove the entire folder without having to cleanup the admin module as well. It also makes it easier to introduce new modules as we are not messing with core code.

The only change to the core boilerplate is an addition of Admin to the top level menu structure and an open /admin route, that doesn't do anything by default. They only see the menu item if the user has an "admin" role which is not default. Everything else is in the users module. So I am not sure on how that introduces complexities. If you were to remove the users module then the manage users functionality would go away. If you are worried about the top level menu of Admin with no functionality then you would just comment out the route and menu. Though it would not be a big deal because with no user module there is no functionality.

[lirantal]

Ok so what's the bottom line on the status here? which PR should be merged for the admin functionality cause I've seen a couple of them being opened and closed, including this one, and I already lost track :)

[trainerbill]

@lirantal It's kind of up to the contributors on which way to go. My module started using an admin module which some people like. I prefer @rhutchison implementation #676 which pushes the admin to the specific modules so it can be extended to future modules without updating core code. We just need buy in from contributors that this approach is how we want to go. If we agree on his PR then I will close this one.

[mleanos]

@trainerbill @rhutchison @lirantal I see your points. I think they're very valid. I hadn't looked at it from the perspective of maintainability, when one might want to remove/add Admin functionality for a specific module. This approach does make it much cleaner. Furthermore, it would be easier for individual developers to work on specific Admin features, in a specific module that they may have "ownership" over. Personally, I prefer admin features to be locked down to a specific module. However, this can be negated if the proper security restrictions are in place. Which would be quite attainable with this design.

Let's roll with @rhutchison implementation. I really do like what was done with this feature. Thank you both for devoting time to this.

[lirantal]

Thanks guys for all the time invested on this, much appreciated.
@trainerbill thanks for this PR, I'll close and we'll go on with this feature support in #676 as you suggested.

[dtytomi]

@trainerbill please I would like to ask how to implement the admin signup.