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

Support Basic Authentication (Issue #2) #10

Merged
merged 3 commits into from
Jun 10, 2017

Conversation

mikepatrick
Copy link
Contributor

First of all, thanks for putting this out here!

New Functionality

Users may now provide a username and password in their .jenkins file, and connect to a secured Jenkins instance.

Acceptance criteria:

Existing users who connect to an unsecured Jenkins instance should be unaffected. They will not have to modify their .jenkins file, and things will continue to work as they have.

Users wishing to connect to a secured Jenkins instance can simply add username and password to their .jenkins file, as outlined in README.

Outline of changes and expected behavior

  • With these changes, a basic authorization header is always sent to Jenkins.
  • The credentials will default to empty strings, if not provided in the .jenkins file.
  • For unsecured Jenkins, the .jenkins file can contain no credentials, empty credentials, or invalid credentials. The connection should succeed in any case.
  • The AUTHENTICATION NEEDED error will be shown for both missing and incorrect credentials, when connecting to a secured Jenkins instance. This will usually be a 401, rather than 403, since an auth header is always sent.

Testing

I tested this against an unsecured Jenkins instance (v1.647 running on localhost), and a secured Jenkins instance (v2.49 running remotely). VSCode v1.12.2 was used for all testing.

One Final Thought

I considered adding a friendly reminder to the README, warning users not to accidentally check their plain-text passwords (via .jenkins file) into version control.

I went back and forth on this. You'd like to think this is common knowledge, and goes without saying, but perhaps it doesn't hurt to put out some orange safety/education pylons for the beginners in our ranks.

In the end I didn't add any reminder/warning; my reasoning was that I should defer to the maintainer's opinion on the (a) appropriateness/necessity and (b) verbiage of such a message.

Copy link

@umens umens left a comment

Choose a reason for hiding this comment

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

Looks ok to me but like I commented it should be more tested before merging
Edit : maybe just test on a secured v1 should be enough

"url": "http://127.0.0.1:8080/job/myproject"
"url": "http://127.0.0.1:8080/job/myproject",
"username": "jenkinsuser",
"password": "jenkinspassword"
Copy link

Choose a reason for hiding this comment

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

you should also specify that we can also use a token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; I'll make the change. Something like "jenkins_user" and "jenkins_password_or_token" seem reasonable?

@@ -88,7 +88,12 @@ export class Jenkins {
let result: JenkinsStatus;

request
.get(url + '/api/json')
.get(url + '/api/json', {
'auth': {
Copy link

Choose a reason for hiding this comment

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

I think we should do more tests for this because I couldn't get this work that way. But It worked my way like a I wrote (see #3). It looks like it depends on the version of Jenkins people are using.
I'm gonna check this again to be sure

Copy link

Choose a reason for hiding this comment

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

Forget this. It works also on my side (idk why it didn't work the first time). it should be ok on every version I guess. But I've you tried it on the v1.647 but secured (it could have been some changes in the api between v1 and v2) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@umens Thanks for checking this out.

I didn't test against 1.647 secured; that's a good idea. I will secure my 1.647 instance and test against it later this morning. I'll report back once I've done so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I secured my 1.647 instance and tested. All scenarios perform as expected, whether providing a password or a token.

@mikepatrick mikepatrick mentioned this pull request Jun 8, 2017
Copy link
Owner

@alefragnani alefragnani left a comment

Choose a reason for hiding this comment

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

Let the Readme stuff to me 😄 .
Thanks for the PR by the way ...

@alefragnani alefragnani merged commit 9ccca83 into alefragnani:master Jun 10, 2017
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

3 participants