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.

Sign up for GitHub

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

Jump to bottom

feat(auth): Tenant token #2096

Merged
merged 1 commit into from
Jan 27, 2022
Merged

feat(auth): Tenant token #2096

merged 1 commit into from
Jan 27, 2022

Conversation

ManyTheFish
Copy link
Member

@ManyTheFish ManyTheFish commented Jan 20, 2022 •
edited by MarinPostma

Make meilisearch support JWT authentication signed with meilisearch API keys
using HS256, HS384 or HS512 algorithms.

Related spec: specifications#89 rendered
Fix #1991

@curquiza curquiza added this to the v0.26.0 milestone Jan 20, 2022
@ManyTheFish ManyTheFish force-pushed the tenant-token branch 2 times, most recently from 67e2261 to 87a030a Compare January 25, 2022 16:56
@ManyTheFish ManyTheFish mentioned this pull request Jan 25, 2022
Closed
@ManyTheFish ManyTheFish changed the title Tenant token feat(auth): Tenant token Jan 25, 2022
MarinPostma
MarinPostma suggested changes Jan 26, 2022
View reviewed changes
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-lib/src/index/search.rs Outdated Show resolved Hide resolved
meilisearch-lib/src/index/search.rs Outdated Show resolved Hide resolved
meilisearch-lib/src/index/search.rs Outdated Show resolved Hide resolved
meilisearch-http/src/routes/api_key.rs Show resolved Hide resolved
meilisearch-http/src/extractors/authentication/mod.rs Outdated Show resolved Hide resolved
Kerollmops
Kerollmops requested changes Jan 26, 2022
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Here is half of my review as you updated the branche.

meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/lib.rs Show resolved Hide resolved
meilisearch-auth/src/lib.rs Show resolved Hide resolved
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/lib.rs Show resolved Hide resolved
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-http/src/extractors/authentication/mod.rs
@@ -50,19 +50,23 @@ impl<P: Policy + 'static, D: 'static + Clone> FromRequest for GuardedData<P, D>
Some("Bearer") => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that bearer (without the capitalization) will not be detected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only "Bearer" written with a capital is accepted, with exactly one space between it and the following key.

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm that is not http compliant: https://stackoverflow.com/a/5259004

Copy link
Member Author

Choose a reason for hiding this comment

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

@MarinPostma @Kerollmops, "Bearer" is not the Header name but a part of the Value, the header name is "Authentication".

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it's the good RFC, but, here is a pseudo regex matching the bearer auth:
The OAuth 2.0 Authorization Framework: Bearer Token Usage

Copy link
Member

Choose a reason for hiding this comment

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

If it is a real issue can you "Reference in a new issue", please? If it is not, let's merge that and pray!

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no issue here, I don't understand your point. 😞

meilisearch-http/src/extractors/authentication/mod.rs Outdated Show resolved Hide resolved
meilisearch-http/src/routes/api_key.rs Outdated Show resolved Hide resolved
@curquiza
Copy link
Member

curquiza commented Jan 26, 2022 •
edited

(I'm investigating why we have a failure with our CI on another PR)

bors try

bors bot added a commit that referenced this pull request Jan 26, 2022
@bors
Try #2096:
cc04702
@bors
Copy link
Contributor

bors bot commented Jan 26, 2022

try

Build succeeded:

@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jan 26, 2022
@bors
Try #2096:
e70e841
@bors
Copy link
Contributor

bors bot commented Jan 26, 2022

try

Build succeeded:

@ManyTheFish
feat(auth): Implement Tenant token
7ca647f 
Make meilisearch support JWT authentication signed with meilisearch API keys
using HS256, HS384 or HS512 algorithms.

Related spec: meilisearch/specifications#89
Fix #1991
@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jan 27, 2022
@bors
Try #2096:
2d41652
@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

try

Build succeeded:

MarinPostma
MarinPostma reviewed Jan 27, 2022
View reviewed changes
meilisearch-lib/src/index_controller/mod.rs
Comment on lines -546 to +547
pub async fn get_all_stats(&self, index_filter: &Option<Vec<String>>) -> Result<Stats> {
pub async fn get_all_stats(&self, search_rules: &SearchRules) -> Result<Stats> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll find a way to not depend on meilisearch auth here later, so that's ok for now

@Kerollmops
Copy link
Member

bors merge

bors bot added a commit that referenced this pull request Jan 27, 2022
@bors @ManyTheFish
Merge #2096
0f43c93 
2096: feat(auth): Tenant token r=Kerollmops a=ManyTheFish

Make meilisearch support JWT authentication signed with meilisearch API keys
using HS256, HS384 or HS512 algorithms.

Related spec: [specifications#89](meilisearch/specifications#89) [rendered](https://github.com/meilisearch/specifications/blob/scoped-api-keys/text/0089-tenant-tokens.md)
Fix #1991 


Co-authored-by: ManyTheFish <many@meilisearch.com>
@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"1 review requesting changes and 1 approving review by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

Kerollmops
Kerollmops approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

Build succeeded:

@bors bors bot merged commit 622c15e into main Jan 27, 2022
@bors bors bot deleted the tenant-token branch January 27, 2022 10:52
@curquiza curquiza added the v0.26.0 PRs/issues solved in v0.26.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kerollmops Kerollmops Kerollmops approved these changes

@MarinPostma MarinPostma MarinPostma approved these changes

Assignees
No one assigned
Labels
v0.26.0 PRs/issues solved in v0.26.0
Projects
None yet
Milestone
v0.26.0
Development

Successfully merging this pull request may close these issues.

Tenant Tokens (Multi-Tenant indexes)
4 participants
@ManyTheFish @curquiza @Kerollmops @MarinPostma