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

Allow to override credential lookup. #5

Merged
merged 4 commits into from
Jul 21, 2015
Merged

Conversation

felipeota
Copy link
Contributor

No description provided.

@kumar303
Copy link
Owner

Hi. Thanks for the patch. What you added looks useful for programmatic use but for regular Django usage it would be easier if one could configure a lookup function using a setting. For example:

HAWK_CREDENTIALS_LOOKUP = 'my_app.utils.hawk_credentials_lookup'

Can you try adding that? Actually, another person asked for this feature too.

You could implement it using Django's import_string.

@felipeota
Copy link
Contributor Author

Ok. I've implemented it. Although I prefer the other way around because it allows to make a self contained module without needing to modify the settings file. It also makes it possible to use hawk in different apis inside the same app.

@felipeota felipeota closed this Jul 17, 2015
@felipeota felipeota reopened this Jul 17, 2015
from django.utils.module_loading import import_string
except ImportError:
# compatibility with django < 1.7
from django.utils.module_loading import import_by_path
Copy link
Owner

Choose a reason for hiding this comment

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

nice! Thanks for adding that

@kumar303
Copy link
Owner

Looks great, I just made some minor stylistic comments. Do you have time to add a patch to the docs showing how this setting can be used? If not, I can file an issue to add that later. The new setting would need a mention underneath the HAWK_CREDENTIALS example. Here are instructions for how to build the docs locally while in development.

@kumar303
Copy link
Owner

Although I prefer the other way around because it allows to make a self contained module without needing to modify the settings file. It also makes it possible to use hawk in different apis inside the same app.

Good point! Maybe you could expand on this patch where the Django setting can be used to set a lookup function but also where one could subclass the HawkAuthentication class for lower level usage?

@felipeota
Copy link
Contributor Author

Alright, I added the aesthetic changes and the documentation.
I will open a different pull request for the subclass way of extending lookup.

@kumar303
Copy link
Owner

Awesome. I like the example in the docs. Thanks again.

kumar303 added a commit that referenced this pull request Jul 21, 2015
Allow to override credential lookup.
@kumar303 kumar303 merged commit c72bafd into kumar303:master Jul 21, 2015
@kumar303
Copy link
Owner

I pushed a release with your feature to PyPI: https://pypi.python.org/pypi/hawkrest This new setting will be super helpful. By the way, if you wanted to follow up with another patch that added in your class approach (so that one could subclass it) then I think that would be fine.

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

Successfully merging this pull request may close these issues.

None yet

2 participants