Repository: TBD54566975/ssi-service
Pull request #293 (2023-02-20 12:16:44)

Revoke Key API 
# Overview
Revoke Key APIs

# Description
Revoke a key API

# How Has This Been Tested?
- [ ] Simple Test [Here](pkg/server/router/keystore_test.go) There is the possibility to add more tests, but with 24% code overage across the entire repo, I didn't spend a ton of time worrying about anything other than the basic test vector.

# Checklist

Before submitting this PR, please make sure:

- [x] I have read the CONTRIBUTING document.
- [x] My code is consistent with the rest of the project 
- [x] I have tagged the relevant reviewers and/or interested parties
- [x] I have updated the READMEs and other documentation of affected packages


codecov-commenter: # [Codecov](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) Report
> Merging [#293](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) (73952cc) into [main](https://codecov.io/gh/TBD54566975/ssi-service/commit/d1e9c54ca2c0ece5d6843bbd7c54d01eabc9ac2d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) (d1e9c54) will **decrease** coverage by `0.20%`.
> The diff coverage is `0.00%`.

```diff
@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   24.57%   24.38%   -0.20%     
==========================================
  Files          32       32              
  Lines        3182     3207      +25     
==========================================
  Hits          782      782              
- Misses       2281     2306      +25     
  Partials      119      119              
```


| [Impacted Files](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | Coverage Δ | |
|---|---|---|
| [pkg/server/router/keystore.go](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975#diff-cGtnL3NlcnZlci9yb3V0ZXIva2V5c3RvcmUuZ28=) | `10.25% <0.00%> (-2.45%)` | :arrow_down: |
| [pkg/service/keystore/service.go](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975#diff-cGtnL3NlcnZpY2Uva2V5c3RvcmUvc2VydmljZS5nbw==) | `39.59% <0.00%> (-2.25%)` | :arrow_down: |
| [pkg/service/keystore/storage.go](https://codecov.io/gh/TBD54566975/ssi-service/pull/293?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975#diff-cGtnL3NlcnZpY2Uva2V5c3RvcmUvc3RvcmFnZS5nbw==) | `26.59% <0.00%> (-0.58%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975)

andresuribe87: @andorsk would you mind describing the use case you're addressing with adding Deletion?
andorsk: @andresuribe87  sure: At a high level, just offering the option to delete a key was something I was using while testing things. Make key, delete it after. I also just don't see a great reason to _not_ support this, as it's a basic CRUD op with keys that I'd expect should be supportable unless there's not a good reason not to?
decentralgabe: Soft delete would be *safer* - meaning set a deleted flag in the DB but retain the key just in case. you can get into some hairy situations if you accidentally delete a key
andorsk: @decentralgabe agreed there are different ways to implement this. I was going for a simple implementation. We could easily add a field saying "Hide" into the `StoredKey` object if we want to. For my use case, actually deleting the key was sufficient and what I wanted to do.
andorsk: @decentralgabe changed from delete key to revoke key. 
andorsk: @decentralgabe change implementation to add "revocation" field.  Is e2e test passing required? Otherwise, would like feedback here if possible.
decentralgabe: @andorsk yes getting the e2e test is a requirement. It seems to be passing on the main branch and other PRs so it must be with something you introduced. If you can prove otherwise let me know 😄 
decentralgabe: @andorsk would you mind adding a `revokedAt` field?
andorsk: thanks @decentralgabe . Yep. Adding a revoked at field is a good callout. I'll add it. 

Regarding e2e, I'm wondering if there's possible some dependency that can break in the chain for e2e testing suite ( maybe why it's not "required" on the test suite? I.e e2e started breaking at [a576bd9](https://github.com/TBD54566975/ssi-service/pull/293/commits/a576bd958d004b1a9302cc48adf6d76798252609), which was just a merge into the main branch, so it's strange to me e2e started breaking. I can look into this a little more.
andorsk: lol. CI/CD testing just completed it started passing again, after adding the `revokedAt` field. I have a hard time believing adding a revoked field causes a failing test to pass, but I could be wrong here. It sounds like to me the `e2e` testing had some non-code dependencies that maybe were unavailable yesterday but maybe someone knows better?

Patch:
diff --git a/pkg/server/router/keystore.go b/pkg/server/router/keystore.go
index 4c481b1..6e0332d 100644
--- a/pkg/server/router/keystore.go
+++ b/pkg/server/router/keystore.go
@@ -131,3 +131,33 @@ func (ksr *KeyStoreRouter) GetKeyDetails(ctx context.Context, w http.ResponseWri
 	}
 	return framework.Respond(ctx, w, resp, http.StatusOK)
 }
+
+// RevokeKey godoc
+//
+// @Summary     Revoke Key
+// @Description Revoke a stored key
+// @Tags        KeyStoreAPI
+// @Accept      json
+// @Produce     json
+// @Param       id  path     string true "ID"
+// @Success     200
+// @Failure     400 {string} string "Bad request"
+// @Router      /v1/keys/{id} [delete]
+func (ksr *KeyStoreRouter) RevokeKey(ctx context.Context, w http.ResponseWriter, _ *http.Request) error {
+	id := framework.GetParam(ctx, IDParam)
+	if id == nil {
+		errMsg := "cannot delete key without ID parameter"
+		logrus.Error(errMsg)
+		return framework.NewRequestErrorMsg(errMsg, http.StatusBadRequest)
+	}
+
+	err := ksr.service.RevokeKey(ctx, keystore.RevokeKeyRequest{ID: *id})
+	if err != nil {
+		errMsg := fmt.Sprintf("could not delete key for id: %s", *id)
+		logrus.WithError(err).Error(errMsg)
+		return framework.NewRequestError(errors.Wrap(err, errMsg), http.StatusBadRequest)
+	}
+
+	var resp GetKeyDetailsResponse
+	return framework.Respond(ctx, w, resp, http.StatusOK)
+}
diff --git a/pkg/service/keystore/model.go b/pkg/service/keystore/model.go
index 3d85654..879944a 100644
--- a/pkg/service/keystore/model.go
+++ b/pkg/service/keystore/model.go
@@ -22,6 +22,8 @@ type GetKeyResponse struct {
 	Type       crypto.KeyType
 	Controller string
 	CreatedAt  string
+	Revoked    bool
+	RevokedAt  string
 	Key        gocrypto.PrivateKey
 }
 
@@ -34,4 +36,10 @@ type GetKeyDetailsResponse struct {
 	Type       crypto.KeyType
 	Controller string
 	CreatedAt  string
+	Revoked    bool
+	RevokedAt  string
+}
+
+type RevokeKeyRequest struct {
+	ID string
 }
diff --git a/pkg/service/keystore/service.go b/pkg/service/keystore/service.go
index 0e42795..32ce7da 100644
--- a/pkg/service/keystore/service.go
+++ b/pkg/service/keystore/service.go
@@ -123,9 +123,21 @@ func (s Service) GetKey(ctx context.Context, request GetKeyRequest) (*GetKeyResp
 		Controller: gotKey.Controller,
 		Key:        privKey,
 		CreatedAt:  gotKey.CreatedAt,
+		Revoked:    gotKey.Revoked,
+		RevokedAt:  gotKey.RevokedAt,
 	}, nil
 }
 
+func (s Service) RevokeKey(ctx context.Context, request RevokeKeyRequest) error {
+	logrus.Debugf("revoking key: %+v", request)
+	id := request.ID
+	err := s.storage.RevokeKey(ctx, id)
+	if err != nil {
+		return util.LoggingErrorMsgf(err, "could not delete key: %s", id)
+	}
+	return nil
+}
+
 func (s Service) GetKeyDetails(ctx context.Context, request GetKeyDetailsRequest) (*GetKeyDetailsResponse, error) {
 
 	logrus.Debugf("getting key: %+v", request)
@@ -143,6 +155,8 @@ func (s Service) GetKeyDetails(ctx context.Context, request GetKeyDetailsRequest
 		Type:       gotKeyDetails.KeyType,
 		Controller: gotKeyDetails.Controller,
 		CreatedAt:  gotKeyDetails.CreatedAt,
+		Revoked:    gotKeyDetails.Revoked,
+		RevokedAt:  gotKeyDetails.RevokedAt,
 	}, nil
 }
 
diff --git a/pkg/service/keystore/storage.go b/pkg/service/keystore/storage.go
index 6206dbd..a145b0d 100644
--- a/pkg/service/keystore/storage.go
+++ b/pkg/service/keystore/storage.go
@@ -2,6 +2,7 @@ package keystore
 
 import (
 	"context"
+	"time"
 
 	"github.com/goccy/go-json"
 	"github.com/mr-tron/base58"
@@ -18,6 +19,8 @@ type StoredKey struct {
 	Controller string         `json:"controller"`
 	KeyType    crypto.KeyType `json:"keyType"`
 	Base58Key  string         `json:"key"`
+	Revoked    bool           `json:"revoked"`
+	RevokedAt  string         `json:"revokedAt"`
 	CreatedAt  string         `json:"createdAt"`
 }
 
@@ -26,6 +29,8 @@ type KeyDetails struct {
 	ID         string         `json:"id"`
 	Controller string         `json:"controller"`
 	KeyType    crypto.KeyType `json:"keyType"`
+	Revoked    bool           `json:"revoked"`
+	RevokedAt  string         `json:"revokedAt"`
 	CreatedAt  string         `json:"createdAt"`
 }
 
@@ -125,6 +130,23 @@ func (kss *Storage) StoreKey(ctx context.Context, key StoredKey) error {
 	return kss.db.Write(ctx, namespace, id, encryptedKey)
 }
 
+func (kss *Storage) RevokeKey(ctx context.Context, id string) error {
+	key, err := kss.GetKey(ctx, id)
+	if err != nil {
+		return err
+	}
+	if key == nil {
+		return errors.New("key not found")
+	}
+
+	key.Revoked = true
+	key.RevokedAt = time.Now().UTC().Format(time.RFC3339)
+	if err = kss.StoreKey(ctx, *key); err != nil {
+		return err
+	}
+	return nil
+}
+
 func (kss *Storage) GetKey(ctx context.Context, id string) (*StoredKey, error) {
 	storedKeyBytes, err := kss.db.Read(ctx, namespace, id)
 	if err != nil {
@@ -163,5 +185,6 @@ func (kss *Storage) GetKeyDetails(ctx context.Context, id string) (*KeyDetails,
 		Controller: stored.Controller,
 		KeyType:    stored.KeyType,
 		CreatedAt:  stored.CreatedAt,
+		Revoked:    stored.Revoked,
 	}, nil
 }


Test patch:
diff --git a/pkg/server/router/keystore_test.go b/pkg/server/router/keystore_test.go
index 1b5b7fa..6b2071b 100644
--- a/pkg/server/router/keystore_test.go
+++ b/pkg/server/router/keystore_test.go
@@ -86,5 +86,12 @@ func TestKeyStoreRouter(t *testing.T) {
 		assert.Equal(tt, keyID, gotDetails.ID)
 		assert.Equal(tt, crypto.Ed25519, gotDetails.Type)
 		assert.Equal(tt, "did:test:me", gotDetails.Controller)
+
+		// delete key checks
+		err = keyStoreService.RevokeKey(context.Background(), keystore.RevokeKeyRequest{ID: keyID})
+		assert.NoError(tt, err)
+		gotDetails, err = keyStoreService.GetKeyDetails(context.Background(), keystore.GetKeyDetailsRequest{ID: keyID})
+		assert.NoError(tt, err)
+		assert.True(tt, gotDetails.Revoked)
 	})
 }