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

enhance(api): OAuth bearer token authentication #9021

Closed

Conversation

Johann150
Copy link
Contributor

What

  • Allow OAuth 2.0 Bearer Token Authentication according to RFC 6750 § 2.1 to be used alternatively to the body parameter i.
  • Adjust the client to use this new method, also for GET requests.
  • Handle authentication errors in the streaming API properly.
  • Adjust the generated OpenAPI specification to document GET requests and the new authentication method.
    • Also properly document that authentication is possible for all API endpoints, even if it is not required. Authenticating on such an endpoint may provide more data.

Why

Towards implementing OAuth 2.0 (#8262), but does not solve the issue yet.

Additional info

Using both authentication methods at the same time is not allowed:

Clients MUST NOT use more than one method to transmit the token in each request.
-- https://datatracker.ietf.org/doc/html/rfc6750#section-2

@Johann150 Johann150 added 🧩API Interface between server and client packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR labels Jul 19, 2022
@@ -43,7 +43,8 @@ export default (endpoint: IEndpoint, ctx: Koa.Context) => new Promise<void>((res
};

// Authentication
authenticate(body['i']).then(([user, app]) => {
// for GET requests, do not even pass on the body parameter as it is considered unsafe
authenticate(ctx.headers.authorization, ctx.method === 'GET' ? null : body['i']).then(([user, app]) => {
// API invoking
call(endpoint.name, user, app, body, ctx).then((res: any) => {
if (ctx.method === 'GET' && endpoint.meta.cacheSec && !body['i'] && !user) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appがGETでBearer認証した時にキャッシュしないために
&& !ctx.headers.authorization && !app とかが必要?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, requests that have an Authorization field are not cacheable. See also RFC 7234 § 3.2 and RFC 7235 § 4.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

通常キャッシュはしないのはわかるけど、あえてキャッシュ可能とサーバーから返しちゃうのは違う感だわ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

まあ、本来ここで考慮しといた方が良かった問題ではあるけどだわ
58e83f8

Copy link
Contributor

@mei23 mei23 Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

現状の実装は間違っていないし今の実装を否定するつもりもないけど、
不適切な実装や、不適切なユーザーでの挙動の上書き をある程度考慮してあげた方が親切と思うのだわ。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found additional specs.

From my understanding, requests that have an Authorization field are not cacheable. See also RFC 7234 § 3.2 and RFC 7235 § 4.2.

This behavior seems to be overridden by the Cache-Control: public directive.
https://datatracker.ietf.org/doc/html/rfc7234#section-3
https://datatracker.ietf.org/doc/html/rfc7234#section-5.2.2.5

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#directives

public
:
Responses for requests with Authorization header fields must not be stored in a shared cache;
however, the public directive will cause such responses to be stored in a shared cache.

So, it seems that if you use the Authorization header, you should not make it public.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this was already the case. (Of course I cannot say if it is still the case.)

For example here, the code first checks that there is no authenticated users before setting public:

if (request.method === 'GET' && endpoint.meta.cacheSec && !body?.['i'] && !user) {
reply.header('Cache-Control', `public, max-age=${endpoint.meta.cacheSec}`);
}

I think even the check !body?.['i'] is not really necessary too? Because if the token is valid, the check for !user will be equivalent, and if it is not valid, it will always be an error.

Copy link
Contributor

@mei23 mei23 Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, user always exists on successful authentication (even with appToken authentication). So I think you should be fine without !body?.['i'].

@Johann150
Copy link
Contributor Author

There are many merge conflicts and I do not have time or motivation to refactor to the new code layout.

@Johann150 Johann150 closed this Dec 4, 2022
@Johann150 Johann150 deleted the mk/bearer-authentication branch December 4, 2022 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧩API Interface between server and client packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants