-
Notifications
You must be signed in to change notification settings - Fork 48
Feature/Tenant Token: Add a module which can generate tenant tokens #299
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
Conversation
This will help to test the generate_tenant_token feature, without this I will need to implement the decode by hand, and test it.
According to the spec meilisearch/specifications#89
dc51ece to
65279dd
Compare
|
bors try |
4a09ebd to
f8061fc
Compare
| end | ||
|
|
||
| def retrieve_valid_key!(*keys) | ||
| key = keys.compact.find { |k| !k.empty? } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you checking the given key is an already existing key in the search engine?
the list of the keys is only retrievable if you pass the master key. What if the users want to use generate_tenant_token but don't instanciate the Client with the master key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you checking the given key is an already existing key in the search engine?
Actually not, thegenerate_tenant_tokenmethod does not make an external request.
What I tried to achieve with this method, is to enable an optional behavior:
If the user wants to generate a token with a particular key, they can: client.generate_tenant_token(rules, api_key: new_key).
Otherwise, we will use the "default" key client.generate_tenant_token(rules), which means, we will use the @api_key defined when the user instantiated the client: client = MeiliSearch::Client.new(url, "masterKey") (we will use the "masterKey") then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should validate the keys using the client.keys, because like you said, it is possible that the user instantiates the client with a key without the superpowers. And if they do that, we'll have a problem handling it, will not be clear to the user what is the "correct behavior".
And there are other possible situations if we validate the key using the keys from the server:
- We will have to raise an error if the user uses the
masterKeyin the method, when theclientis instantiated with the masterKey, ok, we will know that, but when the client was instantiated with another key we will not be able to know which is themasterKeythe user used to start the server. - And if the core changes which key is allowed to create tokens, suppose that only keys with the action "keys.create" is allowed to do that, we know need to introduce a change in every SDK fixing this behavior the SDK will be tightly coupled with the business rules in the core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry we're talking about this earlier and I completely forgot that it was only possible to check the other keys with the master key.
alallema
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the return from @curquiza, Every thing looks good to me! 💪
| end | ||
|
|
||
| def retrieve_valid_key!(*keys) | ||
| key = keys.compact.find { |k| !k.empty? } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry we're talking about this earlier and I completely forgot that it was only possible to check the other keys with the master key.
|
@alallema I've added a use case to test the UTC thing as you mentioned in the Dart SDK! |
Create the
generate_tenant_tokenfollowing the specification meilisearch/specifications#89