Merge pull request #1 from KevinPike/rabbitmq

Address RabbitMQ feedback

builtin/logical/rabbitmq/README.md

@@ -0,0 +1,10 @@
+# RabbitMQ Backend
+
+## Testing
+
+There are unit and integration RabbitMQ backend tests. Unit tests can be run by `go test`. Integration tests require setting the following environment variables:
+```
+RABBITMQ_CONNECTION_URI=
+RABBITMQ_USERNAME=
+RABBITMQ_PASSWORD=
+```

builtin/logical/rabbitmq/backend.go

@@ -21,18 +21,12 @@ 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),
 			pathListRoles(&b),
-			pathRoles(&b),
 			pathRoleCreate(&b),
+			pathRoles(&b),
 		},
 
 		Secrets: []*framework.Secret{
@@ -95,7 +89,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

@@ -44,15 +44,25 @@ func TestBackend_roleCrud(t *testing.T) {
 	})
 }
 
+const (
+	uriEnv      = "RABBITMQ_CONNECTION_URI"
+	usernameEnv = "RABBITMQ_USERNAME"
+	passwordEnv = "RABBITMQ_PASSWORD"
+)
+
+func mustSet(name string) string {
+	return fmt.Sprintf("%s must be set for acceptance tests", name)
+}
+
 func testAccPreCheck(t *testing.T) {
-	if uri := os.Getenv("RABBITMQ_MG_URI"); uri == "" {
-		t.Fatal("RABBITMQ_MG_URI must be set for acceptance tests")
+	if uri := os.Getenv(uriEnv); uri == "" {
+		t.Fatal(mustSet(uriEnv))
 	}
-	if username := os.Getenv("RABBITMQ_MG_USERNAME"); username == "" {
-		t.Fatal("RABBITMQ_MG_USERNAME must be set for acceptance tests")
+	if username := os.Getenv(usernameEnv); username == "" {
+		t.Fatal(mustSet(usernameEnv))
 	}
-	if password := os.Getenv("RABBITMQ_MG_PASSWORD"); password == "" {
-		t.Fatal("RABBITMQ_MG_PASSWORD must be set for acceptance tests")
+	if password := os.Getenv(passwordEnv); password == "" {
+		t.Fatal(mustSet(passwordEnv))
 	}
 }
 
@@ -61,9 +71,9 @@ func testAccStepConfig(t *testing.T) logicaltest.TestStep {
 		Operation: logical.UpdateOperation,
 		Path:      "config/connection",
 		Data: map[string]interface{}{
-			"uri":      os.Getenv("RABBITMQ_MG_URI"),
-			"username": os.Getenv("RABBITMQ_MG_USERNAME"),
-			"password": os.Getenv("RABBITMQ_MG_PASSWORD"),
+			"connection_uri": os.Getenv(uriEnv),
+			"username":       os.Getenv(usernameEnv),
+			"password":       os.Getenv(passwordEnv),
 		},
 	}
 }
@@ -100,7 +110,7 @@ func testAccStepReadCreds(t *testing.T, b logical.Backend, name string) logicalt
 			}
 			log.Printf("[WARN] Generated credentials: %v", d)
 
-			uri := os.Getenv("RABBITMQ_MG_URI")
+			uri := os.Getenv(uriEnv)
 
 			client, err := rabbithole.NewClient(uri, d.Username, d.Password)
 			if err != nil {
@@ -182,15 +192,15 @@ func testAccStepReadRole(t *testing.T, name, tags, rawVHosts string) logicaltest
 				}
 
 				if actualPermission.Configure != permission.Configure {
-					fmt.Errorf("expected permission %s to be %s, got %s", "configure", permission.Configure, actualPermission.Configure)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "configure", permission.Configure, actualPermission.Configure)
 				}
 
 				if actualPermission.Write != permission.Write {
-					fmt.Errorf("expected permission %s to be %s, got %s", "write", permission.Write, actualPermission.Write)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "write", permission.Write, actualPermission.Write)
 				}
 
 				if actualPermission.Read != permission.Read {
-					fmt.Errorf("expected permission %s to be %s, got %s", "read", permission.Read, actualPermission.Read)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "read", permission.Read, actualPermission.Read)
 				}
 			}
 

builtin/logical/rabbitmq/path_config_connection.go

@@ -32,15 +32,15 @@ func pathConfigConnection(b *backend) *framework.Path {
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathConnectionWrite,
+			logical.UpdateOperation: b.pathConnectionUpdate,
 		},
 
 		HelpSynopsis:    pathConfigConnectionHelpSyn,
 		HelpDescription: pathConfigConnectionHelpDesc,
 	}
 }
 
-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("connection_uri").(string)
 	username := data.Get("username").(string)
 	password := data.Get("password").(string)

builtin/logical/rabbitmq/path_config_lease.go

@@ -1,56 +1,58 @@
 package rabbitmq
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 )
 
+const (
+	leaseLabel        = "ttl"
+	leaseMaxLabel     = "ttl_max"
+	leasePatternLabel = "config/" + leaseLabel
+)
+
+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: "config/lease",
-		Fields: map[string]*framework.FieldSchema{
-			"lease": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Default lease for roles.",
-			},
-
-			"lease_max": &framework.FieldSchema{
-				Type:        framework.TypeString,
-				Description: "Maximum time a credential is valid for.",
-			},
-		},
+		Pattern: leasePatternLabel,
+		Fields:  configFields(),
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.ReadOperation:  b.pathLeaseRead,
-			logical.UpdateOperation: b.pathLeaseWrite,
+			logical.ReadOperation:   b.pathLeaseRead,
+			logical.UpdateOperation: b.pathLeaseUpdate,
 		},
 
 		HelpSynopsis:    pathConfigLeaseHelpSyn,
 		HelpDescription: pathConfigLeaseHelpDesc,
 	}
 }
 
-func (b *backend) pathLeaseWrite(
+func (b *backend) pathLeaseUpdate(
 	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) {
+
+	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)
 
-const pathConfigLeaseHelpDesc = `
-This configures the default lease information used for credentials
-generated by this backend. The lease specifies the duration that a
+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

@@ -30,7 +30,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)
@@ -50,15 +54,8 @@ func (b *backend) pathRoleCreateRead(
 		lease = &configLease{}
 	}
 
-	displayName := req.DisplayName
-	if len(displayName) > 26 {
-		displayName = displayName[:26]
-	}
-	userUUID := uuid.GenerateUUID()
-	username := fmt.Sprintf("%s-%s", displayName, userUUID)
-	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
@@ -67,6 +64,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,
@@ -85,7 +86,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
 		}
 	}