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

requires_credentials circumvents any overriden credentials store #10

Open
sbv-trueenergy opened this issue Aug 19, 2019 · 3 comments
Open

Comments

@sbv-trueenergy
Copy link

Seeing as the Gigya and Kamareon classes can be initialized with a credentials variable it seems like the @requires_credentials decorator would still use the singleton CredentialsStore() to verify that right api-keys are available.

https://github.com/jamesremuscat/pyze/blob/develop/src/pyze/api/credentials.py#L19
https://github.com/jamesremuscat/pyze/blob/develop/src/pyze/api/kamereon.py#L41

We are considering using this library in a webservice but the hard dependency on a single car/login in a file-on-disk makes that a bit cumbersome when running multiple flask processes on the same machine.

Currently we would scratch the file on each request and write a new file with json in it that matches the given request - to switch between tokens/users/cars. But if multiple requests are being handled at once the file would be overriden/inconsistent if it were to happen on a single machine/container.

I don't have a straightforward idea for a fix but perhaps it could be decorator from self._credentials instead of a global one?. Or a something similar to my psedocode decorator here:

    def requires_credentials(*names):
        def _requires_credentials(func):
            def inner(*args, **kwargs):
                credentials = args[0]._credentials if args and hasattr(args[0], '_credentials') else CredentialStore()
                for name in names:
                    if name not in credentials:
                        raise MissingCredentialException(name)
                return func(*args, **kwargs)
            return inner
        return _requires_credentials
    return requires_credentials

or something more radical where the api code won't use the decorator and let the api consumers deal with the missing keys.

@jamesremuscat
Copy link
Owner

Thanks for opening an issue - you're right that a long-running webservice isn't a well-supported use case right now.

Your idea looks good - off the top of my head, you could then create your own implementation of a CredentialsStore that didn't touch the disk at all, and inject that when you create your Gigya and Kamereons.

@epenet
Copy link
Contributor

epenet commented Jun 2, 2020

Following the negative feedback on PR 61, I've created a new PR #68 which keeps the decorator and is similar to the pseudo-code from @sbv-trueenergy.
However, since requires_credentials is only ever called from Gigya, Kamereon and Vehicle objects I feel there is no need for the else CredentialStore(). I've simply had to take into account calling from within the Vehicle object (and so the added check for hasattr(args[0], '_kamereon')

@epenet
Copy link
Contributor

epenet commented Aug 20, 2020

I think this can be closed now, it's available in release v0.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants