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)
Conduit
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: davidwalsh, Assigned: davidwalsh)
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
mozreview-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/#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 3•7 years ago
|
||
mozreview-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 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 hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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 6•7 years ago
|
||
mozreview-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 ::: 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 7•7 years ago
|
||
mozreview-review-reply |
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 8•7 years ago
|
||
mozreview-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/#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
Comment 10•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8853494 -
Flags: review?(smacleod)
Updated•7 years ago
|
Attachment #8853494 -
Flags: review+
You need to log in
before you can comment on or make changes to this bug.
Description
•