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

Added parameter to Yubico for specifying custom validation URLs. #18

Merged
merged 2 commits into from May 21, 2013

Conversation

dainnilsson
Copy link
Contributor

I noticed that the only way to specify validation server URLs was a rather ugly way of overwriting the constant where they are stored. This pull request adds an optional parameter, api_urls, which can be passed to Yubico to set the API URLs used by that instance.

@@ -76,12 +76,13 @@

class Yubico(object):
def __init__(self, client_id, key=None, use_https=True, verify_cert=True,
translate_otp=True):
translate_otp=True, api_urls=None):
Copy link
Owner

Choose a reason for hiding this comment

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

I would change API_URLS variable name to DEFAULT_API_URLS and then use a default argument value here - api_urls=DEFAULT_API_URLS.

@Kami
Copy link
Owner

Kami commented May 17, 2013

It would also be nice to add a test case for this functionality.

Besides this and a couple of other minor things I have mentioned, the change looks good to me. Thanks.

@dainnilsson
Copy link
Contributor Author

Initially I was weary of adding any changes that may break backwards-compatibility, but since you're ok with the the change in general I'll look over the comments and send an updated pull request in response to your comments!

@dainnilsson
Copy link
Contributor Author

There, I've added a check to ensure that the URLs are a list or tuple. I also added support for a string in case there is only a single server. It seemed to make more sense to only generate the URL list once now, so I've replaced generate_request_urls with _init_request_urls, which is run during construction, and the use_https flag is discarded afterwards. I've also added a TestCase for the new behavior.

@Kami Kami merged commit f593257 into Kami:master May 21, 2013
@Kami
Copy link
Owner

Kami commented May 21, 2013

Thanks :)

I've fixed the pep8 issue and merged your pull request into master. I will publish new version with this change to PyPi soon.

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