Skip to content
This repository was archived by the owner on May 10, 2018. It is now read-only.

Conversation

@thanasisk
Copy link

added support for HTTP(S) Basic authentication. Basically, when creating a context configuration, if you specify basic, it will automatically generate the correct Authorization header, that will take the place of Spunk token authentication.

I have signed the contributor agreement.

If you see something that you do not like in this PR, let me know and I will do my best to address your comments. As this is required for production use in the day job, I would really appreciate a fast response.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the default here be False instead?

@itay
Copy link
Contributor

itay commented Aug 25, 2015

@thanasisk overall looks great. A few minor comments:

  1. Consider changing the doc comment for the new parameter.
  2. Could you add a test for this functionality? It could look something like:
    a. Create a new user with a specific username/password
    b. Create a new Service with that username/password and basic=True
    c. Make sure basic auth works.
  3. You should consider making login a no-op when this is set.

@thanasisk
Copy link
Author

Hello, thanks for the quick feedback. I have addressed your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add something like assert_true(service.apps.length() > 0) to verify that the service can access a REST endpoint?

@thanasisk
Copy link
Author

addressed - how far away we are from merging this PR?

shakeelmohamed pushed a commit that referenced this pull request Aug 27, 2015
added support for HTTP Basic Authentication, bumped version
@shakeelmohamed shakeelmohamed merged commit b7a4ac7 into splunk:develop Aug 27, 2015
@shakeelmohamed
Copy link
Contributor

Thanks @thanasisk! I've merged it in. It may take a bit for us to release the changes. If this is urgent for you, we can prioritize the release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants