feat(users) Separate admin routes and cleanup

[Wuntenn]

• removed unused users.authorization.server.controller
• moved admin routes to admin path

[mleanos]

@Wuntenn The idea of adding admin to these types of routes was discussed at some length here:
#713 (comment)

The discussion picks back up again (after the PR was merged): #713 (comment)

I'll spend some time reviewing that thread again, and come back with my thoughts. However, I'm leaning toward not wanting /api/admin/*** type routes for the api. Can you offer up some reasoning, or ideas on why it would be better to use that pattern?

[Wuntenn]

I want to add a singleton resource to users which works via a route like: /api/users/myresource, however I had issues with myresource being interpreted as in incorrect mongoose Id type.

Now Looking at that thread, maybe it's an ordering issue, however I doubt I would have discovered this because I followed the convention in place and gave myresource it's own route file.

After digging through the routes I noticed (again) that the userById was redundant and that only admin used anything like it.

The req.user has all the details of the user already so I couldn't see the need for routes like:

api/users/:userId/myresource;and
api/users/:userId/myresource/:myresourceId

As we don't really need to look up the user. It a waste of a param. Only the admin will ever look up a user.

I liked the idea of being able to user the ACL* with all user user routes and the idea of keeping the apis separate as a security precaution. Admin seemed harmless and like the direction being taken on the front-end.

I'm flexible though, maybe I'll have to go with the above route style...

• I ran into an issue after using the ACL to protect the resource, where the admin route was checked first and denied access for my user.

I'll check this again. Maybe it was something I did.

[simison]

As we don't really need to look up the user. It a waste of a param. Only the admin will ever look up a user.

This boilerplate doesn't, but an app using this boilerplate might. ;-)

Sorry but adding/admin doesn't seem like a typical RESTful pattern to me... having both admin routes and ACL rules is kinda doing role management twice.

This also restricts to one role only ("admin"). Some app might need to have "moderators" or other admin-like roles.

While security precaution is a valid point, I recon it's a matter of having proper testing, really.

So I'd vote not merging this.

[simison]

Oh and totally sorry if I'm repeating points already discussed elsewhere, no time to even skim through that discussion. ;-) Cheers!

[mleanos]

@Wuntenn I can't recall the depth of the discussion, in that thread I referenced, so I may be repeating as well :)

I think the crux of the issue here is the non-standard User routes. They aren't using the :userId parameter, but IMO they should be. Furthermore, we should be enforcing ACL on all the User routes.

It may seem redundant to include :userId in these routes when more often than not, the requesting User is the same as the entity requested. However, we find ourselves in this situation where we're having to hack through these limitations. The more I use MEANJS for starter/prototype projects, the more I'm realizing the need to standardize these routes; I'm finding myself working around this issue far too often.

Daron, would you be interested in submitting a new PR to "fix" these non-standard User routes?

See #1608 (comment), for more or less my current view on the matter.