Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TopicDelete Ensign integration #223

Merged
merged 13 commits into from
Feb 20, 2023
11 changes: 10 additions & 1 deletion pkg/tenant/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type TenantClient interface {
TopicList(context.Context, *PageQuery) (*TopicPage, error)
TopicDetail(ctx context.Context, id string) (*Topic, error)
TopicUpdate(context.Context, *Topic) (*Topic, error)
TopicDelete(ctx context.Context, id string) error
TopicDelete(ctx context.Context, in *Confirmation) (*Confirmation, error)
bbengfort marked this conversation as resolved.
Show resolved Hide resolved

ProjectAPIKeyList(ctx context.Context, id string, in *PageQuery) (*ProjectAPIKeyPage, error)
ProjectAPIKeyCreate(ctx context.Context, id string, in *APIKey) (*APIKey, error)
Expand All @@ -66,6 +66,14 @@ type TenantClient interface {
// Top Level Requests and Responses
//===========================================================================

// Confirmation allows APIs to protect users from unintended actions such as deleting
// data by including a confirmation token in the request.
type Confirmation struct {
ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
Token string `json:"token,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a bit nicer.

}

// Reply contains standard fields that are used for generic API responses and errors.
type Reply struct {
Success bool `json:"success"`
Expand Down Expand Up @@ -224,6 +232,7 @@ type ProjectTopicPage struct {
type Topic struct {
ID string `json:"id" uri:"id"`
Name string `json:"topic_name"`
State string `json:"state"`
Created string `json:"created,omitempty"`
Modified string `json:"modified,omitempty"`
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/tenant/api/v1/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -684,22 +684,22 @@ func (s *APIv1) TopicUpdate(ctx context.Context, in *Topic) (out *Topic, err err
return out, nil
}

func (s *APIv1) TopicDelete(ctx context.Context, id string) (err error) {
if id == "" {
return ErrTopicIDRequired
func (s *APIv1) TopicDelete(ctx context.Context, in *Confirmation) (out *Confirmation, err error) {
if in.ID == "" {
return nil, ErrTopicIDRequired
}

path := fmt.Sprintf("/v1/topics/%s", id)
path := fmt.Sprintf("/v1/topics/%s", in.ID)

// Make the HTTP request
var req *http.Request
if req, err = s.NewRequest(ctx, http.MethodDelete, path, nil, nil); err != nil {
return err
if req, err = s.NewRequest(ctx, http.MethodDelete, path, in, nil); err != nil {
return nil, err
}
if _, err = s.Do(req, nil, true); err != nil {
return err
if _, err = s.Do(req, &out, true); err != nil {
return nil, err
}
return nil
return out, nil
}

func (s *APIv1) ProjectAPIKeyList(ctx context.Context, id string, in *PageQuery) (out *ProjectAPIKeyPage, err error) {
Expand Down
11 changes: 9 additions & 2 deletions pkg/tenant/api/v1/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1073,7 +1073,10 @@ func TestTopicUpdate(t *testing.T) {
}

func TestTopicDelete(t *testing.T) {
fixture := &api.Reply{}
fixture := &api.Confirmation{
ID: "topic001",
Token: "token",
}

// Creates a test server
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Expand All @@ -1090,8 +1093,12 @@ func TestTopicDelete(t *testing.T) {
client, err := api.New(ts.URL)
require.NoError(t, err, "could not create api client")

err = client.TopicDelete(context.TODO(), "topic001")
req := &api.Confirmation{
ID: "topic001",
}
out, err := client.TopicDelete(context.TODO(), req)
require.NoError(t, err, "could not execute api request")
require.Equal(t, fixture, out, "unexpected response error")
}

func TestProjectAPIKeyList(t *testing.T) {
Expand Down
53 changes: 53 additions & 0 deletions pkg/tenant/db/tokens.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package db

import (
"encoding/base64"
"time"

"github.com/oklog/ulid/v2"
"github.com/rotationalio/ensign/pkg/quarterdeck/keygen"
"github.com/vmihailenco/msgpack/v5"
)

// NewResourceToken creates a new string token that includes a random secret and
// expires after 5 minutes.
func NewResourceToken(id ulid.ULID) (string, error) {
token := &ResourceToken{
ID: id,
Secret: keygen.Secret(),
ExpiresAt: time.Now().Add(5 * time.Minute),
}

return token.create()
}

// ResourceToken protects access to a resource by encoding an ID with a
// cryptographically secure secret and an expiration time.
type ResourceToken struct {
ID ulid.ULID `msgpack:"id"`
Secret string `msgpack:"token"`
ExpiresAt time.Time `msgpack:"expires_at"`
}

func (t *ResourceToken) IsExpired() bool {
return t.ExpiresAt.Before(time.Now())
}

func (t *ResourceToken) create() (_ string, err error) {
var data []byte
if data, err = msgpack.Marshal(t); err != nil {
return "", err
}

return base64.RawStdEncoding.EncodeToString(data), nil
}

// Decode a base64 encoded string into the struct.
func (t *ResourceToken) Decode(token string) (err error) {
var data []byte
if data, err = base64.RawStdEncoding.DecodeString(token); err != nil {
return err
}

return msgpack.Unmarshal(data, t)
}
33 changes: 33 additions & 0 deletions pkg/tenant/db/tokens_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package db_test

import (
"testing"

"github.com/rotationalio/ensign/pkg/tenant/db"
ulids "github.com/rotationalio/ensign/pkg/utils/ulid"
"github.com/stretchr/testify/require"
)

func TestResourceToken(t *testing.T) {
id := ulids.New()

// Should be able to create the token from an ID
token, err := db.NewResourceToken(id)
require.NoError(t, err, "could not create token")

// Should be able to decode the token
decoded := &db.ResourceToken{}
require.NoError(t, decoded.Decode(token), "could not decode token")
require.Equal(t, id, decoded.ID, "decoded ID does not match original ID")
require.False(t, decoded.IsExpired(), "token should not be expired")
require.NotEmpty(t, decoded.Secret, "token secret should not be empty")

// Tokens should have unique secrets
other, err := db.NewResourceToken(id)
require.NoError(t, err, "could not create token")
require.NotEqual(t, token, other, "tokens should not be equal")

otherDecoded := &db.ResourceToken{}
require.NoError(t, otherDecoded.Decode(other), "could not decode token")
require.NotEqual(t, decoded.Secret, otherDecoded.Secret, "secrets should not be equal")
}
15 changes: 9 additions & 6 deletions pkg/tenant/db/topics.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"time"

"github.com/oklog/ulid/v2"
pb "github.com/rotationalio/ensign/pkg/api/v1beta1"
"github.com/rotationalio/ensign/pkg/tenant/api/v1"
ulids "github.com/rotationalio/ensign/pkg/utils/ulid"
"github.com/vmihailenco/msgpack/v5"
Expand All @@ -13,12 +14,14 @@ import (
const TopicNamespace = "topics"

type Topic struct {
OrgID ulid.ULID `msgpack:"org_id"`
ProjectID ulid.ULID `msgpack:"project_id"`
ID ulid.ULID `msgpack:"id"`
Name string `msgpack:"name"`
Created time.Time `msgpack:"created"`
Modified time.Time `msgpack:"modified"`
OrgID ulid.ULID `msgpack:"org_id"`
ProjectID ulid.ULID `msgpack:"project_id"`
ID ulid.ULID `msgpack:"id"`
Name string `msgpack:"name"`
State pb.TopicTombstone_Status `msgpack:"state"`
ConfirmDeleteToken string `msgpack:"confirm_delete_token"`
Created time.Time `msgpack:"created"`
Modified time.Time `msgpack:"modified"`
}

var _ Model = &Topic{}
Expand Down
148 changes: 137 additions & 11 deletions pkg/tenant/topics.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package tenant

import (
"context"
"errors"
"net/http"

"github.com/gin-gonic/gin"
"github.com/oklog/ulid/v2"
pb "github.com/rotationalio/ensign/pkg/api/v1beta1"
qd "github.com/rotationalio/ensign/pkg/quarterdeck/api/v1"
middleware "github.com/rotationalio/ensign/pkg/quarterdeck/middleware"
"github.com/rotationalio/ensign/pkg/quarterdeck/tokens"
"github.com/rotationalio/ensign/pkg/tenant/api/v1"
Expand Down Expand Up @@ -257,30 +261,152 @@ func (s *Server) TopicUpdate(c *gin.Context) {
c.JSON(http.StatusOK, t.ToAPI())
}

// TopicDelete deletes a topic from a user's request with a given ID
// and returns a 200 OK response instead of an error response.
// TopicDelete completely destroys a topic, removing the metadata in Trtl and as well
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice of status code!

//
// Route: /topic/:topicID
func (s *Server) TopicDelete(c *gin.Context) {
var (
err error
err error
ctx context.Context
claims *tokens.Claims
)

// User credentials are required for Quarterdeck requests
if ctx, err = middleware.ContextFromRequest(c); err != nil {
log.Error().Err(err).Msg("could not create user context from request")
c.JSON(http.StatusUnauthorized, api.ErrorResponse("could not fetch credentials for authenticated user"))
return
}

// User claims are required to verify that the user owns the topic
if claims, err = middleware.GetClaims(c); err != nil {
log.Error().Err(err).Msg("could not fetch claims from context")
c.JSON(http.StatusUnauthorized, api.ErrorResponse("could not fetch claims from context"))
return
}

// Get the topic ID from the URL and return a 400 response
// if the topic does not exist.
// if the ID is not parseable
var topicID ulid.ULID
if topicID, err = ulid.Parse(c.Param("topicID")); err != nil {
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.StatusNotFound, api.ErrorResponse("topic not found"))
return
}

// Parse the request body for the confirmation token
confirm := &api.Confirmation{}
if err = c.BindJSON(confirm); err != nil {
log.Warn().Err(err).Msg("could not bind topic delete request")
c.JSON(http.StatusBadRequest, api.ErrorResponse("could not bind user request"))
return
}

// Sanity check that the ID in the request body matches the ID in the URL
if confirm.ID != topicID.String() {
log.Warn().Err(err).Msg("topic id in request body does not match topic id in URL")
c.JSON(http.StatusBadRequest, api.ErrorResponse("id in request body does not match id in URL"))
return
}

// Fetch the topic metadata from the database
var topic *db.Topic
if topic, err = db.RetrieveTopic(ctx, topicID); err != nil {
if errors.Is(err, db.ErrNotFound) {
log.Warn().Err(err).Str("topicID", topicID.String()).Msg("topic not found")
c.JSON(http.StatusNotFound, api.ErrorResponse("topic not found"))
return
}
log.Error().Err(err).Str("topicID", topicID.String()).Msg("could not retrieve topic")
bbengfort marked this conversation as resolved.
Show resolved Hide resolved
c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not delete topic"))
return
}

// 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.StatusNotFound, api.ErrorResponse("topic not found"))
return
}

// Send confirmation token if not provided
if confirm.Token == "" {
// Create a short-lived confirmation token in the database
if topic.ConfirmDeleteToken, err = db.NewResourceToken(topic.OrgID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log.Error().Err(err).Str("topicID", topicID.String()).Msg("could not generate confirmation token")
c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not generate confirmation token"))
return
}
if err = db.UpdateTopic(ctx, topic); err != nil {
log.Error().Err(err).Str("topicID", topicID.String()).Msg("could not save topic")
c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not generate confirmation token"))
return
}

confirm.Name = topic.Name
confirm.Token = topic.ConfirmDeleteToken
c.JSON(http.StatusOK, confirm)
return
}

// Verify the confirmation token is valid and not expired
token := &db.ResourceToken{}
if err = token.Decode(confirm.Token); err != nil {
log.Warn().Err(err).Msg("could not decode confirmation token")
c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("invalid confirmation token"))
return
}

if token.IsExpired() {
log.Warn().Msg("confirmation token has expired")
c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("invalid confirmation token"))
return
}

// Verify that we have the right token
if token.ID.Compare(topic.ID) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log.Warn().Err(err).Str("token_id", token.ID.String()).Str("topic_id", topic.ID.String()).Msg("confirmation token does not match topic")
c.JSON(http.StatusPreconditionFailed, api.ErrorResponse("invalid confirmation token"))
return
}

// Request access to the project from Quarterdeck
req := &qd.Project{
ProjectID: topic.ProjectID,
}
var rep *qd.LoginReply
if rep, err = s.quarterdeck.ProjectAccess(ctx, req); err != nil {
log.Error().Err(err).Msg("could not request one-time claims")
c.JSON(qd.ErrorStatus(err), api.ErrorResponse("could not delete topic"))
return
}

// Create the Ensign context from the one-time claims
ensignContext := qd.ContextWithToken(ctx, rep.AccessToken)

// Send the delete topic request to Ensign
deleteRequest := &pb.TopicMod{
Id: topic.ID.String(),
Operation: pb.TopicMod_DESTROY,
}
var tombstone *pb.TopicTombstone
if _, err = s.ensign.DeleteTopic(ensignContext, deleteRequest); err != nil {
log.Error().Err(err).Msg("could not delete topic in ensign")
c.JSON(qd.ErrorStatus(err), api.ErrorResponse("could not delete topic"))
return
}

// Delete the topic and return a 404 response if it cannot be removed.
if err = db.DeleteTopic(c.Request.Context(), topicID); err != nil {
log.Error().Err(err).Str("topicID", topicID.String()).Msg("could not delete topic")
c.JSON(http.StatusNotFound, api.ErrorResponse("could not delete topic"))
// 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"))
Comment on lines +403 to +407
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

return
}

c.Status(http.StatusOK)
c.Status(http.StatusAccepted)
}