Admin module #676
Admin module #676
Conversation
|
@rhutchison Did you see my comment in #615? I think we should go with this implementation. It really is clean, and extendable. My reservations have been addressed. I read through your conversation with @trainerbill, and I saw the light 😄 LGTM |
|
@rhutchison Overall, I like it. My only question right now is: |
|
@codydaig I see no reason not to - just wasn't apart of the original design. I can see many areas to improve the admin functions once we get this merged. @lirantal @ilanbiala any concerns with this PR? Can probably close #615 at this point. |
|
@rhutchison Sounds Good! Tested and LGTM |
| user.firstName = req.body.firstName; | ||
| user.lastName = req.body.lastName; | ||
| user.roles = req.body.roles; | ||
|
|
There was a problem hiding this comment.
Should we be updating the displayName here as well?
Perhaps, it's a decision for the developer user of this project. I can see some developers preferring to keep this data in the hands of the app user. I'm fine with this method updating the displayName; after all, it is an admin that's doing the update. They always know best.
|
I noticed that the Admin routes are still accessible to a user without the 'admin' role. I saw the discussion in #686. I think this could be extended for this purpose. What have you come up with so far on the client-side roles/security? |
|
We obviously need to fix security issues if exists here. I saw #686 but that's client side which is "tolerable" unlike if it's an actual security problem on the backend where roles/functions can be tempered with. @rhutchison thanks a lot for the work here, please ping when we have all issues addressed and let's merge it in. |
|
@lirantal I should have clarified. The client routes are still accessible, when manually typed into the browser address bar. The API routes return a proper 403 Forbidden response. We seem to be good on the backend security with this. |
|
@lirantal the back-end is secure, the front-end routes are not secure. This has been mentioned even prior to the introduction of an admin module - with articles. #686 does not completely address the problem. What we need is better client-side (role-based) security implementation. This needs to be addressed in a separate PR. If you merge #686 ahead of this commit, I will update this PR to follow that approach. |
|
Haha. I knew this was going to come up. The backend is secure as no REST calls can be made. All a user can do is get to the page, but not do anything. I had the same problem with angular-fullstack-generator in the past and solved it with this commit: trainerbill/generator-angular-fullstack@7d192a3 Basically it allowed you to put an authorize: 'admin' on the state which got check on statechangestart in ui.router. I am guessing we could take the same approach and was actually going to look into it. |
rhutchison
force-pushed
the
admin-feature
branch
from
b7523e0 to
c63de81
Compare
update displayName implements meanjs#700 (client-side role security) on angular routes.
rhutchison
force-pushed
the
admin-feature
branch
from
c63de81 to
6066020
Compare
|
LGTM. Very good work done here. Excited to merge this into my project. No more need to manually handle this. |
|
Great work guys. |
|
@rhutchison I notice both this PR and merged #686 both define the client routes with the 'admin' role. @lirantal Is this what you're referring to? This could cause conflicts when merging this in. |
|
@mleanos I was referring in general to the fact that we should just make sure that merging this in won't cause any troubles since #686 is already merged. With regards to the routes there specifically, both this PR and #686 are making the same change so I don't see any "code" problem or conflict to merge. |
|
Got it. Merging. |
|
Hi guys, I am taking 0.4.0 for a ride and I think something is missing but maybe is on the todo list: shouldn't the app be seeded with an admin user now that the module exists? (I am just asking this since it is common practice in other frameworks) |
|
@rmlopes That's a great point. Would you mind Opening up an issue about this? Since this is closed, discussion is bound to get missed. |
|
it's a good point although we don't want meanjs to become a cms/platform but rather keep it lean and agile for anyone to kickstart a project. I think I remember the meanjs generator had the option to create user/roles, no? (I might be confusing it with meanio) |
|
@lirantal How about adding a line in the getting started position of the documentation with the mongo command to add the admin role to a user? |
|
I'm open to suggestions. Let's continue this in a new User Roles discussion though, would you care to open the issue and let's move it there? |
Supersedes #615