api: add gRPC endpoints for creating, updating and deleting passwords #649
Conversation
There was a problem hiding this comment.
Generally good to separate generated code from hand written code. It lets us review it easier. So the commit history should look like.
api: add gRPC endpoints for ...
api: regenerate protobuf code
As discussed, please address these comments then add a couple tests to server/api_test.go. Those can use the in memory storage found in storage/memory to test your implementation.
| if req.NewHash != nil { | ||
| actual, err := bcrypt.Cost(req.NewHash) | ||
| if err != nil { | ||
| log.Printf("api: bcrypt.Cost, error: %s", err) |
There was a problem hiding this comment.
This should return an error instead of logging it.
| log.Printf("api: bcrypt.Cost, error: %s", err) | ||
| } | ||
| if actual < bcrypt.MinCost { | ||
| log.Printf("api: hash does not meet MinCost requirement: %d", bcrypt.MinCost) |
There was a problem hiding this comment.
Same as above. Return an error instead of logging.
Also we don't want MinCost we probably want DefaultCost. Min is the absolute minimum allowed bcrypt cost, not the minimal secure cost.
| } | ||
|
|
||
| updater := func(old storage.Password) (storage.Password, error) { | ||
| new := storage.Password{ |
There was a problem hiding this comment.
new is a keyword. Generally not a good idea to assign to it.
Also don't create a new struct here, just modify old. There might be new Password fields added later that you'll zero otherwise. See example in #648.
| return &api.UpdatePasswordResp{NotFound: true}, nil | ||
| } | ||
| log.Printf("api: failed to update password: %v", err) | ||
| return nil, err |
There was a problem hiding this comment.
Probably want to use fmt.Errorf to add context as well. For example
return nil, fmt.Errorf("update password: %v", err)
| return &api.DeletePasswordResp{NotFound: true}, nil | ||
| } | ||
| log.Printf("api: failed to delete password: %v", err) | ||
| return nil, err |
There was a problem hiding this comment.
Same comment as above, use fmt.Errorf
|
|
||
| p := storage.Password{ | ||
| Email: req.Password.Email, | ||
| Hash: req.Password.Hash, |
There was a problem hiding this comment.
We should probably check the hash the same way we do in UpdatePassword. Maybe pull out that logic into a helper function and use it here too?
| if req.Password == nil { | ||
| return nil, errors.New("no password supplied") | ||
| } | ||
| if req.Password.Hash != nil { |
There was a problem hiding this comment.
They have to provide a password. So if this is nil we should return an error.
There was a problem hiding this comment.
Also the storage package doc recommends a cost value of at least 14. Where as DefaultCost = 10. Should I change the condition to check for a cost value of 14 instead?
| return nil, errors.New("no password supplied") | ||
| } | ||
| if req.Password.Hash != nil { | ||
| err := checkCost(req.Password.Hash) |
There was a problem hiding this comment.
golang tip: You can define variables as part of an if statement and make this a bit more concise:
if err := checkCost(req.Password.Hash); err != nil {
return nil, err
}
|
Changed storage package doc to have min cost value of 10 instead of 14. |
|
All the unit tests passed: |
| "github.com/coreos/dex/storage/memory" | ||
| ) | ||
|
|
||
| var s = memory.New() |
There was a problem hiding this comment.
Avoid global state. Please create this instead each test.
There was a problem hiding this comment.
Actually just combine these into one test that
- Creates a password.
- Updates the password.
- Deletes the password
| Username: "test", | ||
| UserId: "test123", | ||
| } | ||
| hash, err := bcrypt.GenerateFromPassword([]byte("test1"), 10) |
There was a problem hiding this comment.
Generate this value beforehand instead of requiring each test run to compute a bcrypt.
For example, just use this instead:
// bcrypt hash of the value "test1" with cost 10
hash := []byte("$2a$10$XVMN/Fid.Ks4CXgzo8fpR.iU1khOMsP5g9xQeXuBm1wXjRX8pjUtO")
| } | ||
| hash, err := bcrypt.GenerateFromPassword([]byte("test1"), 10) | ||
| if err != nil { | ||
| t.Errorf("Unable to generate password hash: %v", err) |
There was a problem hiding this comment.
t.Errorf doesn't actually stop the tests. You either need to return or use t.Fatalf
|
|
||
| if _, err := serv.UpdatePassword(ctx, &req); err != nil { | ||
| t.Errorf("Unable to update password: %v", err) | ||
| } |
There was a problem hiding this comment.
need to get the user after to ensure the new username was actually set.
| } | ||
| } | ||
|
|
||
| func TestDeletePassword(t *testing.T) { |
There was a problem hiding this comment.
I commented on this above, but two tests shouldn't ever be dependent on one another. If they are, it should probably just be one test :)
|
Unit test passed: |
| // Attempts to create, update and delete a test Password | ||
| func TestPassword(t *testing.T) { | ||
| var s = memory.New() | ||
| var serv = NewAPI(s) |
There was a problem hiding this comment.
nit: var serv = ... is redundant, just use serv := ... instead.
|
one last nit, other than that lgtm |
closes #639
cc @ericchiang