Feedback/review

[bhearsum]

(@jbuck asked me to have a look at this, it seems like an issue is the easiest way to comment on the overall repo)

There's a couple of things that I definitely think need fixing:

• I see you're already pinning versions in https://github.com/mozilla-services/bouncer-admin/blob/566d1020d0d0d84c9d2379ddc47d70861eab4637/requirements.txt (great!). It would be a good idea to specify hashes, too, as this is a critical service, and we want to do everything we can to make sure no rogue code gets into the builds. hashin can help you easily add them to the file.
• It looks like SECRET_KEY is always set to dev (https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/__init__.py#L9). I imagine this is intended to be overridden in production, but it would be safer to set that default in the else branch a bit further down.
• https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L141 is unsafe. You shouldn't execute code that you got from a client. Using JSON instead of x-www-form-urlencoded would fix this, as you could safely pass JSON objects and decode them into strings/lists/dicts.

And there's some other questions/comments. Whether or not you address them is up to you and @jbuck as far as I'm concerned:

• I see from

  bouncer-admin/nazgul/__init__.py

  Line 13 in 566d102

  app.config.from_pyfile("config.py", silent=True)

  that Nazgul supports a config.py. Could you provide an sample/example one?
• This repo should probably have https://pyup.io/ set-up to ensure dependencies do not get out of date.
• data.sql appears to only contain a schema. It may be better called schema.sql? You could also consider using alembic to manage it, but it's probably overkill here.
• It would be good to log errors in spots like https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L37 where you're eating exceptions. Without them being logged, it can be extremely difficult to debug.
• A 500 status code would be more appropriate in any place where you have an "Unknown error". Returning a 400 is a client error, and is usually reserved for when you know the client has made a mistake (for example, if they pass the wrong type for an argument).
• Using autocommit is probably a bad idea, because it will commit after each statement. Doing this means that any API call that updates/inserts/deletes multiple rows could possibly commit only part of the request, which may cause an invalid or unknown state in the database. You should use transactions to ensure that each request is all or nothing. https://docs.sqlalchemy.org/en/13/orm/session_transaction.html
• https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L230 always returns a 400 status code, but it also appears to return data? Is that intended. It feels like a 200 would be better here.
• Is there any way to run this locally? docker run complains about MySQL not being available.
• I see the API is using xml responses. Is this required for backwards compatibility, or was it chosen for some other reason? If the latter, JSON would probably be a better choice.
• Similarly, it would be more convenient/modern to use JSON for POST data. Uusing request.form means that clients must use x-www-form-urlencoded.

[AmanuelAaron]

I see you're already pinning versions in https://github.com/mozilla-services/bouncer-admin/blob/566d1020d0d0d84c9d2379ddc47d70861eab4637/requirements.txt (great!). It would be a good idea to specify hashes, too, as this is a critical service, and we want to do everything we can to make sure no rogue code gets into the builds. hashin can help you easily add them to the file.

This was resolved in commit 603e60c

[AmanuelAaron]

It looks like SECRET_KEY is always set to dev (https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/__init__.py#L9). I imagine this is intended to be overridden in production, but it would be safer to set that default in the else branch a bit further down.

Resolved in commit 8d2e6ed

[AmanuelAaron]

https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L141 is unsafe. You shouldn't execute code that you got from a client. Using JSON instead of x-www-form-urlencoded would fix this, as you could safely pass JSON objects and decode them into strings/lists/dicts.

Resolved in commit a97dbb0

[AmanuelAaron]

A 500 status code would be more appropriate in any place where you have an "Unknown error". Returning a 400 is a client error, and is usually reserved for when you know the client has made a mistake (for example, if they pass the wrong type for an argument).

Resolved in commit 3cbc4c9

[AmanuelAaron]

I see the API is using xml responses. Is this required for backwards compatibility, or was it chosen for some other reason? If the latter, JSON would probably be a better choice.

XML responses were done for backwards compatibility.

[AmanuelAaron]

Similarly, it would be more convenient/modern to use JSON for POST data. Uusing request.form means that clients must use x-www-form-urlencoded.

This was for backwards compatibility.

[AmanuelAaron]

https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L230 always returns a 400 status code, but it also appears to return data? Is that intended. It feels like a 200 would be better here.

The original API returns a 400 error code. The message and errno value in the XML file change depending on the type of error. It could be changed to a 200 but I'm not sure whether bouncer script expects a 400 or not.

[AmanuelAaron]

Is there any way to run this locally? docker run complains about MySQL not being available.

Resolved in PR #27

[AmanuelAaron]

It would be good to log errors in spots like https://github.com/mozilla-services/bouncer-admin/blob/master/nazgul/api.py#L37 where you're eating exceptions. Without them being logged, it can be extremely difficult to debug.

Resolved in PR #35