-
Notifications
You must be signed in to change notification settings - Fork 160
Resource and invocation authentication definition #643
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
Resource and invocation authentication definition #643
Conversation
Signed-off-by: Charles d'Avernas <[email protected]>
@ricardozanini I closed the messed up PR and opened this one instead ;) |
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.
Many thanks for this, man! Great work, just a few questions if you don't mind.
specification.md
Outdated
| invocation | References an auth definition to be used to invoke the operation | string | no | | ||
|
||
The `authRef` property references a name of a defined workflow [auth definition](#Auth-Definition). | ||
It can be a string, in which case the referenced [auth definition](#Auth-Definition) is used solely for the function's invocation, or an object, in which case it is possible to specify an [auth definition](#Auth-Definition) to use for the function's resource retrieval (as defined by the `operation` property) and another for its invocation. |
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.
WDYT about breaking this paragraph into two sentences to make it easier to read?
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.
Good point! On it!
specification.md
Outdated
invocation: My OIDC Auth | ||
``` | ||
|
||
Note that if multiple functions share the same `operation` value, and if one of them defines an [auth definition](#Auth-Definition) for resource access, then it should always be used to access said resource. |
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 here we should explicitly say that it's only the file part of the string, before the operation id. Because "operation value" might lead to misunderstandings.
specification.md
Outdated
| resource | References an auth definition to be used to access the resource defined in the operation parameter | string | yes | | ||
| invocation | References an auth definition to be used to invoke the operation | string | no | | ||
|
||
The `authRef` property references a name of a defined workflow [auth definition](#Auth-Definition). |
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 this authDef
will override the security scheme defined in the openapi spec? I imagine we will also reuse it for the rest
invocation? 🤔
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.
Well, that is a true subject indeed. OpenAPI is the only function type that can define and describe properly an useable auth component (even if AsyncAPI does too, its IMO not good enough at that point to be considered), and should probably deserve a small explicative note on how to proceed if auth has been defined in a function's document.
I would suggest that, when invoking an OpenAPI function, if the describing document defines auth, then it should be prioritized over the one described by SW. If it does not (like in legacy docs, where auth was not fully implemented), then the auth defined in SW should be used.
WDYT?
I imagine we will also reuse it for the rest invocation
That was my idea, yes.
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.
Agreed! I think the external spec should be prioritized in this case.
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.
@ricardozanini Just appended a small note regarding that. Let's proceed with merging then?
Signed-off-by: Charles d'Avernas <[email protected]>
Signed-off-by: Charles d'Avernas <[email protected]>
Many thanks for submitting your Pull Request ❤️!
Please specify parts of this PR update:
Discussion or Issue link:
#612
What this PR does / why we need it:
Adds the possibility to configure the
authDefinition
to use for both function's resource access and function invocation.Special notes for reviewers:
As discussed in #612, using a string-based
authRef
defines theauthDefinition
to use to invoke the configured function.Also, as proposed by @ricardozanini, if the
resource
auth of a function has been defined, all other functions using the same resource (as defined by theoperation
property) must implicitly used the configured authDefinition. In other words, a resource auth definition is tightly coupled to its resource, even if it is omitted by other functions using the same resource.