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

Adding generate_tenant_token method #412

Merged
merged 13 commits into from
Feb 24, 2022
Merged

Conversation

alallema
Copy link
Contributor

@alallema alallema commented Feb 16, 2022

Tenant tokens

Introduction of the new method generate_tenant_token in order to facilitate the generation of the tenant token.

Related to:

@sanders41
Copy link
Collaborator

sanders41 commented Feb 21, 2022

@alallema I have been getting the same invalid API key errors when using a token. I thought I must have something wrong so didn't report it, but I see you are getting the same error. Do you think this is an issue in Meilisearch with the tokens?

Sorry after looking closer I think the issue is different. You are only getting the invalid key in one specific case.

@alallema
Copy link
Contributor Author

@alallema I have been getting the same invalid API key errors when using a token. I thought I must have something wrong so didn't report it, but I see you are getting the same error. Do you think this is an issue in Meilisearch with the tokens?

Hi @sanders41,
Yes, the reason why my tests are failing it's because I test without searchRules field. It's the expected behavior that failed, I didn't finish this test but I wanted to push it before the end of my working day.
But I totally agree with you @brunoocasali has open a discussion about it, we find the error message is not really relevant. Is really complicated to know why the generate token doesn't work if the trouble didn't come from the encoding but for example if you forgot the searchRules field or if this one is just not well-formatted.
You can see the comment and the discussion.
I don't take the time to comment it yet but I will do it soon as I can.
For your issue, my first test passed well if you want to check it. But some advice:

  • you can't use the masterKey to create a token
  • the field searchRules is mandatory
  • the field apiKeyPrefix is also mandatory with the 8 first characters of your key

@alallema
Copy link
Contributor Author

alallema commented Feb 21, 2022

Sorry after looking closer I think the issue is different. You are only getting the invalid key in one specific case.

Yes sorry I was too long to reply 😄

@sanders41
Copy link
Collaborator

Thanks this gives me my issue. I don't have the apiKeyPrefix.

@alallema alallema marked this pull request as ready for review February 22, 2022 13:10
@alallema alallema force-pushed the generate_token branch 3 times, most recently from 8ac1373 to 70b9301 Compare February 22, 2022 14:37
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Hi @alallema I left some comments about the implementation/tests, let me know what do you think! :)

"alg": "HS256"
}

api_key = str(self.config.api_key) if api_key is None else str(api_key)
Copy link
Member

Choose a reason for hiding this comment

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

You can handle empty strings? ''

Copy link
Collaborator

Choose a reason for hiding this comment

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

api_key = self.config.api_key if api_key is None else api_key

api_key is a string so no need to convert.

Also, the master key can't be used right? self.config.api_key could be the master key. There is a default search key available that can be used as a default instead.

Copy link
Contributor Author

@alallema alallema Feb 23, 2022

Choose a reason for hiding this comment

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

@sanders41, yes I didn't convert it at first but mypy wasn't agree at all with it and generate this error:

meilisearch/client.py:510: error: Value of type "Optional[str]" is not indexable
meilisearch/client.py:523: error: Item "None" of "Optional[str]" has no attribute "encode"
Found 2 errors in 1 file (checked 7 source files)

How can I fix this in a better way than cast it in string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think mypy is complaining because self.config.api_key could also be None and there isn’t a check for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can handle empty strings? ''

I can handle empty string search_rule is Union[Dict[str, Any], List[str]] not a string. I can check if it's not an empty Dict or an empty List but I'm not sure it's relevant.

token_client = meilisearch.Client(BASE_URL, token)
response = token_client.index('indexUID').search('')
assert isinstance(response, dict)
assert len(response['hits']) == 20
Copy link
Member

Choose a reason for hiding this comment

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

Actually, you don't really care about the size of the response, because it's not related to the key generation itself. But you do care if the /search request answer with a success status!

Copy link
Contributor Author

@alallema alallema Feb 23, 2022

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean. There is no status field in the search response so I just check if the response has enough results to be valid. It could for example don't have the right on my index or had filters

Copy link
Member

Choose a reason for hiding this comment

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

What I want to mean is, if your request returned just 10 records caused by a new version of the engine v0.27 for example, that's an error? this test should break because of that change? In this case no, because your test just wanted to check if your generated token can make a request successfully.

But, since the assertion is check if the returned records are equal to 20 now you have to fix this test too because the core engine changed internally.

Giving more context: this idea does not apply everywhere, because there are some use-cases we will need to handle that, ie. the tests regarding the filters or the search itself.

A different approach is to use more unit testing for some cases instead of e2e tests give a look here:

Copy link
Contributor Author

@alallema alallema Feb 23, 2022

Choose a reason for hiding this comment

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

I get it you mean if the search limit by default is modified from 20 to 10.
I can change the request with a specific limit like:

    response = token_client.index('indexUID').search('', {
        'limit': 5
    })
    assert len(response['hits']) == 5

This way I'm sure the search works well and the test will not be breaking. What do you think?
Also, I just let it in the first test when the search_rule is set to ['*']


def test_generate_tenant_token_with_search_rules(get_private_key, index_with_documents):
"""Tests create a tenant token with only search rules."""
index_with_documents()
Copy link
Member

Choose a reason for hiding this comment

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

Did you have a setup handler without documents or a simple index creation? Because it's good to avoid unnecessarily setup :D

Copy link
Contributor Author

@alallema alallema Feb 23, 2022

Choose a reason for hiding this comment

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

I had documents to check the search return at least a few documents. I find that doing a search without results was less convincing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And same as above I just let this specific verification on the first one with search_rules ['*']

meilisearch/client.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
meilisearch/client.py Show resolved Hide resolved
meilisearch/client.py Show resolved Hide resolved
"alg": "HS256"
}

api_key = str(self.config.api_key) if api_key is None else str(api_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

api_key = self.config.api_key if api_key is None else api_key

api_key is a string so no need to convert.

Also, the master key can't be used right? self.config.api_key could be the master key. There is a default search key available that can be used as a default instead.

meilisearch/client.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
meilisearch/client.py Outdated Show resolved Hide resolved
meilisearch/client.py Outdated Show resolved Hide resolved
meilisearch/client.py Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
meilisearch/client.py Outdated Show resolved Hide resolved
meilisearch/client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm

meilisearch/client.py Show resolved Hide resolved
meilisearch/client.py Show resolved Hide resolved
tests/client/test_client_tenant_token.py Outdated Show resolved Hide resolved
tests/client/test_client_tenant_token.py Show resolved Hide resolved
alallema and others added 2 commits February 23, 2022 19:01
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@alallema alallema merged commit ebc3298 into bump-meilisearch-v0.26.0 Feb 24, 2022
@alallema alallema deleted the generate_token branch February 24, 2022 09:09
@alallema alallema added the enhancement New feature or request label Mar 9, 2022
bors bot added a commit that referenced this pull request Mar 14, 2022
407: Changes related to the next Meilisearch release (v0.26.0) r=alallema a=meili-bot

Related to this issue: meilisearch/integration-guides#181

This PR:
- gathers the changes related to the next Meilisearch release (v0.26.0) so that this package is ready when the official release is out.
- should pass the tests against the [latest pre-release of Meilisearch](https://github.com/meilisearch/meilisearch/releases).
- might eventually contain test failures until the Meilisearch v0.26.0 is out.

⚠️ This PR should NOT be merged until the next release of Meilisearch (v0.26.0) is out.

_This PR is auto-generated for the [pre-release week](https://github.com/meilisearch/integration-guides/blob/master/guides/pre-release-week.md) purpose._

Done:
- #410 
- #412
- #415 

Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Co-authored-by: alallema <amelie@meilisearch.com>
Co-authored-by: Amélie <alallema@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants