-
Notifications
You must be signed in to change notification settings - Fork 226
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
Operation level authentication and scopes #2901
Conversation
@microsoft-github-policy-service agree |
2f3c62e
to
4b32f52
Compare
❌ There is undocummented changes. Run The following packages have changes but are not documented.
|
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/2901/ Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/prs/2901/ |
8417dc8
to
9297247
Compare
9297247
to
29fa3de
Compare
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.
Hi @susliko this starts to look pretty good, I think I have a bit of feedback on the new resolve api and what I think might look better. Let me know what you think.
I am trying to review your deviation of the approved design with our team but I am optimistic we should be able to get that approved and merged for the next(march) release.
4768936
to
49dbb07
Compare
0561528
to
2aea817
Compare
2aea817
to
8304b8f
Compare
Resolves microsoft#2624 Major changes: - `@useAuth` can now be used both at interface and operation levels - OAuth2 scopes can be overriden at operation level
8304b8f
to
6ef7da0
Compare
Hi! 🖖🏻 This PR resolves microsoft#2624 by implementing the [design doc](https://gist.github.com/timotheeguerin/56690786e61a436710dd647de9febc0f), but in its initial form: - `@useAuth` can now be applied not only to service namespace, but to interfaces and operations as well. Its arguments override all authentication, which was set for enclosing scopes. - OAuth2 scopes can now be set at operation level (though, the code doing this in OpenAPI emitter is a bit clunky). - New `NoAuth` authentication option allows to declare optional authentication (`NoAuth | AnyOtherAuth`) or override authentication to none in nested scopes. This implementation does not introduce new `@authScopes` decorator as design doc comments suggest, and here's why: 1. It does not compose well with `@useAuth` at operation level. For example ``` ... @useAuth(BasicAuth) @authScopes(MyOauth2, ["read"]) op gogo(): void ``` Should that be equivalent to `BasicAuth | MyOauth2`, or to `[BasicAuth, MyOauth2]`? 2. Introducing new decorator would increase complexity, but (imho) it would not reduce the amount of boilerplate: ``` alias MyOAuth2 = OAuth2Auth<{ ... }>; @useAuth(MyOAuth2) @authAcopes(MyOauth2, ["read"]) @service namepsace Foo; ``` vs ``` model MyOAuth2Flow<T extends string[]> { ... }; alias MyOauth2<T extends string[]> = Oauth2Auth<[MyOauth2Flow[T]]> @useAuth(MyOAuth2<["read"]>) @service namepsace Foo ``` I would be happy to hear any feedback and apply suggested changes. And thanks for a convenient development setup and thorough test coverage! --------- Co-authored-by: Timothee Guerin <timothee.guerin@outlook.com>
Hi! 🖖🏻
This PR resolves #2624 by implementing the design doc, but in its initial form:
@useAuth
can now be applied not only to service namespace, but to interfaces and operations as well. Its arguments override all authentication, which was set for enclosing scopes.NoAuth
authentication option allows to declare optional authentication (NoAuth | AnyOtherAuth
) or override authentication to none in nested scopes.This implementation does not introduce new
@authScopes
decorator as design doc comments suggest, and here's why:@useAuth
at operation level. For exampleShould that be equivalent to
BasicAuth | MyOauth2
, or to[BasicAuth, MyOauth2]
?vs
I would be happy to hear any feedback and apply suggested changes.
And thanks for a convenient development setup and thorough test coverage!