Address feedback

builtin/logical/rabbitmq/backend.go

@@ -21,17 +21,11 @@ func Backend() *framework.Backend {
 	b.Backend = &framework.Backend{
 		Help: strings.TrimSpace(backendHelp),
 
-		PathsSpecial: &logical.Paths{
-			Root: []string{
-				"config/*",
-			},
-		},
-
 		Paths: []*framework.Path{
 			pathConfigConnection(&b),
 			pathConfigLease(&b),
-			pathRoles(&b),
 			pathRoleCreate(&b),
+			pathRoles(&b),
 		},
 
 		Secrets: []*framework.Secret{
@@ -94,7 +88,7 @@ func (b *backend) ResetClient() {
 
 // Lease returns the lease information
 func (b *backend) Lease(s logical.Storage) (*configLease, error) {
-	entry, err := s.Get("config/lease")
+	entry, err := s.Get(leasePatternLabel)
 	if err != nil {
 		return nil, err
 	}

builtin/logical/rabbitmq/backend_test.go

@@ -58,7 +58,7 @@ func testAccPreCheck(t *testing.T) {
 
 func testAccStepConfig(t *testing.T) logicaltest.TestStep {
 	return logicaltest.TestStep{
-		Operation: logical.WriteOperation,
+		Operation: logical.UpdateOperation,
 		Path:      "config/connection",
 		Data: map[string]interface{}{
 			"uri":      os.Getenv("RABBITMQ_MG_URI"),
@@ -70,7 +70,7 @@ func testAccStepConfig(t *testing.T) logicaltest.TestStep {
 
 func testAccStepRole(t *testing.T) logicaltest.TestStep {
 	return logicaltest.TestStep{
-		Operation: logical.WriteOperation,
+		Operation: logical.UpdateOperation,
 		Path:      "roles/web",
 		Data: map[string]interface{}{
 			"tags":   "administrator",

builtin/logical/rabbitmq/path_config_connection.go

@@ -27,7 +27,7 @@ func pathConfigConnection(b *backend) *framework.Path {
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.WriteOperation: b.pathConnectionWrite,
+			logical.UpdateOperation: b.pathConnectionUpdate,
 		},
 
 		HelpSynopsis:    pathConfigConnectionHelpSyn,
@@ -35,7 +35,7 @@ func pathConfigConnection(b *backend) *framework.Path {
 	}
 }
 
-func (b *backend) pathConnectionWrite(req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+func (b *backend) pathConnectionUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 	uri := data.Get("uri").(string)
 	username := data.Get("username").(string)
 	password := data.Get("password").(string)

builtin/logical/rabbitmq/path_config_lease.go

@@ -1,6 +1,7 @@
 package rabbitmq
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
@@ -8,24 +9,34 @@ import (
 	"github.com/hashicorp/vault/logical/framework"
 )
 
-func pathConfigLease(b *backend) *framework.Path {
-	return &framework.Path{
-		Pattern: "config/lease",
-		Fields: map[string]*framework.FieldSchema{
-			"lease": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Default lease for roles.",
-			},
+const (
+	leaseLabel        = "ttl"
+	leaseMaxLabel     = "ttl_max"
+	leasePatternLabel = "config/" + leaseLabel
+)
 
-			"lease_max": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Maximum time a credential is valid for.",
-			},
+func configFields() map[string]*framework.FieldSchema {
+	return map[string]*framework.FieldSchema{
+		leaseLabel: &framework.FieldSchema{
+			Type:        framework.TypeDurationSecond,
+			Description: "Default " + leaseLabel + " for roles.",
 		},
 
+		leaseMaxLabel: &framework.FieldSchema{
+			Type:        framework.TypeDurationSecond,
+			Description: "Maximum time a credential is valid for.",
+		},
+	}
+}
+
+func pathConfigLease(b *backend) *framework.Path {
+	return &framework.Path{
+		Pattern: leasePatternLabel,
+		Fields:  configFields(),
+
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.ReadOperation:  b.pathLeaseRead,
-			logical.WriteOperation: b.pathLeaseWrite,
+			logical.ReadOperation:   b.pathLeaseRead,
+			logical.UpdateOperation: b.pathLeastUpdate,
 		},
 
 		HelpSynopsis:    pathConfigLeaseHelpSyn,
@@ -33,24 +44,15 @@ func pathConfigLease(b *backend) *framework.Path {
 	}
 }
 
-func (b *backend) pathLeaseWrite(
+func (b *backend) pathLeastUpdate(
 	req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
-	leaseRaw := d.Get("lease").(string)
-	leaseMaxRaw := d.Get("lease_max").(string)
-
-	lease, err := time.ParseDuration(leaseRaw)
+	lease, leaseMax, err := validateLeases(d)
 	if err != nil {
-		return logical.ErrorResponse(fmt.Sprintf(
-			"Invalid lease: %s", err)), nil
-	}
-	leaseMax, err := time.ParseDuration(leaseMaxRaw)
-	if err != nil {
-		return logical.ErrorResponse(fmt.Sprintf(
-			"Invalid lease: %s", err)), nil
+		return nil, err
 	}
 
 	// Store it
-	entry, err := logical.StorageEntryJSON("config/lease", &configLease{
+	entry, err := logical.StorageEntryJSON(leasePatternLabel, &configLease{
 		Lease:    lease,
 		LeaseMax: leaseMax,
 	})
@@ -77,8 +79,8 @@ func (b *backend) pathLeaseRead(
 
 	return &logical.Response{
 		Data: map[string]interface{}{
-			"lease":     lease.Lease.String(),
-			"lease_max": lease.LeaseMax.String(),
+			leaseLabel:    lease.Lease.String(),
+			leaseMaxLabel: lease.LeaseMax.String(),
 		},
 	}, nil
 }
@@ -88,16 +90,29 @@ type configLease struct {
 	LeaseMax time.Duration
 }
 
-const pathConfigLeaseHelpSyn = `
-Configure the default lease information for generated credentials.
-`
+func validateLeases(data *framework.FieldData) (lease, leaseMax time.Duration, err error) {
 
-const pathConfigLeaseHelpDesc = `
-This configures the default lease information used for credentials
-generated by this backend. The lease specifies the duration that a
+	leaseRaw := data.Get(leaseLabel).(int)
+	leaseMaxRaw := data.Get(leaseMaxLabel).(int)
+
+	if leaseRaw == 0 && leaseMaxRaw == 0 {
+		err = errors.New(leaseLabel + " or " + leaseMaxLabel + " must have a value")
+		return
+	}
+
+	return time.Duration(leaseRaw) * time.Second, time.Duration(leaseMaxRaw) * time.Second, nil
+}
+
+var pathConfigLeaseHelpSyn = fmt.Sprintf(`
+Configure the default %s information for generated credentials.
+`, leaseLabel)
+
+var pathConfigLeaseHelpDesc = fmt.Sprintf(`
+This configures the default %s information used for credentials
+generated by this backend. The %s specifies the duration that a
 credential will be valid for, as well as the maximum session for
 a set of credentials.
 
-The format for the lease is "1h" or integer and then unit. The longest
+The format for the %s is "1h" or integer and then unit. The longest
 unit is hour.
-`
+`, leaseLabel, leaseLabel, leaseLabel)

builtin/logical/rabbitmq/path_config_lease_test.go

@@ -0,0 +1,53 @@
+package rabbitmq
+
+import (
+	"testing"
+
+	"github.com/hashicorp/vault/logical/framework"
+)
+
+type validateLeasesTestCase struct {
+	Lease    int
+	LeaseMax int
+	Fail     bool
+}
+
+func TestConfigLease_validateLeases(t *testing.T) {
+	cases := map[string]validateLeasesTestCase{
+		"Both lease and lease max": {
+			Lease:    60 * 60,
+			LeaseMax: 60 * 60,
+		},
+		"Just lease": {
+			Lease:    60 * 60,
+			LeaseMax: 0,
+		},
+		"No lease nor lease max": {
+			Lease:    0,
+			LeaseMax: 0,
+			Fail:     true,
+		},
+	}
+
+	data := &framework.FieldData{
+		Schema: configFields(),
+	}
+	for name, c := range cases {
+		data.Raw = map[string]interface{}{
+			leaseLabel:    c.Lease,
+			leaseMaxLabel: c.LeaseMax,
+		}
+
+		_, _, err := validateLeases(data)
+		if err != nil && c.Fail {
+			// This was expected
+			continue
+		} else if err != nil {
+			// This was unexpected
+			t.Errorf("Failed: %s", name)
+		} else if err == nil && c.Fail {
+			// This was unexpected
+			t.Errorf("Failed to fail: %s", name)
+		}
+	}
+}

builtin/logical/rabbitmq/path_role_create.go

@@ -31,7 +31,11 @@ func pathRoleCreate(b *backend) *framework.Path {
 
 func (b *backend) pathRoleCreateRead(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	name := data.Get("name").(string)
+	// Validate name
+	name, err := validateName(data)
+	if err != nil {
+		return nil, err
+	}
 
 	// Get the role
 	role, err := b.Role(req.Storage, name)
@@ -51,15 +55,8 @@ func (b *backend) pathRoleCreateRead(
 		lease = &configLease{Lease: 1 * time.Hour}
 	}
 
-	// Generate the username, password and expiration. PG limits user to 63 characters
-	displayName := req.DisplayName
-	if len(displayName) > 26 {
-		displayName = displayName[:26]
-	}
-	username := fmt.Sprintf("%s-%s", displayName, uuid.GenerateUUID())
-	if len(username) > 63 {
-		username = username[:63]
-	}
+	// Ensure username is unique
+	username := fmt.Sprintf("%s-%s", req.DisplayName, uuid.GenerateUUID())
 	password := uuid.GenerateUUID()
 
 	// Get our connection
@@ -68,6 +65,10 @@ func (b *backend) pathRoleCreateRead(
 		return nil, err
 	}
 
+	if client == nil {
+		return logical.ErrorResponse("unable to get client"), nil
+	}
+
 	// Create the user
 	_, err = client.PutUser(username, rabbithole.UserSettings{
 		Password: password,
@@ -86,7 +87,12 @@ func (b *backend) pathRoleCreateRead(
 		})
 
 		if err != nil {
-			return nil, err
+			// Delete the user because it's in an unknown state
+			_, rmErr := client.DeleteUser(username)
+			if rmErr != nil {
+				return logical.ErrorResponse(fmt.Sprintf("failed to update user: %s, failed to delete user: %s, user: %s", err, rmErr, username)), rmErr
+			}
+			return logical.ErrorResponse(fmt.Sprintf("failed to update user: %s, 	  user: %s", err, username)), err
 		}
 	}
 

builtin/logical/rabbitmq/path_roles.go

@@ -2,35 +2,40 @@ package rabbitmq
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 )
 
+func rolesFields() map[string]*framework.FieldSchema {
+	return map[string]*framework.FieldSchema{
+		"name": &framework.FieldSchema{
+			Type:        framework.TypeString,
+			Description: "Name of the role.",
+		},
+
+		"tags": &framework.FieldSchema{
+			Type:        framework.TypeString,
+			Description: "Comma-separated list of tags for this role.",
+		},
+
+		"vhosts": &framework.FieldSchema{
+			Type:        framework.TypeString,
+			Description: "A map of virtual hosts to permissions.",
+		},
+	}
+}
+
 func pathRoles(b *backend) *framework.Path {
 	return &framework.Path{
 		Pattern: "roles/" + framework.GenericNameRegex("name"),
-		Fields: map[string]*framework.FieldSchema{
-			"name": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Name of the role.",
-			},
-
-			"tags": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Comma-separated list of tags for this role.",
-			},
-
-			"vhosts": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "A map of virtual hosts to permissions.",
-			},
-		},
+		Fields:  rolesFields(),
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
 			logical.ReadOperation:   b.pathRoleRead,
-			logical.WriteOperation:  b.pathRoleCreate,
+			logical.CreateOperation: b.pathRoleCreate,
 			logical.DeleteOperation: b.pathRoleDelete,
 		},
 
@@ -58,7 +63,13 @@ func (b *backend) Role(s logical.Storage, n string) (*roleEntry, error) {
 
 func (b *backend) pathRoleDelete(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	err := req.Storage.Delete("role/" + data.Get("name").(string))
+
+	name, err := validateName(data)
+	if err != nil {
+		return nil, err
+	}
+
+	err = req.Storage.Delete("role/" + name)
 	if err != nil {
 		return nil, err
 	}
@@ -68,7 +79,13 @@ func (b *backend) pathRoleDelete(
 
 func (b *backend) pathRoleRead(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	role, err := b.Role(req.Storage, data.Get("name").(string))
+
+	name, err := validateName(data)
+	if err != nil {
+		return nil, err
+	}
+
+	role, err := b.Role(req.Storage, name)
 	if err != nil {
 		return nil, err
 	}
@@ -86,7 +103,10 @@ func (b *backend) pathRoleRead(
 
 func (b *backend) pathRoleCreate(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	name := data.Get("name").(string)
+	name, err := validateName(data)
+	if err != nil {
+		return nil, err
+	}
 	tags := data.Get("tags").(string)
 	rawVHosts := data.Get("vhosts").(string)
 
@@ -124,6 +144,15 @@ type vhostPermission struct {
 	Read      string `json:"read"`
 }
 
+func validateName(data *framework.FieldData) (string, error) {
+	name := data.Get("name").(string)
+	if len(name) == 0 {
+		return "", errors.New("name is required")
+	}
+
+	return name, nil
+}
+
 const pathRoleHelpSyn = `
 Manage the roles that can be created with this backend.
 `

builtin/logical/rabbitmq/path_roles_test.go

@@ -0,0 +1,42 @@
+package rabbitmq
+
+import (
+	"testing"
+
+	"github.com/hashicorp/vault/logical/framework"
+)
+
+type validateNameTestCase struct {
+	Name string
+	Fail bool
+}
+
+func TestRoles_validateName(t *testing.T) {
+	cases := map[string]validateNameTestCase{
+		"test name": {
+			Name: "test",
+		},
+		"empty name": {
+			Name: "",
+			Fail: true,
+		},
+	}
+
+	data := &framework.FieldData{
+		Schema: rolesFields(),
+	}
+	for name, c := range cases {
+		data.Raw = map[string]interface{}{
+			"name": c.Name,
+		}
+
+		actual, err := validateName(data)
+		if err != nil && !c.Fail {
+			t.Error(err)
+		}
+
+		if c.Name != actual {
+			t.Errorf("Fail: %s: expected %s, got %s", name, c.Name, actual)
+		}
+	}
+}

builtin/logical/rabbitmq/secret_creds.go

@@ -2,7 +2,6 @@ package rabbitmq
 
 import (
 	"fmt"
-	"time"
 
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
@@ -26,9 +25,6 @@ func secretCreds(b *backend) *framework.Secret {
 			},
 		},
 
-		DefaultDuration:    1 * time.Hour,
-		DefaultGracePeriod: 10 * time.Minute,
-
 		Renew:  b.secretCredsRenew,
 		Revoke: b.secretCredsRevoke,
 	}
@@ -42,10 +38,10 @@ func (b *backend) secretCredsRenew(
 		return nil, err
 	}
 	if lease == nil {
-		lease = &configLease{Lease: 1 * time.Hour}
+		lease = &configLease{}
 	}
 
-	f := framework.LeaseExtend(lease.Lease, lease.LeaseMax, false)
+	f := framework.LeaseExtend(lease.Lease, lease.LeaseMax, b.System())
 	resp, err := f(req, d)
 	if err != nil {
 		return nil, err
@@ -61,7 +57,7 @@ func (b *backend) secretCredsRevoke(
 	if !ok {
 		return nil, fmt.Errorf("secret is missing username internal data")
 	}
-	username, ok := usernameRaw.(string)
+	username := usernameRaw.(string)
 
 	// Get our connection
 	client, err := b.Client(req.Storage)