From 23e6de57a2e3a917c98dd3a38aa33f63e8655c3e Mon Sep 17 00:00:00 2001
From: Mariano Cano <mariano@smallstep.com>
Date: Wed, 13 Mar 2019 11:26:18 -0700
Subject: [PATCH] Address comments in code review.

---
 authority/authority_test.go         |  6 ------
 authority/authorize.go              |  1 -
 authority/provisioner/collection.go | 10 +++++-----
 authority/provisioner/jwk.go        |  2 +-
 authority/provisioners_test.go      | 13 -------------
 5 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/authority/authority_test.go b/authority/authority_test.go
index 1ef7c2d5..e008b22d 100644
--- a/authority/authority_test.go
+++ b/authority/authority_test.go
@@ -126,12 +126,6 @@ func TestAuthorityNew(t *testing.T) {
 					// sanity check
 					_, ok = auth.provisioners.Load("fooo")
 					assert.False(t, ok)
-
-					// assert.Equals(t, auth.audiences, []string{
-					// 	"step-certificate-authority",
-					// 	"https://127.0.0.1/sign",
-					// 	"https://127.0.0.1/1.0/sign",
-					// })
 				}
 			}
 		})
diff --git a/authority/authorize.go b/authority/authorize.go
index 0b207ee9..2d63a58e 100644
--- a/authority/authorize.go
+++ b/authority/authorize.go
@@ -25,7 +25,6 @@ type Claims struct {
 
 // Authorize authorizes a signature request by validating and authenticating
 // a OTT that must be sent w/ the request.
-// TODO(mariano): protection against reuse for oidc
 func (a *Authority) Authorize(ott string) ([]provisioner.SignOption, error) {
 	var errContext = map[string]interface{}{"ott": ott}
 
diff --git a/authority/provisioner/collection.go b/authority/provisioner/collection.go
index 0998adb7..8edd1965 100644
--- a/authority/provisioner/collection.go
+++ b/authority/provisioner/collection.go
@@ -101,8 +101,8 @@ func (c *Collection) LoadByCertificate(cert *x509.Certificate) (Interface, bool)
 	return &noop{}, true
 }
 
-// LoadEncryptedKey returns a the encrypted key by KeyID. At this moment only
-// JWK encrypted keys are indexed by KeyID.
+// LoadEncryptedKey returns an encrypted key by indexed by KeyID. At this moment
+// only JWK encrypted keys are indexed by KeyID.
 func (c *Collection) LoadEncryptedKey(keyID string) (string, bool) {
 	p, ok := loadProvisioner(c.byKey, keyID)
 	if !ok {
@@ -112,15 +112,15 @@ func (c *Collection) LoadEncryptedKey(keyID string) (string, bool) {
 	return key, ok
 }
 
-// Store adds a provisioner to the collection, it makes sure two provisioner
-// does not have the same ID.
+// Store adds a provisioner to the collection and enforces the uniqueness of
+// provisioner IDs.
 func (c *Collection) Store(p Interface) error {
 	// Store provisioner always in byID. ID must be unique.
 	if _, loaded := c.byID.LoadOrStore(p.GetID(), p); loaded == true {
 		return errors.New("cannot add multiple provisioners with the same id")
 	}
 
-	// Store provisioner in byKey in EncryptedKey is defined.
+	// Store provisioner in byKey if EncryptedKey is defined.
 	if kid, _, ok := p.GetEncryptedKey(); ok {
 		c.byKey.Store(kid, p)
 	}
diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go
index 79933f30..d81ea21f 100644
--- a/authority/provisioner/jwk.go
+++ b/authority/provisioner/jwk.go
@@ -47,7 +47,7 @@ func (p *JWK) GetEncryptedKey() (string, string, bool) {
 	return p.Key.KeyID, p.EncryptedKey, len(p.EncryptedKey) > 0
 }
 
-// Init initializes and validates a the fields of Provisioner type.
+// Init initializes and validates the fields of a JWK type.
 func (p *JWK) Init(config Config) (err error) {
 	switch {
 	case p.Name == "":
diff --git a/authority/provisioners_test.go b/authority/provisioners_test.go
index 53f2c733..303c4e8a 100644
--- a/authority/provisioners_test.go
+++ b/authority/provisioners_test.go
@@ -38,19 +38,6 @@ func TestGetEncryptedKey(t *testing.T) {
 					http.StatusNotFound, context{}},
 			}
 		},
-		// "fail-invalid-type-found": func(t *testing.T) *ek {
-		// 	c, err := LoadConfiguration("../ca/testdata/ca.json")
-		// 	assert.FatalError(t, err)
-		// 	a, err := New(c)
-		// 	assert.FatalError(t, err)
-		// 	a.encryptedKeyIndex.Store("foo", 5)
-		// 	return &ek{
-		// 		a:   a,
-		// 		kid: "foo",
-		// 		err: &apiError{errors.Errorf("stored value is not a string"),
-		// 			http.StatusInternalServerError, context{}},
-		// 	}
-		// },
 	}
 
 	for name, genTestCase := range tests {