-
Notifications
You must be signed in to change notification settings - Fork 582
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
Add extension for auth context #1218
Conversation
Signed-off-by: Thomas Bouldin <[email protected]>
- Constriants | ||
- REQUIRED | ||
- SHOULD be one of the above enum values | ||
- If the enum values are not sufficient for a Producer, they SHOULD amend this spec to include the new enum value. |
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.
For folks who want to extend this w/o submitting a PR (e.g. they want to keep their values proprietary/secret/inhouse), we normally will say stuff like:
This specification defines the following values, and it is RECOMMENDED that they be used. However, implementations MAY define additional values.
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.
Sure.
## Attributes | ||
|
||
### authtype | ||
|
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.
for consistency , let's remove this blank line
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
### authclaims | ||
- Type: `String` | ||
- Description: A JSON string representing claims of the principal that triggered | ||
the event. This field MAY be omitted. |
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.
You can remove the "This field MAY be omitted" since you tagged it as optional already
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
- Type: `String` | ||
- Description: A unique identifier of the principal that triggered the occurence. This might, for example, be a unique ID in an identity database (userID), an email of a platform user or service account, or the label for an API key. | ||
- Constraints | ||
- OPTIONAL |
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 this be REQUIRED? Seems like w/o this info the extension doesn't provide too much value to the consumer. Unless there are cases in just knowing "some" app_user caused 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.
I think many clouds will not have an ID for api keys and most clouds will not want to have an ID for system changes.
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.
ah ok - thanks!
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 I just added "unauthenticated" to the enum for "authtype" so that a provider can indicate that this extension is supported but the request had no authentication. That would obviously not come with an authid either.
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.
ok can you sign your commits?
@@ -0,0 +1,55 @@ | |||
# Auth Context | |||
|
|||
This extension embeds information about the principal which triggered an occurence. This allows consumers of the |
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 you wrap all line at 80?
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
Signed-off-by: Thomas Bouldin <[email protected]>
- `system`: An obscured identity used when a cloud platform or other system | ||
service triggers an event. Examples include a database record which | ||
was deleted based on a TTL. | ||
- Constriants |
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: Contraints (sorry for not spotting this before)
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.
"before"? do we have this typo in other docs? I couldn't find 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.
I had early access to this PR ;)
Signed-off-by: Thomas Bouldin <[email protected]>
@inlined did you want to join the call next week to present this to the team or to answer any questions? |
I can try to make it. Jon is OOO until next Thursday, so I can't get details from him. Mind emailing me a calendar invite? |
On the call today more time was requested for review. Please look it over so we can vote on it next week |
@inlined can you sign all of your commits? The DCO checker isn't happy |
Signed-off-by: Thomas Bouldin <[email protected]>
Approved on the 7/27 call |
Provides an extension where an event source can annotate an event with information about the actor that triggered the occurrence. This spec is explicitly not meant to secure a CloudEvent nor provide authorization to any handler of a CloudEvent; it is merely informational. For information on how this might be used, see the pre-CloudEvents feature in the Firebase Realtime Database events for Google Cloud Functions 1st Gen. This extension aims to expand the concept in a way that is suitable beyond the narrow context of Firebase's needs and can be used by others.
Proposed Changes
Adds an extension with three attributes:
authtype
(Required). The kind of principal that was used to authorize the change captured in an occurrenceauthid
(Optional) A unique ID for the principal that authorized the change captured in an occurrence.authclaims
(Optional). Various claims held by the authorizing principal. This might include email, profile photo URLs, custom claims (e.g. isAdmin), etc.Release Note