TopicDelete Ensign integration #223

pdeziel · 2023-02-16T20:33:05Z

Scope of changes

This updates the TopicDelete endpoint so that it actually calls Ensign to delete the topic. This endpoint is protected by a confirmation token to prevent accidental deletes, since this will destroy all of the data in the topic. The user must call the endpoint once to retrieve the confirmation token. At this point the frontend should prompt the user for a confirmation, then send a second request to the endpoint including the token to actually perform the delete.

Since the delete is actually performed in Ensign, the Tenant database object will stay around, although the frontend can interpret the state value on the topic in order to determine whether or not to render the resource for the user. In a future story we will actually implement a routine on Tenant which polls Ensign asynchronously and makes the database updates.

Fixes SC-13773

Type of change

Acceptance criteria

See checklist

Author checklist

I have manually tested the change and/or added automation in the form of unit tests or integration tests
I have updated the dependencies list
I have recompiled and included new protocol buffers to reflect changes I made
I have added new test fixtures as needed to support added tests
Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.
Are there any TODOs in this PR that should be turned into stories?
Does the confirmation flow make sense?

shortcut-integration · 2023-02-16T20:33:09Z

This pull request has been linked to Shortcut Story #13773: Update TopicDelete.

pdeziel · 2023-02-16T20:34:03Z

pkg/utils/secrets/token.go

+		}
+	}
+	return true
+}


Copied directly from GDS

Doh, not sure why we didn't use crypto/rand in GDS - but that's probably a problem. Would you mind copying the quarterdeck/keygen package instead?

https://github.com/rotationalio/ensign/blob/main/pkg/quarterdeck/keygen/keygen.go

(You can use it directly if you'd like, but copying it into a secrets utils is fine; I'd like them decoupled so that we can change the way that Quarterdeck issues api keys; e.g. either use the keygen package directly or port it to utils so quarterdeck does not have a dependency on utils).

I'll create a story to fix GDS.

Sure that works for me

pdeziel · 2023-02-16T20:35:34Z

pkg/tenant/topics_test.go

-	err = suite.client.TopicDelete(ctx, "01GNA926JCTKDH3VZBTJM8MAF6")
-	suite.requireError(err, http.StatusNotFound, "could not delete topic", "expected error when topic ID is not found")
+	_, err = suite.client.TopicDelete(ctx, req)
+	suite.requireError(err, http.StatusNotFound, "topic not found", "expected error when topic ID is not found")


The rest of the endpoint will be tested in SC-13774

bbengfort

Great start! I had an alternative suggestion for the token; let me know what you think.

pkg/tenant/api/v1/api.go

bbengfort · 2023-02-17T16:27:00Z

pkg/tenant/db/topics.go

+	ConfirmDeleteToken  string                   `msgpack:"confirm_delete_token"`
+	ConfirmDeleteExpire time.Time                `msgpack:"confirm_delete_expire"`


I'd recommend the token be a base64 encoded msgpack data structure that includes the topic ID and expiration time stamp as well as a cryptographically randomly generated string. That way the token is self composed and in the future we can do things like cryptographically sign the token for further security.

bbengfort · 2023-02-17T16:27:24Z

pkg/tenant/api/v1/api.go

+// data by including a confirmation token in the request.
+type Confirmation struct {
+	ID           string `json:"id"`
+	Name         string `json:"name"`


Is the name something you're thinking the user should supply?

I think it would be part of the response, so the user shouldn't supply it in the request but it gives the frontend something to display in the confirmation prompt without having to reference the original object.

bbengfort · 2023-02-17T16:27:59Z

pkg/tenant/topics.go

+// as all of the data in Ensign. Because this is irreversible, the first call returns
+// a confirmation token to the user. The user must provide this token in a subsequent
+// request in order to confirm the deletion. Because this operation is asynchronous,
+// the endpoint returns a 202 Accepted response.


Good choice of status code!

bbengfort · 2023-02-17T16:30:54Z

pkg/tenant/topics.go

-		log.Error().Err(err).Msg("could not parse topic ulid")
-		c.JSON(http.StatusBadRequest, api.ErrorResponse("could not parse topic ulid"))
+		log.Warn().Err(err).Msg("could not parse topic id")
+		c.JSON(http.StatusBadRequest, api.ErrorResponse("could not parse topic id"))


I'm thinking I'd actually prefer to return a 404 here (I know this has nothing to do with your PR, but just thinking about it again. E.g. if the user puts in /project/:projectID/topics/foo - that's not parseable as a ULID, but it should be not found not 400 right? The 404 is a little more secure and will allow us to change ID types in the future.

Suggested change

c.JSON(http.StatusBadRequest, api.ErrorResponse("could not parse topic id"))

c.JSON(http.StatusNotFound, api.ErrorResponse("topic not found"))

Agreed, I think we've gone back and forth but it does seem to make a bit more sense to return 404 since the ID is a part of the URL.

bbengfort · 2023-02-17T16:35:36Z

pkg/tenant/topics.go

+	// Verify that the user owns the topic
+	if claims.OrgID != topic.OrgID.String() {
+		log.Warn().Err(err).Str("user_org", claims.OrgID).Str("topic_org", topic.OrgID.String()).Msg("topic OrgID does not match user OrgID")
+		c.JSON(http.StatusForbidden, api.ErrorResponse("user is not authorized to delete this topic"))


This is not for this PR - but in general I've been thinking about security principles with our responses. If we return a 403 here then we're letting someone know that the topic exists and belongs to a different organization -- which could be information they could use to get a topic ID to do other attacks.

E.g. a user simply has to register for an account to get valid claims then submit API requests with different Topic IDs - once they get a 403 instead of a 404 they know they have a good topic id belonging to someone else.

Because of this I think it is probably better to return a 404 here; e.g. semantically it is "no topic found belonging to that organization". But that gives no information that the topic even exists.

Suggested change

c.JSON(http.StatusForbidden, api.ErrorResponse("user is not authorized to delete this topic"))

c.JSON(http.StatusNotFound, api.ErrorResponse("topic not found"))

Yeah that makes sense, at some point we might need to audit the other Tenant endpoints to make sure we're being consistent about the responses and not accidentally exposing info.

Could you create a future story for that?

bbengfort · 2023-02-17T16:39:37Z

pkg/tenant/topics.go

+	// Verify that the confirmation token is correct
+	if confirm.ConfirmToken != topic.ConfirmDeleteToken {
+		log.Warn().Err(err).Msg("confirmation token does not match")
+		c.JSON(http.StatusBadRequest, api.ErrorResponse("unrecognized confirmation token"))


I think we should return a 412 Precondition Failed here to let the user know they should try the delete without the confirmation token again and then use a valid confirmation token.

Suggested change

c.JSON(http.StatusBadRequest, api.ErrorResponse("unrecognized confirmation token"))

c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("unrecognized confirmation token"))

bbengfort · 2023-02-17T16:39:50Z

pkg/tenant/topics.go

+	// Verify the confirmation token has not expired
+	if time.Now().After(topic.ConfirmDeleteExpire) {
+		log.Warn().Err(err).Msg("confirmation token has expired")
+		c.JSON(http.StatusBadRequest, api.ErrorResponse("confirmation token has expired"))


Suggested change

c.JSON(http.StatusBadRequest, api.ErrorResponse("confirmation token has expired"))

c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("confirmation token has expired"))

Going to be fun to document this in Postman!

bbengfort · 2023-02-17T16:40:27Z

pkg/tenant/topics.go

+	// The delete request is asynchronous so just update the state in the database
+	topic.State = tombstone.State
+	if err = db.UpdateTopic(ctx, topic); err != nil {
+		log.Error().Err(err).Str("topicID", topicID.String()).Msg("could not update topic state")
+		c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not delete topic"))


Nicely done!

bbengfort · 2023-02-17T16:42:51Z

pkg/utils/secrets/token.go

+		}
+	}
+	return true
+}


Doh, not sure why we didn't use crypto/rand in GDS - but that's probably a problem. Would you mind copying the quarterdeck/keygen package instead?

https://github.com/rotationalio/ensign/blob/main/pkg/quarterdeck/keygen/keygen.go

(You can use it directly if you'd like, but copying it into a secrets utils is fine; I'd like them decoupled so that we can change the way that Quarterdeck issues api keys; e.g. either use the keygen package directly or port it to utils so quarterdeck does not have a dependency on utils).

I'll create a story to fix GDS.

bbengfort

Thank you for making those changes; I was confused about one thing but then it should be ready to go!

bbengfort · 2023-02-17T22:55:07Z

pkg/tenant/api/v1/api.go

-	ConfirmToken string `json:"confirm_token,omitempty"`
+	ID    string `json:"id,omitempty"`
+	Name  string `json:"name,omitempty"`
+	Token string `json:"token,omitempty"`


I agree this is a bit nicer.

bbengfort · 2023-02-17T22:57:49Z

pkg/tenant/topics.go

 		// Create a short-lived confirmation token in the database
-		topic.ConfirmDeleteToken = secrets.CreateToken(16)
-		topic.ConfirmDeleteExpire = time.Now().Add(5 * time.Minute)
+		if topic.ConfirmDeleteToken, err = db.NewResourceToken(topic.OrgID); err != nil {


Why is the resource token ID the topic's OrgID? I was expecting it to be the topic ID?

bbengfort · 2023-02-17T22:59:02Z

pkg/tenant/topics.go

-		log.Warn().Err(err).Msg("confirmation token has expired")
-		c.JSON(http.StatusBadRequest, api.ErrorResponse("confirmation token has expired"))
+	// Verify that we have the right token
+	if token.ID.Compare(topic.ID) != 0 {


Am I missing something? It looks like the OrgID is put in but then it is compared to the topic ID. If I am not missing anything why aren't the tests catching this?

My bad, it should be the topic ID and the test coverage isn't complete yet so I didn't catch it.

No worries - that's what code reviews are for!

pdeziel added 3 commits February 16, 2023 14:04

delete topic ensign integration

ea1cd31

add comment

72e6b83

add name to struct

b0af3f7

Merge branch 'main' into sc-13773

887e99f

pdeziel commented Feb 16, 2023

View reviewed changes

pdeziel requested a review from bbengfort February 16, 2023 20:37

pdeziel and others added 3 commits February 17, 2023 07:54

Merge branch 'main' into sc-13773

1e9ade2

fix lint error

ce205f9

change response code

7c47555

bbengfort reviewed Feb 17, 2023

View reviewed changes

bbengfort and others added 5 commits February 17, 2023 10:44

Merge branch 'main' into sc-13773

6932458

Merge branch 'main' into sc-13773

084afbc

Merge branch 'main' into sc-13773

3de3406

review comments

2905c27

Merge branch 'sc-13773' of github.com:rotationalio/ensign into sc-13773

da3d0f7

pdeziel requested a review from bbengfort February 17, 2023 22:11

bbengfort approved these changes Feb 17, 2023

View reviewed changes

wrong ID

64f6bdf

pdeziel merged commit 8c13097 into main Feb 20, 2023

pdeziel deleted the sc-13773 branch February 20, 2023 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TopicDelete Ensign integration #223

TopicDelete Ensign integration #223

pdeziel commented Feb 16, 2023

shortcut-integration bot commented Feb 16, 2023

pdeziel Feb 16, 2023

bbengfort Feb 17, 2023

pdeziel Feb 17, 2023

pdeziel Feb 16, 2023

bbengfort left a comment

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

pdeziel Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

pdeziel Feb 17, 2023

bbengfort Feb 17, 2023

pdeziel Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort left a comment

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

bbengfort Feb 17, 2023

pdeziel Feb 17, 2023

bbengfort Feb 17, 2023

		ConfirmDeleteToken string `msgpack:"confirm_delete_token"`
		ConfirmDeleteExpire time.Time `msgpack:"confirm_delete_expire"`

	c.JSON(http.StatusBadRequest, api.ErrorResponse("could not parse topic id"))
	c.JSON(http.StatusNotFound, api.ErrorResponse("topic not found"))

	c.JSON(http.StatusForbidden, api.ErrorResponse("user is not authorized to delete this topic"))
	c.JSON(http.StatusNotFound, api.ErrorResponse("topic not found"))

	c.JSON(http.StatusBadRequest, api.ErrorResponse("unrecognized confirmation token"))
	c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("unrecognized confirmation token"))

TopicDelete Ensign integration #223

TopicDelete Ensign integration #223

Conversation

pdeziel commented Feb 16, 2023

Scope of changes

Type of change

Acceptance criteria

Author checklist

Reviewer(s) checklist

shortcut-integration bot commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbengfort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbengfort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment