Closed Bug 1352511 Opened 7 years ago Closed 7 years ago

Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate.

Categories

(Conduit :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davidwalsh, Assigned: davidwalsh)

Details

Attachments

(1 file)

We should use the same headers as bugzilla for authentication.
- add header to commitindex request
- add parameter to commitindex POST function to take API key
- pass key to review service (needs new parameter)
- commitindex calls validate function in bugzilla adapter
- return 4XX if API key is invalid
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128188

This is more of an f?  This is how I've tested so far:

```
$ pip install httpie

$ echo '{ "commits":["1","2"] }' | http --json POST http://commitindex:8888/iterations/

{
    "detail": "Missing header parameter 'X-Bugzilla-API-Key'",
    "status": 400,
    "title": "Bad Request",
    "type": "about:blank"
}


$ echo '{ "commits":["1","2"] }' | http --json POST http://commitindex:8888/iterations/ X-Bugzilla-API-Key:blahblah

{
    "detail": "Missing header parameter 'X-Bugzilla-Login'",
    "status": 400,
    "title": "Bad Request",
    "type": "about:blank"
}

$ echo '{ "commits":["1","2"] }' | http --json POST http://commitindex:8888/iterations/ X-Bugzilla-API-Key:blahblah X-Bugzilla-Login:blahblah

Traceback (most recent call last):
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1994, in __call__
    return self.wsgi_app(environ, start_response)
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1985, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1540, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.5/site-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/decorator.py", line 118, in wrapper
    response_container = function(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/validation.py", line 122, in wrapper
    response = function(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/validation.py", line 294, in wrapper
    return function(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/produces.py", line 100, in wrapper
    response = function(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/decorator.py", line 98, in wrapper
    operation_handler_result = function(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/connexion/decorators/parameter.py", line 161, in wrapper
    return function(*args, **kwargs)
  File "/app/commitindex/api/iterations.py", line 38, in post
    request.headers['X-Bugzilla-API-Key']
  File "/app/commitindex/reviews/bugzilla.py", line 129, in valid_api_key
    'GET', '/valid_login', data=params, api_key=quote(api_key)
  File "/app/commitindex/reviews/bugzilla.py", line 62, in call
    self.rest_url + path, params=data, headers=headers
  File "/usr/local/lib/python3.5/site-packages/requests/sessions.py", line 501, in get
    return self.request('GET', url, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/requests/sessions.py", line 488, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/local/lib/python3.5/site-packages/requests/sessions.py", line 609, in send
    r = adapter.send(request, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/requests/adapters.py", line 487, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPConnectionPool(host='mountebank', port=4000): Max retries exceeded with url: /valid_login?login=blahblah (Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x7f29ae0b60b8>: Failed to establish a new connection: [Errno 111] Connection refused',))
```

Everything appears to work except for the obvious death in the last command. So...

1.  Any feedback appreciated
2.  Am I missing any part of the card?
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128568

Looks like the right approach to me, see feedback below.

::: commitindex/commitindex/api/iterations.py:37
(Diff revision 1)
> +        request.headers['X-Bugzilla-Login'],
> +        request.headers['X-Bugzilla-API-Key']

What happens if one of these headers isn't present? Do we end up throwing an exception? We should make sure lack of headers has an expected error, rather than a 500 or something.

::: commitindex/commitindex/api/iterations.py:40
(Diff revision 1)
> +    bugzilla = get_bugzilla_client()
> +    if not bugzilla.valid_api_key(
> +        request.headers['X-Bugzilla-Login'],
> +        request.headers['X-Bugzilla-API-Key']
> +    ):
> +        return 'API key is invalid', 400

These need to take the error format we specify in the swagger.yml, using "problem()" creates the proper response.

::: commitindex/commitindex/api/iterations.py:40
(Diff revision 1)
> +    bugzilla = get_bugzilla_client()
> +    if not bugzilla.valid_api_key(
> +        request.headers['X-Bugzilla-Login'],
> +        request.headers['X-Bugzilla-API-Key']
> +    ):
> +        return 'API key is invalid', 400

This should be a 401 should it not?

::: commitindex/commitindex/reviews/bugzilla.py:185
(Diff revision 1)
> +def get_bugzilla_client():
> +    return Bugzilla(rest_url=current_app.config['BUGZILLA_URL'])

This bugzilla module really shouldn't know about flask, could you revert this change of fetching the url?

::: commitindex/commitindex/spec/swagger.yml:65
(Diff revision 1)
>        responses:
>          200:
>            description: OK
>            schema:
>              type: object
>              properties:
>                data:
>                  $ref: '#/definitions/Iteration'
>          default:
>            description: Unexpected error
>            schema:
>              allOf:
>                - $ref: '#/definitions/Error'

An explicit error response for the 400 case should probably be added.
Attachment #8853494 - Flags: review?(smacleod) → review-
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128568

> What happens if one of these headers isn't present? Do we end up throwing an exception? We should make sure lack of headers has an expected error, rather than a 500 or something.

I've defined these as `required` in the `swagger.yml` file, so if they aren't provided, the user will see `Missing header parameter 'X-Bugzilla-API-Key'` or `Missing header parameter 'X-Bugzilla-Login'`.  Dropping as you may have missed that in the `swagger.yml` file.
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128980

::: commitindex/commitindex/api/iterations.py:36
(Diff revision 2)
>      # TODO: Validate the passed commits form a single linear DAG line.
>      # TODO: Create and persist a real iteration.
>  
> +    bugzilla = get_bugzilla_client()
> +    if not bugzilla.valid_api_key(
> +        request.headers['X-Bugzilla-Login'],

I think this code will cause a HTTP 500 if the X-Bugzilla keys are missing?  We probably want to return either a HTTP 400 (malformed) or HTTP 401 (not authorized).

There should be a unit test for missing headers, headers with invalid keys, and headers with OK keys (that last one might already be covered by other tests).

::: commitindex/commitindex/api/iterations.py:49
(Diff revision 2)
>      topic = data.get('topic', 1)
>  
>      # TODO: Topic lookup and validation
>  
>      # Trigger review creation for this iteration.
> -    trigger_review(data['commits'])
> +    trigger_review(data['commits'], request.header['X-Bugzilla-API-Key'])

This looks like a refactoring opportunity.  If the `X-Bugzilla` headers are hard-coded in multiple places in this file and in the bugzilla client module, the header strings should probably be turned into a const in the bugzilla client module itself.

Can be in a follow-up commit.
Attachment #8853494 - Flags: review-
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128980

> This looks like a refactoring opportunity.  If the `X-Bugzilla` headers are hard-coded in multiple places in this file and in the bugzilla client module, the header strings should probably be turned into a const in the bugzilla client module itself.
> 
> Can be in a follow-up commit.

FWIW, I like the way the requests library API uses simple lowercase names for things like this (e.g. `content_type`, [status codes](http://docs.python-requests.org/en/master/api/#requests.codes))
Comment on attachment 8853494 [details]
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511)

https://reviewboard.mozilla.org/r/125570/#review128990

Dropped an issue, r+ with possible refactoring in a follow-up commit.
Attachment #8853494 - Flags: review- → review+
Pushed by mcote@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/970b35b04ac0
commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. r=smacleod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e72fe39d9f95 -d 970b35b04ac0: rebasing 136:e72fe39d9f95 "commitindex: Iterations endpoint POST accepts bugzilla api key and user in headers to authenticate. (Bug 1352511) r=mars" (tip)
merging commitindex/commitindex/reviews/bugzilla.py
merging commitindex/commitindex/reviews/triggers.py
warning: conflicts while merging commitindex/commitindex/reviews/triggers.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Attachment #8853494 - Flags: review?(smacleod)
Attachment #8853494 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: