-
Notifications
You must be signed in to change notification settings - Fork 4
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
fist pass on api key crud operations #25
Conversation
|
||
message GetAPIKeysResponse { | ||
// The list of API keys in ascending ids order | ||
repeated temporal.api.cloud.identity.v1.APIKey api_keys = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the list the user won't know the owner of each of the apikeys, right?
Maybe we should add identity type and id to the APIKey message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right let me add owner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
message CreateAPIKeyRequest { | ||
// The id of the identity to create the API key for | ||
string identity_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider moving the identity_id and identity_type to the Spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. and changed name to "owner_id" and "owner_type"
// Get an API key | ||
rpc GetAPIKey (GetAPIKeyRequest) returns (GetAPIKeyResponse) { | ||
option (google.api.http) = { | ||
get: "/api/v1/cloud/api-keys/{api_key_id}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get: "/api/v1/cloud/api-keys/{api_key_id}", | |
get: "/api/v1/cloud/api-keys/{key_id}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The param name should be the same as the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -234,3 +234,80 @@ message GetRegionResponse { | |||
// The temporal cloud region. | |||
temporal.api.cloud.region.v1.Region region = 1; | |||
} | |||
|
|||
message GetAPIKeysRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message GetAPIKeysRequest { | |
message GetApiKeysRequest { |
We're not making other acronyms like Mtls
all-upper
string resource_version = 2; | ||
// The API key specification | ||
APIKeySpec spec = 3; | ||
// The current state of the API key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not using proper enumerates, may need to define in docs the string literals expected here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed.
|
||
message APIKey { | ||
// The id of the API Key | ||
string key_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string key_id = 1; | |
string id = 1; |
Other entities are not prefixing something to the ID here
// The id of the API Key created | ||
string key_id = 1; | ||
// The secret of the API Key created | ||
string secret = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should call this token
to make it clearer that this is the API key.
// The id of the owner the api key belongs to | ||
string owner_id = 1; | ||
// The type of the owner the api key belongs to | ||
string owner_type = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since owner is not update-able, I think we should put the owner_id
and owner_type
into the APIKey message and then allow it as optional parameters into CreateAPIKey
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked about this offline, we will keep the owner_id and owner_type in the Spec to keep consistent with the the other apis, where all user provided info is in the spec, irrespective of their mutability.
} | ||
|
||
message GetApiKeysResponse { | ||
// The list of API keys in ascending ids order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] "ascending ids order" --> "ascending id order"
} | ||
|
||
message CreateApiKeyRequest { | ||
// The spec for the API key to invite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the API key to invite" --> "the API key to create" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
message CreateApiKeyResponse { | ||
// The id of the API Key created | ||
string key_id = 1; | ||
// The token of the API Key created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is sensitive/secret, should we prefix this or indicate that on the comment somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
// The new API key specification | ||
temporal.api.cloud.identity.v1.ApiKeySpec spec = 2; | ||
// The version of the API key for which this update is intended for | ||
// The latest version can be found in the GetAPIKey operation response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] Does the casing of this 'GetAPIKey' reference in the comment match the actual operation name casing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
message ApiKeySpec { | ||
// The id of the owner to create the API key for | ||
string owner_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mark the owner_id, owner_type and expiry as immutable in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
message ApiKeySpec { | ||
// The id of the owner to create the API key for | ||
string owner_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should mention that the id here will be the id of the service account when the type is service account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comments.
// The id of the owner to create the API key for | ||
string owner_id = 1; | ||
// The type of the owner to create the API key for | ||
// Possible values: user, service-account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only talk about the service-account for now. We can later add user to the list when we officially support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind, we need when listing/getting apikeys of users.
What was changed
Fist pass on api key crud operations. Note that we these apis should accept generic principal ids and types.
This is a first pass to get ideas out there--will meditate on it and reference the other service account pr as well.
Why?
for control ops automation requirement
Checklist
Closes
How was this tested: