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

Default bucket salt #2647

Merged
merged 14 commits into from
Dec 21, 2020
Merged

Default bucket salt #2647

merged 14 commits into from
Dec 21, 2020

Conversation

feiming
Copy link
Contributor

@feiming feiming commented Oct 31, 2020

Fixes #1535 and #881

  • Add documentation.
  • Add tests.
  • Add a changelog entry.
  • Add your name in the contributors file.
  • If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs
  • If you added a new configuration setting, update the kinto.tpl file with it.

@feiming
Copy link
Contributor Author

feiming commented Oct 31, 2020

please add hacktoberfest-accepted label to this PR

@Natim
Copy link
Member

Natim commented Oct 31, 2020

Thank you for you work on this !

@Natim Natim self-requested a review October 31, 2020 08:58
Copy link
Member

@Natim Natim left a comment

Choose a reason for hiding this comment

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

Do you mind adding a line in the documentation next to userid_hmac_secret (see docs/configuration/settings.rst)
I wonder if to be consistent we should call the config default_bucket_hmac_secret rather than default_bucket_id_salt

tests/plugins/test_default_bucket.py Outdated Show resolved Hide resolved
kinto/plugins/default_bucket/__init__.py Outdated Show resolved Hide resolved
@feiming
Copy link
Contributor Author

feiming commented Oct 31, 2020

@Natim I've renamed all default_bucket_ID_salt to default_bucket_hmac_secret

@feiming feiming requested a review from Natim November 1, 2020 01:41
kinto/config/__init__.py Outdated Show resolved Hide resolved
kinto/core/utils.py Outdated Show resolved Hide resolved
@Natim
Copy link
Member

Natim commented Nov 2, 2020

I would add a test to see that when the default_bucket_hmac secret is not defined it uses the user_hmac one and that when it is defined it doesn't.

docs/configuration/settings.rst Outdated Show resolved Hide resolved
@feiming
Copy link
Contributor Author

feiming commented Nov 2, 2020

I would add a test to see that when the default_bucket_hmac secret is not defined it uses the user_hmac one and that when it is defined it doesn't.

I'm not too sure how to test it. Does something like this work?

    def test_default_bucket_hmac_secret_define(self):
        settings = {"default_bucket_hmac_secret":"bucket_id_salt","userid_hmac_secret":"secret"}
        secret = settings.get("default_bucket_hmac_secret", settings["userid_hmac_secret"])

        self.assertEqual(secret,"bucket_id_salt")

    def test_default_bucket_hmac_secret_not_define_fallback_to_userid_hmac_secret(self):
        settings = {"userid_hmac_secret":"super_secret"}
        secret = settings.get("default_bucket_hmac_secret", settings["userid_hmac_secret"])

        self.assertEqual(secret,"super_secret")

or something like this

    def test_default_bucket_hmac_secret_define(self):
        settings = self.get_app_settings(
            {"default_bucket_hmac_secret": "bucket_id_salt", "userid_hmac_secret": "secret"}
        )
        secret = settings.get("default_bucket_hmac_secret", settings["userid_hmac_secret"])

        self.assertEqual(secret, "bucket_id_salt")

    def test_default_bucket_hmac_secret_not_define_fallback_to_userid_hmac_secret(self):
        settings = self.get_app_settings({"userid_hmac_secret": "secret"})
        if "default_bucket_hmac_secret" in settings:
            del settings["default_bucket_hmac_secret"]
        secret = settings.get("default_bucket_hmac_secret", settings["userid_hmac_secret"])

        self.assertEqual(secret, "secret")

@leplatrem
Copy link
Contributor

Maybe you could test that hmac_digest() is called with a value or another?

@feiming
Copy link
Contributor Author

feiming commented Nov 4, 2020

I had to mock hmac_secret because the default hmac_secret is an empty string. I also had to delete default_bucket_hmac_secret to test if it's not defined.

I hope this satisfy the test case.

@feiming feiming requested a review from Natim November 4, 2020 14:35
@feiming
Copy link
Contributor Author

feiming commented Nov 13, 2020

@Natim I've added the test

I would add a test to see that when the default_bucket_hmac secret is not defined it uses the user_hmac one and that when it is defined it doesn't.

@Natim I've added the test

@Natim Natim merged commit fa70183 into Kinto:master Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to change the default_bucket_id generator
3 participants