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

Fix crumb issue #195

Merged
merged 3 commits into from
Jan 14, 2022
Merged

Fix crumb issue #195

merged 3 commits into from
Jan 14, 2022

Conversation

martinda
Copy link
Collaborator

@martinda martinda commented Dec 5, 2021

Background

This fixes the crumb issue #169

The Jenkins documentation states that:

  • Clients that authenticate using a username:password must request a crumb.
  • Requests authenticating with an API token are exempt from CSRF protection in Jenkins (no crumb needed).

Furthermore, the Cloudbees documentation states that:

  • Anonymous access must request a crumb.

Anonymous access is rare but possible. It happens when users start Jenkins without the wizard and without configuring users.

Solution description

The main difficulty faced when addressing this issue is that the current jenkins-rest implementation does not clearly distinguishes between username:password and username:apiToken. The only way I could solve it was by introducing a the new JenkinsClient.builder().apiToken("username:apiToken") API.

Note that jenkins-rest has a Bearer Token implementation, but Jenkins supports no such thing. This PR removes the Bearer Token implementation in favor of what Jenkins supports: Anonymous Authentication and Basic Authentication. Basic Authentication is used for both username:password and username:apiToken.

Testing

There are three cases to test:

  1. Anonymous access
  2. Username and password
  3. Username and API Token

I have tested all cases on a local live server, however providing unit and integration tests requires more work.

I am not sure how to provide tests for 1. and 3. The docker container used for Live tests is preconfigured using username:password which means we're currently only testing for case 2.

It is possible to obtain an API Token using the REST API, so conceivably a test can be written for case 3. I just haven't gotten to that part yet.

Copy link
Owner

@cdancy cdancy left a comment

Choose a reason for hiding this comment

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

LGTM. @martinda feel free to merge when ready.

@martinda
Copy link
Collaborator Author

martinda commented Dec 7, 2021

I have some ideas on how to test this. I would need to add the API for getting a token, which might be useful in other circumstances. The anonymous mode should be testable as well since the live container allows anonymous read access, but I am not there yet.

@martinda
Copy link
Collaborator Author

I added mockTests, but I still don't have live tests. I prefer to have live tests before merging.

@martinda
Copy link
Collaborator Author

Just to say that I found how to write the live integration test. Anonymous access is only testable when the Authorization strategy is "anyone can do anything". The live tests still pass when the test server in docker is configured as such. But the strange part is that the crumb for the anonymous user is only needed when POSTing.

One might question: why bother with anonymous? The answer is that it's very useful to pre-configure Jenkins using anonymous access without adding an "artificial" account that does not exist on the production server where the user database resides in LDAP or other external system.

Anyway, just wanted to say the solution is found, now it's about implementing it.

@martinda
Copy link
Collaborator Author

martinda commented Jan 2, 2022

As part of testing the crumb feature, I needed to dynamically switch from username:password to username:apiToken and witness that the built-in filter stopped using the crumb. In order to do that in the live tests with the dynamically created docker container, I decided to create a new api: the UserApi. This lets me generate a new token, use it for testing, and then revoke it.

@martinda
Copy link
Collaborator Author

martinda commented Jan 3, 2022

@cdancy This is ready for a review. Lots of major changes internally.

@cdancy cdancy merged commit 7bed241 into cdancy:master Jan 14, 2022
@cdancy cdancy added this to the v1.0.0 milestone Jan 14, 2022
@martinda martinda mentioned this pull request Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants