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

Also support Authorization:Bearer header to specify access token #1720

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Jan 20, 2025

The access token (for privileged functionality) could so far only be passed as a parameter (that is, either as a field in the data for application/x-www-form-urlencoded or as a query parameter). With this change, the access token is also read from the Authorization header, provided it exists and the value has the form Bearer <access token>. If the access token is specified both via the Authorization header and via the access-token parameter, they must be the same. Resolves #1691.

Copy link
Member Author

@Qup42 Qup42 left a comment

Choose a reason for hiding this comment

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

Some initial comments. Mostly waiting for another PR to be merged, to avoid excessive conflicts.

test/ServerTest.cpp Outdated Show resolved Hide resolved
test/ServerTest.cpp Outdated Show resolved Hide resolved
src/engine/Server.cpp Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.97%. Comparing base (49934c0) to head (8928c4d).

Files with missing lines Patch % Lines
src/engine/Server.cpp 81.81% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1720      +/-   ##
==========================================
+ Coverage   89.94%   89.97%   +0.03%     
==========================================
  Files         395      395              
  Lines       37625    37656      +31     
  Branches     4229     4235       +6     
==========================================
+ Hits        33840    33882      +42     
+ Misses       2482     2478       -4     
+ Partials     1303     1296       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Qup42
Copy link
Member Author

Qup42 commented Jan 20, 2025

Note: The suggestions is to use a raw string literal for a string with 3 ". A raw string literal would be awkward for the rest of the error message string (".). To avoid having a raw literal string and a normal string next to each other, I have decided to not apply the suggestion.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

A first round of reviews.

src/engine/Server.cpp Outdated Show resolved Hide resolved
src/engine/Server.cpp Show resolved Hide resolved
test/ServerTest.cpp Outdated Show resolved Hide resolved
test/ServerTest.cpp Outdated Show resolved Hide resolved
src/engine/Server.cpp Outdated Show resolved Hide resolved
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Also two very minor suggestions.

src/engine/Server.cpp Outdated Show resolved Hide resolved
src/engine/Server.cpp Outdated Show resolved Hide resolved
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

1-1 with Julian, this looks great and works fine.

  1. Some minor comments
  2. It seems that "Bearer " is not correct
  3. We checked the RFC, and everything after the space after "Bearer" (or whatever the right word here is) already counts as the token, including further spaces

@hannahbast hannahbast changed the title Accept access token from Authorization header Also support Authorization: header to specify access token Jan 24, 2025
@hannahbast
Copy link
Member

@Qup42 Thanks a lot + please let me know when the minor comments are addressed, then I can merge this.

NOTE: the Bearer , which might be changed, is currently also mentioned in the description.

@Qup42
Copy link
Member Author

Qup42 commented Jan 27, 2025

For the HTTP Authorization the schemes Basic and Bearer are in consideration.

Basic (RFC 7617) uses a user and password combination that is sent base64 encoded.
It is commonly realized in browser as a popup asking for username and password.
The standard defines that the server responds with a 401 and WWW-Authenticate for protected resources. The client then asks for the credentials and sends them in another request to the resource.

Bearer (RFC 6750) uses a token that is opaque to the client using it.
It originated with OAuth 2.0 (see RFC) but nowadays is also commonly used for generic authentication.

Basic seems to be implemented in more client libraries and would support multiple users/identities (if we want that in the future).
Bearer fits well with the current state of a single access token and requires no challenge-response flow.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Thank you very much

@joka921 joka921 changed the title Also support Authorization: header to specify access token Also support Authorization:Bearer header to specify access token Jan 30, 2025
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

This looks great now and is ready to merge, thanks a lot!

@sparql-conformance
Copy link

Conformance check passed ✅

Test Status Changes 📊

Number of Tests Previous Status Current Status
38 Failed Passed

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=8928c4df06229ea4d61c8062d59e8d7cdab79153&prev=49934c0013cc1861d665ec3647e748df3b593457

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.

Accept access_token provided via header.
3 participants