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

Improve handling of API errors #11343

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Improve handling of API errors #11343

merged 2 commits into from
Mar 15, 2022

Conversation

nottrobin
Copy link
Contributor

@nottrobin nottrobin commented Mar 12, 2022

This is an attempt to provide a better structure for handling CVE 404 errors following my comment here:

#11305 (comment)

Basically, we ended up in a situation where we were trying to raise a 404 error from within the security_api_error error handler, but we had to do it manually with flask.render_template rather than the more proper flask.abort because you can't abort from within an active errorhandler.

I think this was pointing to a sort of code smell, that we shouldn't really have got as far as raising a SecurityAPIError if really all that happened is that a CVE is missing. In fact, I think it would be much more reasonable for SecurityAPI.get_cve to simply return None, rather than an error, if the CVE in question doesn't exist. This is reinforced by the fact that the code to turn that None response into a flask.abort(404) actually already existed in the cve view, even though it was not not being used.

So I've restructured the code to do the logic of reading the 404 code from the API's response inside the SecurityAPIError.get_cve method itself, and return None in that case. The SecurityAPIError._get method now simply does raise_for_status(), which seems reasonable as it is simply a generic HTTP request method and shouldn't have any greater knowledge than that, and then it's up to the public methods (currently get_cve and get_releases) to turn those HTTPError exceptions into SecurityAPIErrors or None as appropriate. I hope this makes sense.

To facilitate this branch I've also proposed a change to canonicalwebteam.flask-base. This branch uses that new version. A review of this branch is effectively a review of that one. Please go and approve that one once this is reviewed, but don't merge this until that PR is merged and published and this PR's requirements.txt is updated.

QA

Go to https://ubuntu-com-11343.demos.haus/security/CVE-1234-5678, see a beautiful 404 page, with a custom error message.

Go to https://ubuntu-com-11343.demos.haus/zfgzdfsd check the normal 404 page still works.

Go to https://ubuntu-com-11343.demos.haus/security/CVE-2022-23639, check a normal CVE still displays properly.

@webteam-app
Copy link

Demo starting at https://ubuntu-com-11343.demos.haus

@codecov
Copy link

codecov bot commented Mar 12, 2022

Codecov Report

Merging #11343 (634d880) into main (2124bd2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #11343   +/-   ##
=======================================
  Coverage   58.68%   58.68%           
=======================================
  Files         109      109           
  Lines        2544     2544           
  Branches      708      708           
=======================================
  Hits         1493     1493           
  Misses        990      990           
  Partials       61       61           

Copy link
Contributor

@mtruj013 mtruj013 left a comment

Choose a reason for hiding this comment

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

QA'd successfully, #56 has been merged so I believe all that's needed is to update requirements.txt :)

@mtruj013 mtruj013 merged commit e26c8df into canonical:main Mar 15, 2022
@nottrobin nottrobin deleted the cve-404s branch March 15, 2022 18:06
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.

3 participants