Repository: pycontribs/jenkinsapi

Issue:
Connection failed with crumb requester as 403 Client Error: Forbidden 
##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report

##### Jenkinsapi VERSION
0.3.7
##### Jenkins VERSION
Jenkins ver. 2.150.1
##### SUMMARY
<!--- Explain the problem briefly -->
Looks like in the latest release the connection to a jenkins instance that has CSRF protection enabled is not working anymore.
For example by executing:
```
#!/usr/bin/env python3

from jenkinsapi.jenkins import Jenkins
from jenkinsapi.utils.crumb_requester import CrumbRequester

jenkins = Jenkins(
    'http://localhost:8080', username='test', password='test',
    requester=CrumbRequester(
        baseurl='http://localhost:8080',
        username='test',
        password='test'
    )
)

for job_name in jenkins.jobs:
    print (job_name)
```
##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Print all the jobs currently available on the jenkins instance.
In the previous version (0.3.6) the above code just worked fine.
##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->
Getting 403 forbidden when instantiating a new Jenkins object.
The stacktrace is:
```
ERROR:root:Failed request at http://localhost:8080/api/python with params: {'tree': 'jobs[name,color,url]'} jobs[name,color,url]
Traceback (most recent call last):
  File "./test.py", line 11, in <module>
    password='test'
  File "/usr/local/lib/python3.6/dist-packages/jenkinsapi/jenkins.py", line 63, in __init__
    JenkinsBase.__init__(self, baseurl, poll=not lazy)
  File "/usr/local/lib/python3.6/dist-packages/jenkinsapi/jenkinsbase.py", line 35, in __init__
    self.poll()
  File "/usr/local/lib/python3.6/dist-packages/jenkinsapi/jenkinsbase.py", line 57, in poll
    data = self._poll(tree=tree)
  File "/usr/local/lib/python3.6/dist-packages/jenkinsapi/jenkins.py", line 68, in _poll
    if not tree else tree)
  File "/usr/local/lib/python3.6/dist-packages/jenkinsapi/jenkinsbase.py", line 81, in get_data
    response.raise_for_status()
  File "/usr/lib/python3/dist-packages/requests/models.py", line 935, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: http://localhost:8080/api/python?tree=jobs%5Bname%2Ccolor%2Curl%5D
```
##### USEFUL INFORMATION
<!---
For bugs, show exactly how to reproduce the problem.
For new features, show how the feature would be used.
-->
Just run the code above using both 0.3.6 and 0.3.7 and compare output.
<!--- Paste example code and full stacktrace below -->


Discussion:
JonasVerhofste: Could someone explain to me what line [line 46](https://github.com/pycontribs/jenkinsapi/blob/master/jenkinsapi/utils/requester.py#L46) and 47 do? An error gets raised anyway, why surround it with a try-block and redefine it manually?
codecov[bot]: # [Codecov](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=h1) Report
> Merging [#682](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=desc) into [master](https://codecov.io/gh/pycontribs/jenkinsapi/commit/a87a5cd53ad0c0566836041427539a54d7fd0cbc?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100%`.

```diff
@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
- Coverage   78.46%   78.42%   -0.04%     
==========================================
  Files          34       34              
  Lines        2637     2637              
==========================================
- Hits         2069     2068       -1     
- Misses        568      569       +1
```

lechat: I think that this change will break some existing code because it implies that username ans password must be named parameters. Better code would be to leave current functionality and add check to see if unnamed parameters were not passed and only then use named ones.


JonasVerhofste: > because it implies that username ans password must be named parameters.
I don't think it does? I set the default value of the named parameter to the unnamed one created above. That's the second parameter of the get-command: it's the default value when a keyword is not found in a dictionary. Seeing as kwargs is just a dictionary with keyword-value pairs.

I just tested it and `CrumbRequester(username, password, baseurl=url)` and `CrumbRequester(username=username, password=password, baseurl=url)` both work. Could set up a test-case, should you want it.
lechat: Yes, you are right. Please make unit test and I will accept this PR. Thank you!
codecov-io: # [Codecov](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=h1) Report
> Merging [#682](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=desc) into [master](https://codecov.io/gh/pycontribs/jenkinsapi/commit/a87a5cd53ad0c0566836041427539a54d7fd0cbc?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

```diff
@@           Coverage Diff           @@
##           master     #682   +/-   ##
=======================================
  Coverage   78.46%   78.46%           
=======================================
  Files          34       34           
  Lines        2637     2637           
=======================================
  Hits         2069     2069           
  Misses        568      568
```

codecov-io: # [Codecov](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=h1) Report
> Merging [#682](https://codecov.io/gh/pycontribs/jenkinsapi/pull/682?src=pr&el=desc) into [master](https://codecov.io/gh/pycontribs/jenkinsapi/commit/a87a5cd53ad0c0566836041427539a54d7fd0cbc?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

```diff
@@           Coverage Diff           @@
##           master     #682   +/-   ##
=======================================
  Coverage   78.46%   78.46%           
=======================================
  Files          34       34           
  Lines        2637     2637           
=======================================
  Hits         2069     2069           
  Misses        568      568
```


Patch:
diff --git a/jenkinsapi/utils/requester.py b/jenkinsapi/utils/requester.py
index a51bf0a..5dd0716 100644
--- a/jenkinsapi/utils/requester.py
+++ b/jenkinsapi/utils/requester.py
@@ -40,23 +40,42 @@ class Requester(object):
 
     def __init__(self, *args, **kwargs):
 
-        if args:
-            try:
-                username, password = args
-            except ValueError as Error:
-                raise Error
-        else:
-            username = None
-            password = None
-
-        baseurl = kwargs.get('baseurl', None)
+        username = None
+        password = None
+        ssl_verify = True
+        cert = None
+        baseurl = None
+        timeout = 10
+
+        if len(args) == 1:
+            username, = args
+        elif len(args) == 2:
+            username, password = args
+        elif len(args) == 3:
+            username, password, ssl_verify = args
+        elif len(args) == 4:
+            username, password, ssl_verify, cert = args
+        elif len(args) == 5:
+            username, password, ssl_verify, cert, baseurl = args
+        elif len(args) == 6:
+            username, password, ssl_verify, cert, baseurl, timeout = args
+        elif len(args) > 6:
+            raise ValueError("To much positional arguments given!")
+
+        baseurl = kwargs.get('baseurl', baseurl)
         self.base_scheme = urlparse.urlsplit(
             baseurl).scheme if baseurl else None
-        self.username = username
-        self.password = password
-        self.ssl_verify = kwargs.get('ssl_verify', True)
-        self.cert = kwargs.get('cert', None)
-        self.timeout = kwargs.get('timeout', 10)
+        self.username = kwargs.get('username', username)
+        self.password = kwargs.get('password', password)
+        if self.username:
+            assert self.password, 'Please provide both username and password '\
+                                   'or don\'t provide them at all'
+        if self.password:
+            assert self.username, 'Please provide both username and password '\
+                                   'or don\'t provide them at all'
+        self.ssl_verify = kwargs.get('ssl_verify', ssl_verify)
+        self.cert = kwargs.get('cert', cert)
+        self.timeout = kwargs.get('timeout', timeout)
 
     def get_request_dict(
             self, params=None, data=None, files=None, headers=None, **kwargs):


Test patch:
diff --git a/jenkinsapi_tests/unittests/test_requester.py b/jenkinsapi_tests/unittests/test_requester.py
index 2f810d9..a0fe4ab 100644
--- a/jenkinsapi_tests/unittests/test_requester.py
+++ b/jenkinsapi_tests/unittests/test_requester.py
@@ -5,6 +5,115 @@ from jenkinsapi.jenkins import Requester
 from jenkinsapi.custom_exceptions import JenkinsAPIException
 
 
+def test_no_parameters_uses_default_values():
+    req = Requester()
+    assert isinstance(req, Requester)
+    assert req.username is None
+    assert req.password is None
+    assert req.ssl_verify
+    assert req.cert is None
+    assert req.base_scheme is None
+    assert req.timeout == 10
+
+
+def test_all_named_parameters():
+    req = Requester(username='foo', password='bar', ssl_verify=False,
+                    cert='foobar', baseurl='http://dummy', timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_mix_one_unnamed_named_parameters():
+    req = Requester('foo', password='bar', ssl_verify=False, cert='foobar',
+                    baseurl='http://dummy', timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_mix_two_unnamed_named_parameters():
+    req = Requester('foo', 'bar', ssl_verify=False, cert='foobar',
+                    baseurl='http://dummy', timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_mix_three_unnamed_named_parameters():
+    req = Requester('foo', 'bar', False, cert='foobar', baseurl='http://dummy',
+                    timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_mix_four_unnamed_named_parameters():
+    req = Requester('foo', 'bar', False, 'foobar', baseurl='http://dummy',
+                    timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_mix_five_unnamed_named_parameters():
+    req = Requester('foo', 'bar', False, 'foobar', 'http://dummy', timeout=5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_all_unnamed_parameters():
+    req = Requester('foo', 'bar', False, 'foobar', 'http://dummy', 5)
+    assert isinstance(req, Requester)
+    assert req.username == 'foo'
+    assert req.password == 'bar'
+    assert not req.ssl_verify
+    assert req.cert == 'foobar'
+    assert req.base_scheme == 'http', 'dummy'
+    assert req.timeout == 5
+
+
+def test_to_much_unnamed_parameters_raises_error():
+    with pytest.raises(Exception):
+        Requester('foo', 'bar', False, 'foobar', 'http://dummy', 5, 'test')
+
+
+def test_username_without_password_raises_error():
+    with pytest.raises(Exception):
+        Requester(username='foo')
+        Requester('foo')
+
+
+def test_password_without_username_raises_error():
+    with pytest.raises(AssertionError):
+        Requester(password='bar')
+
+
 def test_get_request_dict_auth():
     req = Requester('foo', 'bar')