Remove principal + Allow updates to built-in users + Fix ccn/issues/66, ccn/issues/68 #44
Conversation
Signed-off-by: Yuva Shankar <[email protected]>
Signed-off-by: Yuva Shankar <[email protected]>
yuva29
changed the title
| @@ -171,7 +150,7 @@ func (s *dbSuite) TestUpdateLocalUser(c *C) { | |||
| c.Assert(err, IsNil) | |||
|
|
|||
| // change the username and update | |||
| user.LocalUser.Username = username + "_u" | |||
| user.Username = username + "_u" | |||
There was a problem hiding this comment.
since username is key, this should not be updatable. Only updatable fields should be disable, password, first/last name etc. So would this update silently drop the changed username?
There was a problem hiding this comment.
Good catch. Its a wrong use case. I'm changing it. Regarding the update logic, Update(username, newObj)==> deletes the record pointed by given username and adds newObj. https://github.com/contiv/ccn_proxy/pull/44/files#diff-d90b925ca5c4775e0b93a79d97cd12a7R118
the logic of dropping the username(if its getting updated) is pushed to the caller. https://github.com/contiv/ccn_proxy/pull/44/files#diff-e3696ffa263920e5e893b642474ab92cR276
As the library functions are called directly from test code(without going through helper) this new object will be updated with the new username. Let me know what you think of this.
There was a problem hiding this comment.
@rhim Changed update() to overwrite rather delete+add. PTAL.
| @@ -176,6 +176,7 @@ func adminOnly(handler func(*auth.Token, http.ResponseWriter, *http.Request)) fu | |||
| // convert token from string to Token type | |||
| token, err := auth.ParseToken(tokenStr) | |||
| if err != nil { | |||
| log.Infof("From here???") | |||
There was a problem hiding this comment.
what does this info msg mean?
There was a problem hiding this comment.
Removed it. Added to debug one of the test failure.
|
|
||
| // search the data store for old username | ||
| oldUser, err := GetLocalUser(username) | ||
| oldUser, err := GetLocalUser(user.Username) |
There was a problem hiding this comment.
see earlier comments. username should not be updatable, specifically since it might have authorizations against it.
yuva29
force-pushed
the
rm_principal
branch
2 times, most recently
from
1a06b71 to
8594315
Compare
…sers Signed-off-by: Yuva Shankar <[email protected]>
| // return values: | ||
| // error: as returned by AddLocalUser, DeleteLocalUser | ||
| func UpdateLocalUser(username string, user *types.InternalLocalUser) error { | ||
| func UpdateLocalUser(username string, user *types.LocalUser) error { |
There was a problem hiding this comment.
Whoa whoa whoa, I didn't notice this when this was first added... why are we deleting the user record and then replacing it with a completely new one instead of just doing a get + set? This is strictly an edge case, but because our datastore is not guaranteed to support transactions, we shouldn't be doing it this way.
What is the value of delete + create vs. updating in place?
There was a problem hiding this comment.
yeah. I changed to overwrite now.
| // 200 (fetch was successful) | ||
| // 404 (bad request; user not found) | ||
| // 200 (OK; fetch was successful) | ||
| // 404 (NotFound; bad request; user not found) |
There was a problem hiding this comment.
This comment is a bit misleading: "bad request" has a very specific meaning in HTTP, and this is not a bad request according to that meaning. I'd delete those two words here
| updatedUserObj.Principal.Role = role | ||
| func updateLocalUserInfo(username string, updateReq *types.LocalUser, actual *types.LocalUser) (int, []byte) { | ||
| updatedUserObj := &types.LocalUser{ | ||
| // username == actual.Username |
There was a problem hiding this comment.
What is the purpose of this comment? It doesn't seem to communicate anything different than the line below it... if it's legitimately trying to communicate something non-obvious, writing it as a normal sentence would be more clear
| Role: updatedUserObj.Principal.Role.String(), | ||
| } | ||
| updatedUserObj.Password = "" | ||
| updatedUserObj.PasswordHash = []byte{} |
There was a problem hiding this comment.
Not a requested change but more of a general comment: we should probably have a Sanitize() function for preparing objects like this for display (hiding private fields and so on) instead of having to manually edit fields. It's easier to audit that every place we send data back to the user is calling Sanitize() than to inspect the whole callstack to make sure that certain fields were cleared out at some point, and it also gives us a central location to add fields to in the future.
This PR addresses the following
Removes
Principalfrom local user management and authN workflow (everything is now string <username, group name>)Allows any update to built-in user (its treated like any other normal user except that DELETE is not allowed). Updated documentation here https://github.com/contiv/ccn_proxy/wiki/Local-User-Management-API
Fixes https://github.com/contiv/ccn/issues/66, Fixes https://github.com/contiv/ccn/issues/67
Signed-off-by: Yuva Shankar [email protected]