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

Add non-privileged user role #405

Merged
merged 7 commits into from Feb 27, 2018

Conversation

@strangeman
Copy link
Collaborator

@strangeman strangeman commented Jul 26, 2017

Fix for #318 and #198

I added admin property for User. By default, all users after migration will be Admins (for backward compatibility).

Non-Admin Users can:

  • Change his own information (login, email, name, password, alert flag), except Admin flag
  • View all users list

Non-Admin Users can't (will get 401 Unauthorized when trying):

  • Add a new user
  • Edit another user
  • Delete another user

I hide 'add user' button and user editing dialog for Non-Admin User. The first user, created by setup, have admin privileges. Also, I improved User List page, by adding all flags (Admin, Alert, External) to id.

Copy link
Member

@twhiston twhiston left a comment

great feature. Please see the very minor change requests

@@ -28,6 +28,13 @@ func addUser(w http.ResponseWriter, r *http.Request) {
return
}

editor := context.Get(r, "user").(*db.User)
if editor.Admin != true {
log.Warn(editor.Username + " doesn't permitted for user creating")

This comment has been minimized.

@twhiston

twhiston Feb 16, 2018
Member

should be log.Warn(editor.Username + " doesn't permit user creation")
Please correct this for all other instances/variations of this text as well

This comment has been minimized.

@strangeman

strangeman Feb 16, 2018
Author Collaborator

Done

@@ -143,7 +143,7 @@ func doSetup() int {
user.Password = readNewline(" > Password: ", stdin)
pwdHash, _ := bcrypt.GenerateFromPassword([]byte(user.Password), 11)

if _, err := db.Mysql.Exec("insert into user set name=?, username=?, email=?, password=?, created=UTC_TIMESTAMP()", user.Name, user.Username, user.Email, pwdHash); err != nil {
if _, err := db.Mysql.Exec("insert into user set name=?, username=?, email=?, password=?, admin=1, created=UTC_TIMESTAMP()", user.Name, user.Username, user.Email, pwdHash); err != nil {

This comment has been minimized.

@twhiston

twhiston Feb 16, 2018
Member

do we have to have admin as default? I would guess most people would prefer admin not to be the default user mode (especially in enterprise usage)

This comment has been minimized.

@strangeman

strangeman Feb 16, 2018
Author Collaborator

I think, the first system user always should be an admin. That code executes only for initial setup of Semaphore.

@@ -71,5 +71,6 @@ func init() {
{Major: 2, Minor: 3, Patch: 1},
{Major: 2, Minor: 3, Patch: 2},
{Major: 2, Minor: 4},
{Major: 2, Minor: 4, Patch: 2},

This comment has been minimized.

@twhiston

twhiston Feb 16, 2018
Member

Please can you update this to 2.5.0

This comment has been minimized.

@strangeman

strangeman Feb 16, 2018
Author Collaborator

Done

@twhiston twhiston added this to In Progress / In Review in 2.5.0 Feb 16, 2018
@twhiston
Copy link
Member

@twhiston twhiston commented Feb 16, 2018

Would it be possible to also take care of #158 in this pull request and add some extra authentication against log deletion for non admin users (if it's totally out of scope no worries)

strangeman added 2 commits Feb 16, 2018
@strangeman
Copy link
Collaborator Author

@strangeman strangeman commented Feb 16, 2018

@twhiston all done :)

@@ -53,23 +60,44 @@ func getUserMiddleware(w http.ResponseWriter, r *http.Request) {
panic(err)
}

editor := context.Get(r, "user").(*db.User)
if editor.Admin != true && editor.ID != user.ID {
log.Warn(editor.Username + " doesn't permitted for user editing")

This comment has been minimized.

@twhiston

twhiston Feb 18, 2018
Member

should be editor.Username + " is not permitted to edit users" here and in other locations.
Sorry for the pedantry about english sentence construction!

This comment has been minimized.

@strangeman

strangeman Feb 18, 2018
Author Collaborator

Done. That's alright, UI should have clear sentences, without any mess.

@@ -15,6 +15,10 @@
.form-group
label.control-label.col-sm-4 Password
.col-sm-8: input.form-control(type="password" placeholder="Enter new password" ng-readonly="user.external == true" ng-model="user.password")
.form-group(ng-if="!is_self")
.col-sm-8.col-sm-offset-4: .checkbox: label
input(type="checkbox" title="User have admin privileges" ng-model="user.admin")

This comment has been minimized.

@twhiston

twhiston Feb 18, 2018
Member

"User has admin privileges"

This comment has been minimized.

@strangeman

strangeman Feb 18, 2018
Author Collaborator

Done

editor := context.Get(r, "user").(*db.User)

if editor.Admin != true {
log.Warn(editor.Username + " doesn't permit task log deletion")

This comment has been minimized.

@twhiston

twhiston Feb 18, 2018
Member

looks like this file is missing a log import making the tests fail currently

This comment has been minimized.

@strangeman

strangeman Feb 18, 2018
Author Collaborator

Oops, my mistake. Fixed for now.

@twhiston
Copy link
Member

@twhiston twhiston commented Feb 18, 2018

looks really good. Another couple of small things and fixing a missing import and this is good to go. Thanks very much for your work

strangeman added 2 commits Feb 18, 2018
Copy link
Member

@twhiston twhiston left a comment

I feel like a real tyrant to keep asking you for these tiny changes to sentences, but it would be great if we can make it correct and consistent everywhere. Then I promise this PR is done!


var user db.User
if err := mulekick.Bind(w, r, &user); err != nil {
return
}

if editor.Admin != true && editor.ID != oldUser.ID {
log.Warn(editor.Username + " doesn't permitted for user editing")

This comment has been minimized.

@twhiston

twhiston Feb 19, 2018
Member

editor.Username + " is not permitted to edit users"

var pwd struct {
Pwd string `json:"password"`
}

if editor.Admin != true && editor.ID != user.ID {
log.Warn(editor.Username + " doesn't permitted for user editing")

This comment has been minimized.

@twhiston

twhiston Feb 19, 2018
Member

editor.Username + " is not permitted to edit users"

editor := context.Get(r, "user").(*db.User)

if editor.Admin != true && editor.ID != user.ID {
log.Warn(editor.Username + " doesn't permitted for user deletion")

This comment has been minimized.

@twhiston

twhiston Feb 19, 2018
Member

editor.Username + " is not permitted to delete users"

@@ -28,6 +28,13 @@ func addUser(w http.ResponseWriter, r *http.Request) {
return
}

editor := context.Get(r, "user").(*db.User)
if editor.Admin != true {
log.Warn(editor.Username + " doesn't permit user creation")

This comment has been minimized.

@twhiston

twhiston Feb 19, 2018
Member

editor.Username + " is not permitted to create users"

editor := context.Get(r, "user").(*db.User)

if editor.Admin != true {
log.Warn(editor.Username + " doesn't permit task log deletion")

This comment has been minimized.

@twhiston

twhiston Feb 19, 2018
Member

editor.Username + " is not permitted to delete task logs"

@strangeman
Copy link
Collaborator Author

@strangeman strangeman commented Feb 20, 2018

All done. I need to take some lessons with an English teacher, instead of watching Netflix shows. :)

@twhiston twhiston moved this from In Progress / In Review to Done in 2.5.0 Feb 21, 2018
@twhiston twhiston merged commit 0fceedb into ansible-semaphore:develop Feb 27, 2018
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
2.5.0 automation moved this from Done to Merged Feb 27, 2018
@twhiston twhiston removed the needs work label Feb 27, 2018
@vaol
Copy link

@vaol vaol commented Sep 12, 2018

I have just installed Semaphore v2.5.1 and I am still facing the same problem as reported in #198. I would need some user to only have the permission to run some templates, but not update or delete them.
Is it something possible to achieve ?

@strangeman
Copy link
Collaborator Author

@strangeman strangeman commented Sep 12, 2018

@vaol please check user settings ('Admin user' checkbox):
2018-09-12-185213_1366x768_scrot

@vaol
Copy link

@vaol vaol commented Sep 12, 2018

Thank you for answering so quickly !
Here is my config :
image
image

And still I can delete a template when I am connected as user "demo"

@strangeman
Copy link
Collaborator Author

@strangeman strangeman commented Sep 12, 2018

Ugh, sorry, I confused different roles mechanisms. launch-only users still not implemented. I made the PR some time ago, but it was not too good: #413
If you're ok with Go, you may adapt the code from that PR

@strangeman strangeman deleted the UnitedTraders:198-nonpriv-user branch Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.5.0
  
Merged
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.