Rename operation ID to token #16
Conversation
| @@ -229,10 +242,15 @@ Retrieve operation result. | |||
|
|
|||
| Retrieve operation details. | |||
|
|
|||
| **Path**: `/{service}/{operation}/{operation_id}` | |||
There was a problem hiding this comment.
I do not think we should remove the token from the URL path on these. At the very least it should be a query parameter, though in the URL path makes more sense IMO. Using headers as required parameters to specific HTTP calls is strange (as opposed to general metadata like auth). Referencing different operations in a call should be different URLs IMO.
Is the only reason to remove from the URL because after discussion we expect this string to be bigger or something?
There was a problem hiding this comment.
This is similar to the callback token where we decided to not put it in the URL since URLs are typically logged in access logs and we don't want potentially sensitive information leaking there.
There was a problem hiding this comment.
If it is sensitive shouldn't it be encrypted?
There was a problem hiding this comment.
Is token meant to be more sensitive than ID for this use case? Calling a string to reference the operation an "id" or "token" doesn't really change the purpose. Using headers as required parameters for these calls to work is rough devexp I wouldn't expect from normal HTTP APIs.
There was a problem hiding this comment.
If it is sensitive shouldn't it be encrypted?
Yes, but it still doesn't stop someone from potentially stealing your token.
There was a problem hiding this comment.
My concern is this is a significant usability regression of the Nexus HTTP API. Is it possible to not go backwards in dev experience? For instance, maybe this header approach can be an alternative instead of the required way to give this parameter?
There was a problem hiding this comment.
I was just thinking the same thing. We could have two approaches, so users that are concerned with tokens leaking could send it as a header. That's essentially what the related Go SDK PR allows.
Let's keep it path based for now and rename, we can add the header approach later.
There was a problem hiding this comment.
Working on this now and feeling a bit on the fence here. Let me think about this some more.
There was a problem hiding this comment.
Pushed a change, LMK what you think.
There was a problem hiding this comment.
LGTM. Could also understand if we wanted to reserve some paths for an eventual no-token future, or I could also understand if you wanted token to be a query parameter instead of path piece so it can be more easily made optional in lieu of a header later.
bergundy
force-pushed
the
operation-token
branch
2 times, most recently
from
2feefde to
f75530b
Compare
| @@ -229,10 +242,15 @@ Retrieve operation result. | |||
|
|
|||
| Retrieve operation details. | |||
|
|
|||
| **Path**: `/{service}/{operation}/{operation_id}` | |||
There was a problem hiding this comment.
LGTM. Could also understand if we wanted to reserve some paths for an eventual no-token future, or I could also understand if you wanted token to be a query parameter instead of path piece so it can be more easily made optional in lieu of a header later.
|
Hmm.. maybe a query param is a better fit here, then the same paths could be used with either headers or queries. Yeah, let me go with that. |
bergundy
force-pushed
the
operation-token
branch
from
f75530b to
c584aed
Compare
bergundy
force-pushed
the
operation-token
branch
from
c584aed to
f772a76
Compare
- Also fix docstring for handler error types - Also mark all exported functions and structs in completion.go as experimental - those need to be redone to properly rehydrate errors See also nexus-rpc/api#16
Rename operation ID to token to clarify that it is meant to be opaque. Tokens may be encrypted, signed, and used as OTPs.
Also removed the operation ID from the request paths and accept the token as either a query param or a header.