Merge pull request #3681 from infrahq/dnephin/prevent-delete-of-curre…

…nt-access-key

Refuse to delete the access key being used by the API request

internal/access/access_key.go

@@ -57,9 +57,7 @@ func CreateAccessKey(c *gin.Context, accessKey *models.AccessKey) (body string,
 	return body, err
 }
 
-func DeleteAccessKey(c *gin.Context, id uid.ID, name string) error {
-	rCtx := GetRequestContext(c)
-
+func DeleteAccessKey(rCtx RequestContext, id uid.ID, name string) error {
 	var key *models.AccessKey
 	var err error
 
@@ -89,12 +87,15 @@ func DeleteAccessKey(c *gin.Context, id uid.ID, name string) error {
 	if key.IssuedFor == rCtx.Authenticated.User.ID {
 		// users can delete their own keys
 	} else {
-		_, err := RequireInfraRole(c, models.InfraAdminRole)
-		if err != nil {
+		if err := IsAuthorized(rCtx, models.InfraAdminRole); err != nil {
 			return HandleAuthErr(err, "access key", "delete", models.InfraAdminRole)
 		}
 	}
 
+	if rCtx.Authenticated.AccessKey.ID == key.ID {
+		return fmt.Errorf("%w: cannot delete the access key used by this request", internal.ErrBadRequest)
+	}
+
 	return data.DeleteAccessKeys(rCtx.DBTxn, data.DeleteAccessKeysOptions{ByID: key.ID})
 }
 

internal/access/access_key_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/gin-gonic/gin"
 	"gotest.tools/v3/assert"
 
 	"github.com/infrahq/infra/internal/server/data"
@@ -21,7 +22,13 @@ func TestAccessKeys_SelfManagement(t *testing.T) {
 	err = data.CreateIdentity(db, user)
 	assert.NilError(t, err)
 
-	c, _ := loginAs(&data.Transaction{DB: db.DB}, user, org)
+	c, _ := gin.CreateTestContext(nil)
+	tx := txnForTestCase(t, db).WithOrgID(org.ID)
+	rCtx := RequestContext{
+		DBTxn:         tx,
+		Authenticated: Authenticated{User: user, Organization: org},
+	}
+	c.Set(RequestContextKey, rCtx)
 
 	t.Run("can manage my own keys", func(t *testing.T) {
 		key := &models.AccessKey{
@@ -33,7 +40,9 @@ func TestAccessKeys_SelfManagement(t *testing.T) {
 		_, err = CreateAccessKey(c, key)
 		assert.NilError(t, err)
 
-		err = DeleteAccessKey(c, key.ID, "")
+		r := rCtx // shallow copy
+		r.Authenticated.AccessKey = &models.AccessKey{}
+		err = DeleteAccessKey(r, key.ID, "")
 		assert.NilError(t, err)
 	})
 

internal/access/access_test.go

@@ -26,17 +26,6 @@ func setupDB(t *testing.T) *data.DB {
 	return db
 }
 
-func loginAs(db *data.Transaction, user *models.Identity, org *models.Organization) (*gin.Context, *data.Transaction) {
-	ctx, _ := gin.CreateTestContext(nil)
-	tx := db.WithOrgID(org.ID)
-	ctx.Set(RequestContextKey, RequestContext{
-		DBTxn:         tx,
-		Authenticated: Authenticated{User: user, Organization: org},
-	})
-
-	return ctx, tx
-}
-
 func setupAccessTestContext(t *testing.T) (*gin.Context, *data.Transaction, *models.Provider) {
 	// setup db and context
 	db := setupDB(t)

internal/server/access_keys.go

@@ -26,12 +26,12 @@ func (a *API) ListAccessKeys(c *gin.Context, r *api.ListAccessKeysRequest) (*api
 
 // DeleteAccessKey deletes an access key by id
 func (a *API) DeleteAccessKey(c *gin.Context, r *api.Resource) (*api.EmptyResponse, error) {
-	return nil, access.DeleteAccessKey(c, r.ID, "")
+	return nil, access.DeleteAccessKey(getRequestContext(c), r.ID, "")
 }
 
 // DeleteAccessKeys deletes 0 or more access keys by any attribute
 func (a *API) DeleteAccessKeys(c *gin.Context, r *api.DeleteAccessKeyRequest) (*api.EmptyResponse, error) {
-	return nil, access.DeleteAccessKey(c, 0, r.Name)
+	return nil, access.DeleteAccessKey(getRequestContext(c), 0, r.Name)
 }
 
 func (a *API) CreateAccessKey(c *gin.Context, r *api.CreateAccessKeyRequest) (*api.CreateAccessKeyResponse, error) {

internal/server/access_keys_test.go

@@ -243,53 +243,6 @@ func TestAPI_ListAccessKeys(t *testing.T) {
 		}
 	})
 
-	t.Run("delete by name", func(t *testing.T) {
-		key := &models.AccessKey{
-			Name:       "deleteme",
-			IssuedFor:  user.ID,
-			ProviderID: provider.ID,
-			ExpiresAt:  time.Now().Add(5 * time.Minute),
-		}
-		_, err := data.CreateAccessKey(srv.db, key)
-		assert.NilError(t, err)
-
-		resp := httptest.NewRecorder()
-		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deleteme", nil)
-		req.Header.Set("Authorization", "Bearer "+ak1.Token())
-		req.Header.Set("Infra-Version", apiVersionLatest)
-
-		routes.ServeHTTP(resp, req)
-		assert.Equal(t, resp.Code, http.StatusNoContent)
-	})
-
-	t.Run("delete by name as non-admin", func(t *testing.T) {
-		user := &models.Identity{Model: models.Model{ID: uid.New()}, Name: "[email protected]", OrganizationMember: models.OrganizationMember{OrganizationID: provider.OrganizationID}}
-		err := data.CreateIdentity(db, user)
-		assert.NilError(t, err)
-
-		key := &models.AccessKey{Name: "deletemetoo", IssuedFor: user.ID, ProviderID: provider.ID, ExpiresAt: time.Now().Add(5 * time.Minute), OrganizationMember: models.OrganizationMember{OrganizationID: provider.OrganizationID}}
-		_, err = data.CreateAccessKey(srv.db, key)
-		assert.NilError(t, err)
-
-		resp := httptest.NewRecorder()
-		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deletemetoo", nil)
-		req.Header.Set("Authorization", "Bearer "+key.Token())
-		req.Header.Set("Infra-Version", apiVersionLatest)
-
-		routes.ServeHTTP(resp, req)
-		assert.Equal(t, resp.Code, http.StatusNoContent)
-	})
-
-	t.Run("delete by name missing", func(t *testing.T) {
-		resp := httptest.NewRecorder()
-		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deletesomething", nil)
-		req.Header.Set("Authorization", "Bearer "+ak1.Token())
-		req.Header.Set("Infra-Version", apiVersionLatest)
-
-		routes.ServeHTTP(resp, req)
-		assert.Equal(t, resp.Code, http.StatusNotFound)
-	})
-
 	t.Run("show expired", func(t *testing.T) {
 		resp := httptest.NewRecorder()
 		req := httptest.NewRequest(http.MethodGet, "/api/access-keys?showExpired=1", nil)
@@ -345,3 +298,110 @@ func TestAPI_ListAccessKeys(t *testing.T) {
 		assert.Equal(t, errMsg.Code, int32(400))
 	})
 }
+
+func TestAPI_DeleteAccessKey(t *testing.T) {
+	srv := setupServer(t, withAdminUser)
+	routes := srv.GenerateRoutes()
+
+	db := srv.DB()
+
+	user := &models.Identity{
+		Model: models.Model{ID: uid.New()},
+		Name:  "[email protected]",
+	}
+	err := data.CreateIdentity(db, user)
+	assert.NilError(t, err)
+	provider := data.InfraProvider(db)
+	err = data.CreateGrant(db, &models.Grant{
+		Subject:   user.PolyID(),
+		Privilege: "admin",
+		Resource:  "infra",
+	})
+	assert.NilError(t, err)
+
+	ak1 := &models.AccessKey{
+		Name:       "foo",
+		IssuedFor:  user.ID,
+		ProviderID: provider.ID,
+		ExpiresAt:  time.Now().UTC().Add(5 * time.Minute),
+	}
+	_, err = data.CreateAccessKey(db, ak1)
+	assert.NilError(t, err)
+
+	t.Run("delete by name", func(t *testing.T) {
+		key := &models.AccessKey{
+			Name:       "deleteme",
+			IssuedFor:  user.ID,
+			ProviderID: provider.ID,
+			ExpiresAt:  time.Now().Add(5 * time.Minute),
+		}
+		_, err := data.CreateAccessKey(srv.db, key)
+		assert.NilError(t, err)
+
+		resp := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deleteme", nil)
+		req.Header.Set("Authorization", "Bearer "+ak1.Token())
+		req.Header.Set("Infra-Version", apiVersionLatest)
+
+		routes.ServeHTTP(resp, req)
+		assert.Equal(t, resp.Code, http.StatusNoContent)
+	})
+
+	t.Run("do not allow delete of the key used in the request", func(t *testing.T) {
+		resp := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys/"+ak1.ID.String(), nil)
+		req.Header.Set("Authorization", "Bearer "+ak1.Token())
+		req.Header.Set("Infra-Version", apiVersionLatest)
+
+		routes.ServeHTTP(resp, req)
+		assert.Equal(t, resp.Code, http.StatusBadRequest)
+	})
+
+	t.Run("delete by name as non-admin", func(t *testing.T) {
+		user := &models.Identity{
+			Model:              models.Model{ID: uid.New()},
+			Name:               "[email protected]",
+			OrganizationMember: models.OrganizationMember{OrganizationID: provider.OrganizationID},
+		}
+		err := data.CreateIdentity(db, user)
+		assert.NilError(t, err)
+
+		key1 := &models.AccessKey{
+			Name:               "deletemetoo",
+			IssuedFor:          user.ID,
+			ProviderID:         provider.ID,
+			ExpiresAt:          time.Now().Add(5 * time.Minute),
+			OrganizationMember: models.OrganizationMember{OrganizationID: provider.OrganizationID},
+		}
+		_, err = data.CreateAccessKey(srv.db, key1)
+		assert.NilError(t, err)
+
+		key2 := &models.AccessKey{
+			Name:               "deletemetoo2",
+			IssuedFor:          user.ID,
+			ProviderID:         provider.ID,
+			ExpiresAt:          time.Now().Add(5 * time.Minute),
+			OrganizationMember: models.OrganizationMember{OrganizationID: provider.OrganizationID},
+		}
+		_, err = data.CreateAccessKey(srv.db, key2)
+		assert.NilError(t, err)
+
+		resp := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deletemetoo2", nil)
+		req.Header.Set("Authorization", "Bearer "+key1.Token())
+		req.Header.Set("Infra-Version", apiVersionLatest)
+
+		routes.ServeHTTP(resp, req)
+		assert.Equal(t, resp.Code, http.StatusNoContent)
+	})
+
+	t.Run("delete by name missing", func(t *testing.T) {
+		resp := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodDelete, "/api/access-keys?name=deletesomething", nil)
+		req.Header.Set("Authorization", "Bearer "+ak1.Token())
+		req.Header.Set("Infra-Version", apiVersionLatest)
+
+		routes.ServeHTTP(resp, req)
+		assert.Equal(t, resp.Code, http.StatusNotFound)
+	})
+}