Skip to content

Commit

Permalink
Address review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@vishalnayak
vishalnayak committed May 19, 2016
1 parent da4aa80 commit dc234e5
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 55 deletions.
24 changes: 7 additions & 17 deletions builtin/credential/appgroup/path_app.go
View file
Expand Up @@ -197,11 +197,6 @@ func appPaths(b *backend) []*framework.Path {
Type: framework.TypeString,
Description: "Name of the App.",
},
"user_id": &framework.FieldSchema{
Type: framework.TypeString,
Default: "",
Description: "NOT USER SUPPLIED AND UNDOCUMENTED.",
},
},
Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ReadOperation: b.pathAppCredsRead,
Expand All @@ -225,8 +220,8 @@ func appPaths(b *backend) []*framework.Path {
Callbacks: map[logical.Operation]framework.OperationFunc{
logical.UpdateOperation: b.pathAppCredsSpecificUpdate,
},
HelpSynopsis: strings.TrimSpace(appHelp["app-creds-specified"][0]),
HelpDescription: strings.TrimSpace(appHelp["app-creds-specified"][1]),
HelpSynopsis: strings.TrimSpace(appHelp["app-creds-specific"][0]),
HelpDescription: strings.TrimSpace(appHelp["app-creds-specific"][1]),
},
}
}
Expand Down Expand Up @@ -702,21 +697,19 @@ func (b *backend) pathAppCredsRead(req *logical.Request, data *framework.FieldDa
if err != nil {
return nil, fmt.Errorf("failed to generate UserID:%s", err)
}
data.Raw["user_id"] = userID
return b.handleAppCredsCommon(req, data, false)
return b.handleAppCredsCommon(req, data, userID)
}

func (b *backend) pathAppCredsSpecificUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
return b.handleAppCredsCommon(req, data, true)
return b.handleAppCredsCommon(req, data, data.Get("user_id").(string))
}

func (b *backend) handleAppCredsCommon(req *logical.Request, data *framework.FieldData, specified bool) (*logical.Response, error) {
func (b *backend) handleAppCredsCommon(req *logical.Request, data *framework.FieldData, userID string) (*logical.Response, error) {
appName := data.Get("app_name").(string)
if appName == "" {
return logical.ErrorResponse("missing app_name"), nil
}

userID := data.Get("user_id").(string)
if userID == "" {
return logical.ErrorResponse("missing user_id"), nil
}
Expand All @@ -736,13 +729,10 @@ func (b *backend) handleAppCredsCommon(req *logical.Request, data *framework.Fie
return nil, fmt.Errorf("failed to store user ID: %s", err)
}

if specified {
return nil, nil
}

return &logical.Response{
Data: map[string]interface{}{
"user_id": userID,
"user_id": userID,
"selector": "app/" + appName,
},
}, nil
}
Expand Down
2 changes: 1 addition & 1 deletion builtin/credential/appgroup/path_app_test.go
View file
Expand Up @@ -70,7 +70,7 @@ func TestBackend_app_creds(t *testing.T) {
appCredsReq.Operation = logical.UpdateOperation
resp, err = b.HandleRequest(appCredsReq)
failOnError(t, resp, err)
if resp != nil {
if resp.Data["user_id"] != "abcd123" {
t.Fatalf("failed to set specific user_id to app")
}
}
Expand Down
20 changes: 5 additions & 15 deletions builtin/credential/appgroup/path_generic.go
View file
Expand Up @@ -83,11 +83,6 @@ addition to those, a set of policies can be assigned using this.
Type: framework.TypeDurationSecond,
Description: "Duration in seconds after which the issued token should not be allowed to be renewed.",
},
"user_id": &framework.FieldSchema{
Type: framework.TypeString,
Default: "",
Description: "NOT USER SUPPLIED. UNDOCUMENTED.",
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -190,15 +185,14 @@ func (b *backend) pathGenericCredsUpdate(req *logical.Request, data *framework.F
if err != nil {
return nil, fmt.Errorf("failed to generate UserID:%s", err)
}
data.Raw["user_id"] = userID
return b.handleGenericCredsCommon(req, data, false)
return b.handleGenericCredsCommon(req, data, userID)
}

func (b *backend) pathGenericCredsSpecificUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
return b.handleGenericCredsCommon(req, data, true)
return b.handleGenericCredsCommon(req, data, data.Get("user_id").(string))
}

func (b *backend) handleGenericCredsCommon(req *logical.Request, data *framework.FieldData, specified bool) (*logical.Response, error) {
func (b *backend) handleGenericCredsCommon(req *logical.Request, data *framework.FieldData, userID string) (*logical.Response, error) {
generic := &genericStorageEntry{
Groups: strutil.ParseStrings(data.Get("groups").(string)),
Apps: strutil.ParseStrings(data.Get("apps").(string)),
Expand All @@ -221,7 +215,6 @@ func (b *backend) handleGenericCredsCommon(req *logical.Request, data *framework
return logical.ErrorResponse("token_ttl should not be greater than token_max_ttl"), nil
}

userID := data.Get("user_id").(string)
if userID == "" {
return logical.ErrorResponse("missing user_id"), nil
}
Expand All @@ -240,13 +233,10 @@ func (b *backend) handleGenericCredsCommon(req *logical.Request, data *framework
return nil, fmt.Errorf("failed to store user ID: %s", err)
}

if specified {
return nil, nil
}

return &logical.Response{
Data: map[string]interface{}{
"user_id": userID,
"user_id": userID,
"selector": "generic",
},
}, nil
}
Expand Down
2 changes: 1 addition & 1 deletion builtin/credential/appgroup/path_generic_test.go
View file
Expand Up @@ -73,7 +73,7 @@ func TestBackend_generic_creds(t *testing.T) {
genericData["user_id"] = "abcd123"
resp, err = b.HandleRequest(genericCredsReq)
failOnError(t, resp, err)
if resp != nil {
if resp.Data["user_id"] != "abcd123" {
t.Fatalf("failed to set specific user_id to generic")
}
}
20 changes: 5 additions & 15 deletions builtin/credential/appgroup/path_group.go
View file
Expand Up @@ -236,11 +236,6 @@ addition to those, a set of policies can be assigned using this.
Type: framework.TypeString,
Description: "Name of the Group.",
},
"user_id": &framework.FieldSchema{
Type: framework.TypeString,
Default: "",
Description: "NOT USER SUPPLIED. UNDOCUMENTED.",
},
},
Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ReadOperation: b.pathGroupCredsRead,
Expand Down Expand Up @@ -793,21 +788,19 @@ func (b *backend) pathGroupCredsRead(req *logical.Request, data *framework.Field
if err != nil {
return nil, fmt.Errorf("failed to generate UserID:%s", err)
}
data.Raw["user_id"] = userID
return b.handleGroupCredsCommon(req, data, false)
return b.handleGroupCredsCommon(req, data, userID)
}

func (b *backend) pathGroupCredsSpecificUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
return b.handleGroupCredsCommon(req, data, true)
return b.handleGroupCredsCommon(req, data, data.Get("user_id").(string))
}

func (b *backend) handleGroupCredsCommon(req *logical.Request, data *framework.FieldData, specific bool) (*logical.Response, error) {
func (b *backend) handleGroupCredsCommon(req *logical.Request, data *framework.FieldData, userID string) (*logical.Response, error) {
groupName := data.Get("group_name").(string)
if groupName == "" {
return logical.ErrorResponse("missing group_name"), nil
}

userID := data.Get("user_id").(string)
if userID == "" {
return logical.ErrorResponse("missing user_id"), nil
}
Expand All @@ -827,13 +820,10 @@ func (b *backend) handleGroupCredsCommon(req *logical.Request, data *framework.F
return nil, fmt.Errorf("failed to store user ID: %s", err)
}

if specific {
return nil, nil
}

return &logical.Response{
Data: map[string]interface{}{
"user_id": userID,
"user_id": userID,
"selector": "group/" + groupName,
},
}, nil
}
Expand Down
2 changes: 1 addition & 1 deletion builtin/credential/appgroup/path_group_test.go
View file
Expand Up @@ -89,7 +89,7 @@ func TestBackend_group_creds(t *testing.T) {
groupCredsReq.Operation = logical.UpdateOperation
resp, err = b.HandleRequest(groupCredsReq)
failOnError(t, resp, err)
if resp != nil {
if resp.Data["user_id"] != "abcd123" {
t.Fatalf("failed to set specific user_id to group")
}
}
Expand Down
10 changes: 6 additions & 4 deletions builtin/credential/appgroup/path_tidy_user_id.go
View file
Expand Up @@ -5,6 +5,8 @@ import (
"sync/atomic"
"time"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"
)
Expand Down Expand Up @@ -36,14 +38,15 @@ func (b *backend) tidyUserID(s logical.Storage) error {
return err
}

var result error
for _, userID := range userIDs {
userIDEntry, err := s.Get("userID/" + userID)
userIDEntry, err := s.Get("userid/" + userID)
if err != nil {
return fmt.Errorf("error fetching user ID %s: %s", userID, err)
}

if userIDEntry == nil {
return fmt.Errorf("entry for user ID %s is nil", userID)
result = multierror.Append(result, errwrap.Wrapf("[ERR] {{err}}", fmt.Errorf("entry for user ID %s is nil", userID)))
}

if userIDEntry.Value == nil || len(userIDEntry.Value) == 0 {
Expand All @@ -61,8 +64,7 @@ func (b *backend) tidyUserID(s logical.Storage) error {
}
}
}

return nil
return result
}

// pathTidyUserIDUpdate is used to delete the expired UserID entries
Expand Down
5 changes: 4 additions & 1 deletion builtin/credential/appgroup/user_id.go
View file
Expand Up @@ -209,12 +209,15 @@ func (b *backend) userIDEntryValid(s logical.Storage, selectorType, selectorValu
b.userIDLocksMapLock.RLock()
lock := b.userIDLocksMap[userID]
if lock == nil {
defer b.userIDLocksMapLock.RUnlock()
return false, nil
}

entryIndex := fmt.Sprintf("userid/%s/%s/%s", selectorType, selectorValue, b.salt.SaltID(strings.ToLower(userID)))

lock.RLock()
// It is safe to release the lock on the map of locks after acquiring the user ID lock
b.userIDLocksMapLock.RUnlock()

result := userIDStorageEntry{}
if entry, err := s.Get(entryIndex); err != nil {
Expand All @@ -240,7 +243,6 @@ func (b *backend) userIDEntryValid(s logical.Storage, selectorType, selectorValu
// in the storage. Switch the lock from a `read` to a `write` and update
// the storage entry.
lock.RUnlock()
b.userIDLocksMapLock.RUnlock()

b.userIDLocksMapLock.Lock()
defer b.userIDLocksMapLock.Unlock()
Expand Down Expand Up @@ -299,6 +301,7 @@ func (b *backend) userIDEntryValid(s logical.Storage, selectorType, selectorValu
func (b *backend) registerUserIDEntry(s logical.Storage, selectorType, selectorValue, userID string, userIDEntry *userIDStorageEntry) error {
b.userIDLocksMapLock.RLock()
if b.userIDLocksMap[userID] != nil {
b.userIDLocksMapLock.RUnlock()
return fmt.Errorf("user ID is already registered")
}
b.userIDLocksMapLock.RUnlock()
Expand Down

0 comments on commit dc234e5

Please sign in to comment.