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): Refine keys api #2438

Merged
merged 18 commits into from
Jun 2, 2022
Merged

feat(auth): Refine keys api #2438

merged 18 commits into from
Jun 2, 2022

Conversation

ManyTheFish
Copy link
Member

@ManyTheFish ManyTheFish commented May 30, 2022 •
edited
Loading

waiting for #2410 and #2444 to be merged.

fix #2369

@ManyTheFish ManyTheFish mentioned this pull request May 30, 2022
Closed
4 tasks
@ManyTheFish ManyTheFish requested review from MarinPostma and Kerollmops and removed request for MarinPostma May 31, 2022 10:09
@ManyTheFish ManyTheFish marked this pull request as ready for review May 31, 2022 10:10
@ManyTheFish
Copy link
Member Author

ManyTheFish commented May 31, 2022 •
edited
Loading

@MarinPostma I can remove DUMPS_GET action in this PR if you want, or do you prefer to do it in another one?

@MarinPostma
Copy link
Contributor

No it's ok we'll do it in another or to keep things tidy

@MarinPostma
Copy link
Contributor

We need to handle it in dumps

@ManyTheFish
Copy link
Member Author

Ok so let's review!

@Kerollmops
Copy link
Member

Hey @mdubus,

Now that we changed the API key system, we cannot use the master key as the key provided to the mini-dashboard, instead, we should ask the user to provide the admin key and not the search key as the dashboard needs to access the list of indexes.

Could you please change the text message in the modal and ask for help from @ManyTheFish to find a good name and text?
Thank you!

Kerollmops
Kerollmops requested changes May 31, 2022
View reviewed changes
meilisearch-auth/Cargo.toml Outdated Show resolved Hide resolved
meilisearch-auth/src/dump.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/dump.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/error.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/error.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-http/src/extractors/authentication/mod.rs Outdated Show resolved Hide resolved
meilisearch-http/tests/auth/api_keys.rs Outdated Show resolved Hide resolved
meilisearch-http/tests/auth/api_keys.rs Outdated Show resolved Hide resolved
meilisearch-http/tests/auth/api_keys.rs Outdated Show resolved Hide resolved
MarinPostma
MarinPostma suggested changes May 31, 2022
View reviewed changes
meilisearch-auth/src/dump.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/store.rs Outdated
Comment on lines 238 to 242
pub fn generate_key(uid: &[u8], master_key: &[u8]) -> String {
let key = [uid, master_key].concat();
let sha = Sha256::digest(&key);
format!("{:x}", sha)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So you decided not to use specialized KDF?

Copy link
Member Author

Choose a reason for hiding this comment

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

moved in #2449

meilisearch-auth/src/store.rs Outdated Show resolved Hide resolved
@ManyTheFish ManyTheFish mentioned this pull request May 31, 2022
Merged
2 tasks
@ManyTheFish
Copy link
Member Author

@MarinPostma @Kerollmops, could you re-review my PR please?

MarinPostma
MarinPostma previously approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@MarinPostma MarinPostma left a comment

Choose a reason for hiding this comment

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

I assume that you're improving the key derivation in #2449?

Kerollmops
Kerollmops requested changes Jun 1, 2022
View reviewed changes
meilisearch-auth/Cargo.toml Outdated Show resolved Hide resolved
meilisearch-auth/src/key.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/store.rs Outdated Show resolved Hide resolved
meilisearch-auth/src/store.rs Outdated Show resolved Hide resolved
meilisearch-http/src/extractors/authentication/mod.rs Outdated Show resolved Hide resolved
meilisearch-http/tests/auth/api_keys.rs Outdated Show resolved Hide resolved
@ManyTheFish ManyTheFish requested a review from Kerollmops June 1, 2022 16:06
@ManyTheFish
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 1, 2022

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@ManyTheFish ManyTheFish added this to the v0.28.0 milestone Jun 2, 2022
@ManyTheFish ManyTheFish added the breaking change The related changes are breaking for the users label Jun 2, 2022
@ManyTheFish
Copy link
Member Author

bors merge

bors bot added a commit that referenced this pull request Jun 2, 2022
@bors @ManyTheFish
Merge #2438
6b1ed05 
2438: Refine keys api r=ManyTheFish a=ManyTheFish

waiting for #2410 and #2444 to be merged.

fix #2369 

Co-authored-by: ManyTheFish <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 2, 2022

Build failed:

@ManyTheFish
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 2, 2022

Build succeeded:

@bors bors bot merged commit 08d72e3 into main Jun 2, 2022
@bors bors bot deleted the refine-keys-api branch June 2, 2022 08:53
@ManyTheFish ManyTheFish changed the title Refine keys api feat(auth): Refine keys api Jun 7, 2022
@curquiza curquiza added the v0.28.0 PRs/issues solved in v0.28.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmourier gmourier gmourier left review comments

@Kerollmops Kerollmops Kerollmops approved these changes

@MarinPostma MarinPostma MarinPostma left review comments

Assignees
No one assigned
Labels
breaking change The related changes are breaking for the users v0.28.0 PRs/issues solved in v0.28.0
Projects
None yet
Milestone
v0.28.0
Development

Successfully merging this pull request may close these issues.

Change of the API key resource
5 participants
@ManyTheFish @MarinPostma @Kerollmops @gmourier @curquiza