From dc234e5f24e723666d4534961ccdbf320412508a Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Wed, 18 May 2016 21:01:45 -0400 Subject: [PATCH] Address review feedback --- builtin/credential/appgroup/path_app.go | 24 ++++++------------- builtin/credential/appgroup/path_app_test.go | 2 +- builtin/credential/appgroup/path_generic.go | 20 ++++------------ .../credential/appgroup/path_generic_test.go | 2 +- builtin/credential/appgroup/path_group.go | 20 ++++------------ .../credential/appgroup/path_group_test.go | 2 +- .../credential/appgroup/path_tidy_user_id.go | 10 ++++---- builtin/credential/appgroup/user_id.go | 5 +++- 8 files changed, 30 insertions(+), 55 deletions(-) diff --git a/builtin/credential/appgroup/path_app.go b/builtin/credential/appgroup/path_app.go index db40eb8f3063a..0eacec3824ec7 100644 --- a/builtin/credential/appgroup/path_app.go +++ b/builtin/credential/appgroup/path_app.go @@ -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, @@ -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]), }, } } @@ -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 } @@ -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 } diff --git a/builtin/credential/appgroup/path_app_test.go b/builtin/credential/appgroup/path_app_test.go index 520a772c2fce3..5b518e7136c93 100644 --- a/builtin/credential/appgroup/path_app_test.go +++ b/builtin/credential/appgroup/path_app_test.go @@ -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") } } diff --git a/builtin/credential/appgroup/path_generic.go b/builtin/credential/appgroup/path_generic.go index f3cabd8f82554..1d54156a35bc3 100644 --- a/builtin/credential/appgroup/path_generic.go +++ b/builtin/credential/appgroup/path_generic.go @@ -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{ @@ -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)), @@ -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 } @@ -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 } diff --git a/builtin/credential/appgroup/path_generic_test.go b/builtin/credential/appgroup/path_generic_test.go index 18899ea6c3550..a6fb1c6ba61d7 100644 --- a/builtin/credential/appgroup/path_generic_test.go +++ b/builtin/credential/appgroup/path_generic_test.go @@ -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") } } diff --git a/builtin/credential/appgroup/path_group.go b/builtin/credential/appgroup/path_group.go index 6c31ddfa199c4..3455921a6a36a 100644 --- a/builtin/credential/appgroup/path_group.go +++ b/builtin/credential/appgroup/path_group.go @@ -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, @@ -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 } @@ -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 } diff --git a/builtin/credential/appgroup/path_group_test.go b/builtin/credential/appgroup/path_group_test.go index 7dfc88ed52516..6386e8b7ce38b 100644 --- a/builtin/credential/appgroup/path_group_test.go +++ b/builtin/credential/appgroup/path_group_test.go @@ -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") } } diff --git a/builtin/credential/appgroup/path_tidy_user_id.go b/builtin/credential/appgroup/path_tidy_user_id.go index 0e5cbce684737..ba862bcbad7ff 100644 --- a/builtin/credential/appgroup/path_tidy_user_id.go +++ b/builtin/credential/appgroup/path_tidy_user_id.go @@ -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" ) @@ -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 { @@ -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 diff --git a/builtin/credential/appgroup/user_id.go b/builtin/credential/appgroup/user_id.go index 20a5b9fb1d363..5781aeeb3b637 100644 --- a/builtin/credential/appgroup/user_id.go +++ b/builtin/credential/appgroup/user_id.go @@ -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 { @@ -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() @@ -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()