[PulseGuardian] Flask-SSLify should not be required if running locally

ASSIGNED
Assigned to

Status

Webtools
Pulse
P4
normal
ASSIGNED
2 years ago
a year ago

People

(Reporter: mcote, Assigned: Rutuja, Mentored)

Tracking

Trunk

Firefox Tracking Flags

(firefox47 affected)

Details

(Whiteboard: try: from flask_sslify import SSLify except ImportError: #log.debug('flask-sslify failed to import', exc_info=True) logging.warning('flask-sslify failed to import'))

User Story

This is a mentored Pulse bug.  For general information on Pulse, see https://wiki.mozilla.org/Auto-tools/Projects/Pulse, which includes a section on Contributing.

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
The Flask-SSLify package is only used in the production environment on Heroku (when the "DYNO" environment variable is set).  It depends on pyopenssl, which requires some OS packages, so it would be a lot more convenient for contributors if it was optional.

This should be pretty simple.  You would just need a try/except ImportError statement around the flask-sslify import and log a warning.  Same thing in the part that actually calls SSLify(); catch the error and log something, then exit.

To test this, just make sure that you don't have Flask-SSLify installed in your virtualenv (nor on your system) and make sure the app launches, the warning is logged, and the app continues to work fine.

An even better test is to remove the pyopenssl package *and*, if you can, the libopenssl-dev apt package from your system.  This will identify if there are other parts of the code that also need SSL support; we can fix those too.
(Reporter)

Updated

2 years ago
Mentor: mcote
User Story: (updated)
(Assignee)

Updated

2 years ago
Whiteboard: try: from flask_sslify import SSLify except ImportError: #log.debug('flask-sslify failed to import', exc_info=True) logging.warning('flask-sslify failed to import')
(Assignee)

Comment 1

2 years ago
Created attachment 8725067 [details]
requirements.txt
(Assignee)

Comment 2

2 years ago
Created attachment 8725068 [details]
dev_requirements.txt
(Assignee)

Comment 3

2 years ago
Created attachment 8725075 [details] [diff] [review]
bug1245621.diff
(Assignee)

Comment 4

2 years ago
Created attachment 8725076 [details] [diff] [review]
bug1245621_req.diff
(Assignee)

Comment 5

2 years ago
Created attachment 8725080 [details] [diff] [review]
bug1245621_req.diff

This is the final file that includes changes to req, dev_req and web.py
(Reporter)

Updated

2 years ago
Attachment #8725067 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8725068 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8725075 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8725076 - Attachment is obsolete: true
(Reporter)

Comment 6

2 years ago
Comment on attachment 8725080 [details] [diff] [review]
bug1245621_req.diff

Review of attachment 8725080 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few little comments below.  Can you also modify README.md to include information about dev_requirements.txt and requirements.txt?

::: pulseguardian/web.py
@@ +18,4 @@
>                     redirect,
>                     request,
>                     jsonify)
> +#from flask_sslify import SSLify

Please remove this line rather than commenting it out.

@@ +22,5 @@
> +
> +try:
> +    from flask_sslify import SSLify
> +except ImportError:
> +   logging.warning('flask-sslify failed to import')

This is good, but it might be confusing to a new user.  They might wonder what it means exactly.  Can you change this text to "flask_sslify failed to import.  This is okay for local environments but will cause a failure in production."

::: requirements.txt
@@ +2,2 @@
>  Flask-SSLify==0.1.5
> +

Please remove this extra blank line.
Attachment #8725080 - Flags: review-
I don't think this is necessary any more. pyopenssl is only required on old Python versions, and if someone is using a version that old, then they should update.
You need to log in before you can comment on or make changes to this bug.