Skip to content
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

Update casing of Bearer in basic TypeSpec #2639

Closed
wants to merge 1 commit into from
Closed

Conversation

lmazuel
Copy link
Member

@lmazuel lmazuel commented Nov 7, 2023

Like @johanste was saying, it shouldn't matter, but we got services that refused bearer, and worked only on Bearer, hence suggesting this change.

Codegen just take the scheme and put it straight into the prefix of the header value, so fixing it here makes it work everywhere, as a win-win for all parties.

Copy link
Contributor

github-actions bot commented Nov 7, 2023

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/2639/

Website: https://tspwebsitepr.z22.web.core.windows.net/prs/2639/

@markcowl
Copy link
Contributor

markcowl commented Nov 7, 2023

It looks like OpenAPI3 and RFC 6750 use different casings for the scheme name. I think this change is fine, but by our current rules, we would have to run it by the breaking change board before releasing, unless this is a blocking issue.

@timotheeguerin
Copy link
Member

timotheeguerin commented Nov 7, 2023

Yeah I am not sure about this change either in openapi3 the "expected" scheme is lowercase
https://swagger.io/docs/specification/authentication/bearer-authentication/

I think it might need to be understood by the client what bearer or basic means

edit: swagger ui doesn't care of the casing it understand all casing of bearer.
I think we might want to be more specific about what that value should be.

@MaryGao
Copy link
Member

MaryGao commented Nov 13, 2023

I notice that OpenAPI can't clarify a custom scheme(see discussion) for HTTP authentication schemes and they only allow named schemes in HTTP Authentication Scheme Registry.

I was wondering

  • if this would apply for typespec;
  • if the scheme in typespec is the same as OpenAPI(one-one mapping)

If yes, we may need linters to limit the input for http auth and emitter needs to understand their mappings between the scheme and prefix. If no, typespec needs to expand the expression for the mappings.

Authorization: <PREFIX> <token>

@timotheeguerin
Copy link
Member

The discussion does say it so the same thing we do but just that tooling won’t understand that custom scheme which of course how could is
OAI/OpenAPI-Specification#1229 (comment)

@MaryGao
Copy link
Member

MaryGao commented Nov 14, 2023

we do but just that tooling won’t understand that custom scheme which of course how could is

Let me confirm that for an officially named scheme, emitters should understand the scheme and should format the parameter correctly.

For a custom http auth emitters cannot know how to prepare the parameter so it could either throw exceptions or rely on the convention Authorization: <scheme> <token>.

@useAuth({
  type: AuthType.http,
  scheme: "foo",
})

@timotheeguerin
Copy link
Member

we do but just that tooling won’t understand that custom scheme which of course how could is

Let me confirm that for an officially named scheme, emitters should understand the scheme and should format the parameter correctly.

For a custom http auth emitters cannot know how to prepare the parameter so it could either throw exceptions or rely on the convention Authorization: <scheme> <token>.

@useAuth({
  type: AuthType.http,
  scheme: "foo",
})

I think that is the status quo but we should probably discuss that in a design meeting to officialize this behavior

@MaryGao
Copy link
Member

MaryGao commented Nov 14, 2023

RFC 7235 defines the HTTP authentication framework and they ususally use the Authorization header. In typespec we could use a security scheme with type: http and scheme: {schemaName} to express HTTP authentication scheme and codegen is supposed to understand these schemes and format the parameter correctly.

Here is my proposal:

Basic Authentication

It is predefined in typespec and SDK should send Authorization header when making requests:

Authorization: Basic <token>

Bearer Authentication

It is predefined in typespec and SDK should send Authorization header when making requests:

Authorization: Bearer <token>

Other registered HTTP schemes

It is possible that service partner could refer other HTTP schemes as defined by RFC 7235 and HTTP Authentication Scheme Registry and usually the typespec sheme name is lowercase.

Codegen should also follow relevant RFC detail to prepare the parameter and support them case by case (we haven't received any of them yet).

TypeSpec Scheme Name Authentication Scheme Name Reference
digest Digest [RFC7616]
dpop DPoP [RFC9449]
hoba HOBA [RFC7486]
vapid vapid [RFC 8292]
... ... ...

Customed HTTP schemes but not registered yet

Also they could define customed schemes but not officially registered as a http scheme yet.

@useAuth({
  type: AuthType.http,
  scheme: <schemeName>,
})

Codegen could follow the below format to prepare the parameter.

Authorization: <schemeName> <token>

/cc @weidongxu-microsoft @timotheeguerin @lmazuel

@archerzz
Copy link
Member

OpenAPI scheme looks more like an enum to me. As Mary finds more schemes, I'm not sure if it's a good idea to enforce the same letter case from top all the way down to the implementation. Maybe we should delegate to TCGC or emitters to ensure the correct letter case is adopted in the generated codes.

@lmazuel
Copy link
Member Author

lmazuel commented Nov 15, 2023

Closing, TCGC or emitters will take care of it

@timotheeguerin
Copy link
Member

Filed this issue to make sure we define clearly the behavior #2672

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants