feat: optimize API key storage with bitflags implementation #5495

CodeMan62 · 2025-04-03T19:56:32Z

Pull Request

Related issue

Fixes #5489

What does this PR do?

Optimizes API key storage using bitflags instead of individual database entries
Adds new KeyActions struct to efficiently store permissions as bitflags
Reduces database size by storing a single entry per key instead of multiple entries
Improves permission checking performance by using bitwise operations
Maintains backward compatibility with existing API key functionality

Testing Done:

Manual Testing:
- Created new API key with multiple permissions (documents.add, documents.get, indexes.get)
- Verified bitflags storage in debug logs:
```
DEBUG: Setting bit 8 for action IndexesGet
DEBUG: Setting bit 3 for action DocumentsAdd
DEBUG: Setting bit 4 for action DocumentsGet
```
- Tested permission checks:
  - ✅ Successfully added documents (documents.add)
  - ✅ Successfully retrieved documents (documents.get)
  - ✅ Successfully listed indexes (indexes.get)
  - ❌ Correctly denied document deletion (no permission)
Automated Testing:
- Ran auth crate tests: cargo test -p meilisearch-auth
- All tests passing with no regressions

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

ManyTheFish · 2025-04-07T08:43:47Z

Hey @CodeMan62,

Thank you for jumping on my issue, I wasn't expecting it to be as fast! 🏎️

The first part of the job has not been merged yet:
#4940

It may not be convenient for you to work on top of another branch so let's wait for the upper linked PR to be merged before merging yours, it should be merged this week. 😄

I quickly checked your implementation, and I think it meets the requirements, but I'll need to investigate a bit more. ☺️

Thank you again for your commitment!
And sorry for delaying a bit your work.

see you!

CodeMan62 · 2025-04-14T02:12:30Z

any updates on this @ManyTheFish

ManyTheFish · 2025-04-22T14:53:13Z

Hey @CodeMan62,
Sorry for the delay,
The work is ready: #5524.

The previous link is a reference branch containing the first part of the work, adding the bitflags to the code using the bitflags! macro.
We want to keep this work made by another contributor, but as explained in the PR, we need the database change to fully use the bitflag/mask feature.

If you are still interested in contributing to this feature, first, you should rebase your work on the key-actions-to-bitflags-mask branch.
Then:

Adapt your changes to the previous work
Adapt the authentication to compare masks and not actions
Modify this guarded data by a mask requesting DocumentsAdd and DocumentsDelete
Remove all the unnecessary All action, bitmasks should do the job.

We will merge this into the key-actions-to-bitflags-mask branch.

After that, we will have to support the version upgrade of Meilisearch, which will be the third part. 😄

Let me know if something isn't clear enough or if you have any additional questions!

See you!

CodeMan62 · 2025-04-22T15:49:30Z

@ManyTheFish hey, it will take a bit of time, but it will be done soon just give me some time

…ch#5489

CodeMan62 · 2025-04-22T18:51:50Z

Hey @ManyTheFish can you tell me that the rebase I have done is right or not, and also, do I still need to do this??

Adapt your changes to the previous work

crates/meilisearch-auth/src/store.rs

CodeMan62 · 2025-04-23T18:34:44Z

@ManyTheFish I am commiting with a error it would be helpful if you can help me in that and also there will be almost all required CI's will fail due to that

crates/meilisearch-auth/src/store.rs

CodeMan62 · 2025-04-28T16:18:47Z

@ManyTheFish can you answer my question in the requested changes so that we can do further work in this PR

CodeMan62 · 2025-04-29T18:16:15Z

@ManyTheFish can you tell me if anything remains to implement or we can move on to fix tests??

ManyTheFish · 2025-04-30T13:19:29Z

Hey @CodeMan62,
I think you've finished the first part by integrating your work:

Adapt your changes to the previous work
Adapt the authentication to compare masks and not actions
Modify this guarded databy a mask requesting DocumentsAdd and DocumentsDelete
Remove all the unnecessary All action, bitmasks should do the job.

For the second step, you'll have to refactor the get_expiration_date and is_key_authorized functions to take a bitflag as a parameter instead of an Action:

    pub fn get_expiration_date(
        &self,
        uid: Uuid,
-       action: Action,
+       bitflags: u32,
        index: Option<&str>,
    ) -> Result<Option<Option<OffsetDateTime>>> {

Compare the bitflags directly. The goal here is to be able to require multiple actions for the same route, so the bitflags passed as a parameter MUST be a subset of the stored API key's bitflags.

🗒️ note: We may want to create a type alias for our bitflags type, this would make it clearer.

🗒️ note 2: there is no is_superset/is_subset method for bitflags, using a difference should do the job as:
 /// Check if the key bitflags is a superset of the required bitflags
 fn is_key_authorized(key_flags: u32, required_flags: u32) -> bool {
   required_flags.difference(key_flags).is_empty()
 }

CodeMan62 · 2025-04-30T13:48:55Z

Hey @CodeMan62, I think you've finished the first part by integrating your work:

Adapt your changes to the previous work

Adapt the authentication to compare masks and not actions

Modify this guarded databy a mask requesting DocumentsAdd and DocumentsDelete

Remove all the unnecessary All action, bitmasks should do the job.

For the second step, you'll have to refactor the get_expiration_date and is_key_authorized functions to take a bitflag as a parameter instead of an Action:
    pub fn get_expiration_date(
        &self,
        uid: Uuid,
-       action: Action,
+       bitflags: u32,
        index: Option<&str>,
    ) -> Result<Option<Option<OffsetDateTime>>> {
Compare the bitflags directly. The goal here is to be able to require multiple actions for the same route, so the bitflags passed as a parameter MUST be a subset of the stored API key's bitflags.

got it

CodeMan62 · 2025-05-03T18:33:10Z

Converting this to a draft once the work will be done I will let you know @ManyTheFish

CodeMan62 · 2025-05-11T19:30:59Z

@ManyTheFish I think we have finished part 2 Is it done or anything left in it

ManyTheFish

Hey @CodeMan62,

Here is my feedback:

prefix_first_expiration_date should not exist anymore
the Option<Index> given to is_key_authorized is a restriction, None means there is no index restriction.
has_action function should be replaced by a bitflag comparison
get_expiration_date should be replaced by a new is_key_authorized function

This new is_key_authorized should:

check if the key_flags is a superset of the required flag, if not, return refused
check if the key authorized indexes contain the required index, if not, return refused, skip if required index is None
check if the expiration_date is not expired, if expired, return refused
if all previous checks are ok, return OK

ManyTheFish · 2025-05-12T13:04:14Z

crates/meilisearch-auth/src/store.rs

+    fn has_action(&self, bitflags: u32) -> bool {
+        for (index, action_flag) in enum_iterator::all::<Action>().enumerate() {
+            if action_flag.bits() == bitflags && (self.bitflags & (1 << index) != 0) {
+                return true;
+            }
+        }
+        false
+    }


To remove in favor of a bitflag comparison:

Suggested change

fn has_action(&self, bitflags: u32) -> bool {

for (index, action_flag) in enum_iterator::all::<Action>().enumerate() {

if action_flag.bits() == bitflags && (self.bitflags & (1 << index) != 0) {

return true;

}

}

false

}

fn is_bitflag_authorized(&self, bitflags: u32) -> bool {

bitflags.difference(self.bitflags).is_empty()

}

there is no method difference in u32

Not sure, but wouldn't bitflags & self.bitflags == 0 works?

huhh i missed it thanks

Small warning here, an intersection is not what I want, this will not ensure that the key bitflags are a superset of the required bitflags.
Only a difference should work.
The solution may reside in one of the bitflags crate types or traits.
The Flags trait has a difference method, if we implement the function as follows:

fn is_bitflag_authorized<F: Flags>(&self, bitflags: F) -> bool { bitflags.difference(self.bitflags).is_empty() }

It would work if self.bitflags implements the Flags trait as well.

I must agree on something, the crate documentation misses some hints and examples. 😞

I think then we need to import bitflags crate here if we want to use Flags trait or should I implement something like this??

It's already imported, it's used for the Actions, when doing action_flag.bits(), you are using the Flags trait.

Maybe, we should use the type Action everywhere, because it seems that it can contain "unnamed" flags that could correspond to the union of several Actions.

fn is_bitflag_authorized(&self, bitflags: Action) -> bool { // self.bitflag has type: Action bitflags.difference(self.bitflags).is_empty() }

this may work 🤔

Hey

fn is_bitflag_authorized(&self, bitflags: u32) -> bool { Action::from_bits(bitflags) .map(|flags| flags.difference(Action::from_bits(self.bitflags).unwrap_or(Action::empty())).is_empty()) .unwrap_or(false) }

I have tried this and I think this is good to go and we don't need to change type of bitflags to Action
I have pushed it tell me if it seems wrong

ManyTheFish

@CodeMan62,

I finally found the time to continue the revue,

sorry for the delay 😬

crates/meilisearch-auth/src/store.rs

ManyTheFish · 2025-05-26T14:16:10Z

crates/meilisearch-auth/src/store.rs

+    pub fn is_key_authorized(
        &self,
        uid: Uuid,
-        action: Action,
+        bitflags: u32,
        index: Option<&str>,
-    ) -> Result<Option<Option<OffsetDateTime>>> {
+    ) -> Result<AuthorizationStatus> {
        let rtxn = self.env.read_txn()?;
-        let tuple = (&uid, &action, index.map(|s| s.as_bytes()));
-        match self.action_keyid_index_expiration.get(&rtxn, &tuple)? {
-            Some(expiration) => Ok(Some(expiration)),
-            None => {
-                let tuple = (&uid, &action, None);
-                for result in self.action_keyid_index_expiration.prefix_iter(&rtxn, &tuple)? {
-                    let ((_, _, index_uid_pattern), expiration) = result?;
-                    if let Some((pattern, index)) = index_uid_pattern.zip(index) {
-                        let index_uid_pattern = str::from_utf8(pattern)?;
-                        let pattern = IndexUidPattern::from_str(index_uid_pattern)
-                            .map_err(|e| AuthControllerError::Internal(Box::new(e)))?;
-                        if pattern.matches_str(index) {
-                            return Ok(Some(expiration));
-                        }
-                    }
-                }
-                Ok(None)
+
+        // Get key info from database
+        let key_masks = match self.key_actions.get(&rtxn, uid.as_bytes())? {
+            Some(key_masks) => key_masks,
+            None => return Ok(AuthorizationStatus::Refused),
+        };
+
+        // Check if the key's bitflags contain the required bitflags
+        if !key_masks.is_bitflag_authorized(bitflags) {
+            return Ok(AuthorizationStatus::Refused);
+        }
+
+        // Check if the key is authorized for the requested index
+        if let Some(index_str) = index {
+            let index_uid = match IndexUid::try_from(String::from(index_str)) {
+                Ok(uid) => uid,
+                Err(_) => return Ok(AuthorizationStatus::Refused),
+            };
+
+            let authorized_for_index =
+                key_masks.indexes.iter().any(|pattern| pattern.matches(&index_uid));
+            if !authorized_for_index {
+                return Ok(AuthorizationStatus::Refused);
            }
        }
-    }

-    pub fn prefix_first_expiration_date(
-        &self,
-        uid: Uuid,
-        action: Action,
-    ) -> Result<Option<Option<OffsetDateTime>>> {
-        let rtxn = self.env.read_txn()?;
-        let tuple = (&uid, &action, None);
-        let exp = self
-            .action_keyid_index_expiration
-            .prefix_iter(&rtxn, &tuple)?
-            .next()
-            .transpose()?
-            .map(|(_, expiration)| expiration);
-
-        Ok(exp)
+        // Check if the key is expired
+        if let Some(expires_at) = key_masks.expires_at {
+            if expires_at < OffsetDateTime::now_utc() {
+                return Ok(AuthorizationStatus::Refused);
+            }
+        }
+
+        // All checks passed
+        Ok(AuthorizationStatus::Ok)


Perfect!
Clean code, thank you for the clarity

haha after long time someone gave compliment to my code 😄

crates/meilisearch-auth/src/lib.rs

ManyTheFish

Hey @CodeMan62,
Last review before going to the next part:

Small documentation change
can you make the test work and see if there is nothing wrong.

Example of failing test

Snapshot: delete_api_key
Source: crates/meilisearch/tests/auth/api_keys.rs:922
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0     0 │ {
    1       │-  "name": null,
    2       │-  "description": "Indexing API key",
    3       │-  "key": "[ignored]",
    4       │-  "uid": "[ignored]",
    5       │-  "actions": [
    6       │-    "search",
    7       │-    "documents.add",
    8       │-    "documents.get",
    9       │-    "documents.delete",
   10       │-    "indexes.create",
   11       │-    "indexes.get",
   12       │-    "indexes.update",
   13       │-    "indexes.delete",
   14       │-    "tasks.get",
   15       │-    "settings.get",
   16       │-    "settings.update",
   17       │-    "stats.get",
   18       │-    "dumps.create"
   19       │-  ],
   20       │-  "indexes": [
   21       │-    "products"
   22       │-  ],
   23       │-  "expiresAt": "2050-11-13T00:00:00Z",
   24       │-  "createdAt": "[ignored]",
   25       │-  "updatedAt": "[ignored]"
          1 │+  "message": "task 45 panicked with message \"an action is missing a matching serialized value\"",
          2 │+  "code": "internal",
          3 │+  "type": "internal",
          4 │+  "link": "https://docs.meilisearch.com/errors#internal"
   26     5 │ }
────────────┴───────────────────────────────────────────────────────────────────
Stopped on the first failure. Run `cargo insta test` to run all snapshots.

+              impl KeyMasks {
+                  fn is_bitflag_authorized(&self, bitflags: Action) -> bool {
+                      bitflags.difference(self.bitflags).is_empty()

feat: optimize API key storage with bitflags implementation #5495

Are you sure you want to change the base?

feat: optimize API key storage with bitflags implementation #5495

Uh oh!

Conversation

CodeMan62 commented Apr 3, 2025

Pull Request

Related issue

What does this PR do?

Testing Done:

PR checklist

Uh oh!

ManyTheFish commented Apr 7, 2025

Uh oh!

CodeMan62 commented Apr 14, 2025

Uh oh!

ManyTheFish commented Apr 22, 2025

Uh oh!

CodeMan62 commented Apr 22, 2025

Uh oh!

CodeMan62 commented Apr 22, 2025

Uh oh!

Uh oh!

Uh oh!

CodeMan62 commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CodeMan62 commented Apr 28, 2025

Uh oh!

CodeMan62 commented Apr 29, 2025

Uh oh!

ManyTheFish commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeMan62 commented Apr 30, 2025

Uh oh!

CodeMan62 commented May 3, 2025

Uh oh!

CodeMan62 commented May 11, 2025

Uh oh!

ManyTheFish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManyTheFish May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManyTheFish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManyTheFish left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Example of failing test

Next

CodeMan62 commented Apr 23, 2025 •

edited

Loading

ManyTheFish commented Apr 30, 2025 •

edited

Loading

ManyTheFish May 14, 2025 •

edited

Loading

ManyTheFish left a comment •

edited

Loading