Make auth/token/revoke-accessor idempotent (#13661)

The auth/token/revoke will not error out if the token does not exists, it
always tries to revoke the token and return success to the client whether
or not the token exists. This makes the behavior of
auth/token/revoke-accessor coherent with this and remove the need to
check whether the token still exists.

changelog/13661.txt

@@ -0,0 +1,4 @@
+```release-note:improvement
+auth/token: The `auth/token/revoke-accessor` endpoint is now idempotent and will
+not error out if the token has already been revoked.
+```

vault/logical_system.go

@@ -670,6 +670,9 @@ func (b *SystemBackend) handleCapabilitiesAccessor(ctx context.Context, req *log
 	if err != nil {
 		return nil, err
 	}
+	if aEntry == nil {
+		return nil, &logical.StatusBadRequest{Err: "invalid accessor"}
+	}
 
 	d.Raw["token"] = aEntry.TokenID
 	return b.handleCapabilities(ctx, req, d)

vault/token_store.go

@@ -747,6 +747,9 @@ func (ts *TokenStore) tokenStoreAccessorList(ctx context.Context, req *logical.R
 			resp.AddWarning(fmt.Sprintf("Found an accessor entry that could not be successfully decoded; associated error is %q", err.Error()))
 			continue
 		}
+		if aEntry == nil {
+			continue
+		}
 
 		if aEntry.TokenID == "" {
 			resp.AddWarning(fmt.Sprintf("Found an accessor entry missing a token: %v", aEntry.AccessorID))
@@ -1741,12 +1744,12 @@ func (ts *TokenStore) handleCreateAgainstRole(ctx context.Context, req *logical.
 	return ts.handleCreateCommon(ctx, req, d, false, roleEntry)
 }
 
-func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, tainted bool) (accessorEntry, error) {
+func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, tainted bool) (*accessorEntry, error) {
 	var aEntry accessorEntry
 
 	ns, err := namespace.FromContext(ctx)
 	if err != nil {
-		return aEntry, err
+		return nil, err
 	}
 
 	lookupID := id
@@ -1755,7 +1758,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t
 		if nsID != "" {
 			accessorNS, err := NamespaceByID(ctx, nsID, ts.core)
 			if err != nil {
-				return aEntry, err
+				return nil, err
 			}
 			if accessorNS != nil {
 				if accessorNS.ID != ns.ID {
@@ -1773,16 +1776,16 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t
 
 		lookupID, err = ts.SaltID(ctx, id)
 		if err != nil {
-			return aEntry, err
+			return nil, err
 		}
 	}
 
 	entry, err := ts.accessorView(ns).Get(ctx, lookupID)
 	if err != nil {
-		return aEntry, fmt.Errorf("failed to read index using accessor: %w", err)
+		return nil, fmt.Errorf("failed to read index using accessor: %w", err)
 	}
 	if entry == nil {
-		return aEntry, &logical.StatusBadRequest{Err: "invalid accessor"}
+		return nil, nil
 	}
 
 	err = jsonutil.DecodeJSON(entry.Value, &aEntry)
@@ -1790,7 +1793,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t
 	if err != nil {
 		te, err := ts.lookupInternal(ctx, string(entry.Value), false, tainted)
 		if err != nil {
-			return accessorEntry{}, fmt.Errorf("failed to look up token using accessor index: %w", err)
+			return nil, fmt.Errorf("failed to look up token using accessor index: %w", err)
 		}
 		// It's hard to reason about what to do here if te is nil -- it may be
 		// that the token was revoked async, or that it's an old accessor index
@@ -1808,7 +1811,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t
 		aEntry.NamespaceID = namespace.RootNamespaceID
 	}
 
-	return aEntry, nil
+	return &aEntry, nil
 }
 
 // handleTidy handles the cleaning up of leaked accessor storage entries and
@@ -1959,6 +1962,10 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data
 					tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to read the accessor index: %w", err))
 					continue
 				}
+				if accessorEntry == nil {
+					tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to read the accessor index: invalid accessor"))
+					continue
+				}
 
 				// A valid accessor storage entry should always have a token ID
 				// in it. If not, it is an invalid accessor entry and needs to
@@ -2098,6 +2105,9 @@ func (ts *TokenStore) handleUpdateLookupAccessor(ctx context.Context, req *logic
 	if err != nil {
 		return nil, err
 	}
+	if aEntry == nil {
+		return nil, &logical.StatusBadRequest{Err: "invalid accessor"}
+	}
 
 	// Prepare the field data required for a lookup call
 	d := &framework.FieldData{
@@ -2140,6 +2150,9 @@ func (ts *TokenStore) handleUpdateRenewAccessor(ctx context.Context, req *logica
 	if err != nil {
 		return nil, err
 	}
+	if aEntry == nil {
+		return nil, &logical.StatusBadRequest{Err: "invalid accessor"}
+	}
 
 	// Prepare the field data required for a lookup call
 	d := &framework.FieldData{
@@ -2190,6 +2203,11 @@ func (ts *TokenStore) handleUpdateRevokeAccessor(ctx context.Context, req *logic
 	if err != nil {
 		return nil, err
 	}
+	if aEntry == nil {
+		resp := &logical.Response{}
+		resp.AddWarning("No token found with this accessor")
+		return resp, nil
+	}
 
 	te, err := ts.Lookup(ctx, aEntry.TokenID)
 	if err != nil {

vault/token_store_test.go

@@ -931,6 +931,16 @@ func TestTokenStore_HandleRequest_Renew_Revoke_Accessor(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 
+	// Revoking the token using the accessor should be idempotent since
+	// auth/token/revoke is
+	resp, err = ts.HandleRequest(namespace.RootContext(nil), req)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if len(resp.Warnings) != 1 {
+		t.Fatalf("Was expecting 1 warning, got %d", len(resp.Warnings))
+	}
+
 	time.Sleep(200 * time.Millisecond)
 
 	out, err = ts.Lookup(namespace.RootContext(nil), "tokenid")