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

Convert http references to https #3998

Merged
merged 9 commits into from
Apr 11, 2018
Merged

Conversation

anselmbradford
Copy link
Member

I found a number of URLs in the codebase that are listed as http URLs but are accessible at https URLs, so updated those.

Changes

  • Update http:// to https:// URLs in codebase.

Testing

  1. gulp test:acceptance should pass.
  2. gulp test:unit should pass.
  3. gulp audit:perf shouldn't show a redundant protocol http://https:// URL.

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

@sebworks sebworks Apr 11, 2018

Choose a reason for hiding this comment

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

This shouldn't be in the commit. I'll add it the folder to the git ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! Removed it. Can we send those into the /test/ directory too?

chosak
chosak previously requested changes Apr 11, 2018
Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

See platform#2469; it looks like this PR at least partially addresses that.

@@ -114,7 +114,7 @@
"note": "",
"updated": "2017-10-10",
"name": "download_files",
"json_value": "{\"2017-03\":{\"percent_30_60\":{\"County\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/CountyMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"CountyMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"40 KB\"},\"MetroArea\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/MetroAreaMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"MetroAreaMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"148 KB\"},\"State\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/StateMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"StateMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"25 KB\"}},\"thru_month\":\"March 2017\",\"percent_90\":{\"County\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/CountyMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"CountyMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"40 KB\"},\"MetroArea\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/MetroAreaMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"MetroAreaMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"148 KB\"},\"State\":{\"url\":\"http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/StateMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"StateMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"25 KB\"}},\"pub_date\":\"October 2017\"}}"
"json_value": "{\"2017-03\":{\"percent_30_60\":{\"County\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/CountyMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"CountyMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"40 KB\"},\"MetroArea\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/MetroAreaMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"MetroAreaMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"148 KB\"},\"State\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/StateMortgagesPercent-30-89DaysLate-thru-2017-03.csv\",\"slug\":\"StateMortgagesPercent-30-89DaysLate-thru-2017-03\",\"size\":\"25 KB\"}},\"thru_month\":\"March 2017\",\"percent_90\":{\"County\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/CountyMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"CountyMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"40 KB\"},\"MetroArea\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/MetroAreaMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"MetroAreaMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"148 KB\"},\"State\":{\"url\":\"https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/StateMortgagesPercent-90-plusDaysLate-thru-2017-03.csv\",\"slug\":\"StateMortgagesPercent-90-plusDaysLate-thru-2017-03\",\"size\":\"25 KB\"}},\"pub_date\":\"October 2017\"}}"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@anselmbradford anselmbradford Apr 11, 2018

Choose a reason for hiding this comment

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

Fixed in 6f01018

@@ -18,7 +18,7 @@
S3_SECRET = os.getenv('AWS_S3_SECRET_ACCESS_KEY')
BASE_BUCKET = settings.AWS_STORAGE_BUCKET_NAME
MORTGAGE_SUB_BUCKET = "data/mortgage-performance"
PUBLIC_ACCESS_BASE = 'http://{}.s3.amazonaws.com/{}'.format(
PUBLIC_ACCESS_BASE = 'https://{}.s3.amazonaws.com/{}'.format(
Copy link
Member

Choose a reason for hiding this comment

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

See above; the HTTPS format for S3 would be something like

'https://{}/{}'.format(BASE_BUCKET, MORTGAGE_SUB_BUCKET)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is as you have written okay, or does it need testing to verify? Fixed in 6f01018

Copy link
Member

Choose a reason for hiding this comment

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

We could add a new test but it'd just be testing that strings are appended correctly. I tested this manually on your branch:

$ convert_http_to_https* $ ALLOWED_HOSTS='["*"]' DJANGO_SETTINGS_MODULE=cfgov.settings.production cfgov/manage.py shell
>>> from data_research.mortgage_utilities.s3_utils import *
>>> PUBLIC_ACCESS_BASE, S3_SOURCE_BUCKET
(u'https://files.consumerfinance.gov/data/mortgage-performance', u'https://files.consumerfinance.gov/data/mortgage-performance/source')

This looks correct to me.

@@ -117,7 +117,7 @@ def export_downloadable_csv(geo_type, late_value):
Each CSV is to start with a National row for comparison.

CSVs are posted at
http://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/ # noqa: E501
https://files.consumerfinance.gov.s3.amazonaws.com/data/mortgage-performance/downloads/ # noqa: E501
Copy link
Member

Choose a reason for hiding this comment

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

See above; remove s3.amazonaws.com.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 6f01018

@@ -1,32 +1,31 @@

$(function() {
/* this allows us to pass in HTML tags to autocomplete. Without this they get escaped
/* this allows us to pass in HTML tags to autocomplete. Without this they get escaped
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove extraneous whitespace changes to reduce the size of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can view it without whitespace changes by adding ?w=1 to the PR URL, see https://blog.github.com/2011-10-21-github-secrets/#whitespace

Does this work for you?

@@ -201,7 +201,7 @@ const sourceCookies = {
*/
parseOrganicSearch: function( host, query, referrer ) {
// Referrer must not be the same host
if ( hostsAreEqual( 'http://' + host, referrer ) === false ) {
if ( hostsAreEqual( 'https://' + host, referrer ) === false ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a file we should be modifying?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -1,5 +1,12 @@
import $ from 'jquery';
import 'rangeslider.js';
import {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above; would you consider removing extraneous whitespace changes to reduce the scope of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! #3991 appears to have snuck in here. I'll see if I can get that one reviewed and rebase here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the non-whitespace diff URL https://github.com/cfpb/cfgov-refresh/pull/3998/files?w=1

@@ -2,15 +2,15 @@

### Akamai

We use [Akamai](https://www.akamai.com/), a content delivery network, to cache the entirety of [www.consumerfinance.gov](https://www.consumerfinance.gov/) (but not our development servers). We invalidate any given page in Wagtail when it is published or unpublished (by hooking up the custom class [`AkamaiBackend`](https://github.com/cfpb/cfgov-refresh/blob/master/cfgov/v1/models/akamai_backend.py) to [Wagtail's frontend cache invalidator](http://docs.wagtail.io/en/v1.9/reference/contrib/frontendcache.html). By default, we clear the Akamai cache any time we deploy.
We use [Akamai](https://www.akamai.com/), a content delivery network, to cache the entirety of [www.consumerfinance.gov](https://www.consumerfinance.gov/) (but not our development servers). We invalidate any given page in Wagtail when it is published or unpublished (by hooking up the custom class [`AkamaiBackend`](https://github.com/cfpb/cfgov-refresh/blob/master/cfgov/v1/models/akamai_backend.py) to [Wagtail's frontend cache invalidator](http://docs.wagtail.io/en/v2.0.1/reference/contrib/frontendcache.html). By default, we clear the Akamai cache any time we deploy.
Copy link
Member

Choose a reason for hiding this comment

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

It makes me sad that docs.wagtail.io can't be HTTPS; this looks to be a ReadTheDocs thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

:(

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to commit these files?

refresh-data.sh Outdated
@@ -42,7 +42,7 @@ if [[ -z "$refresh_dump_name" ]]; then

Or you can define the location of a dump and this script will download it for you:

export CFGOV_PROD_DB_LOCATION=http://some-bucket.s3.amazonaws.com/wherever/production_django.sql.gz
export CFGOV_PROD_DB_LOCATION=https://some-bucket.s3.amazonaws.com/wherever/production_django.sql.gz
Copy link
Member

Choose a reason for hiding this comment

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

This won't work with HTTPS if you include the s3.amazonaws.com (see above).

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 225af8f

TERMS.md Outdated
co-created with Jonathan Neal, is licensed under the MIT License.
- [Slick](http://kenwheeler.github.io/slick/) by Ken Wheeler is licensed under
Copy link
Member

Choose a reason for hiding this comment

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

If you're cleaning up this reference to Slick (which I guess is no longer used), can we also remove this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

refresh-data.sh Outdated
@@ -42,7 +42,7 @@ if [[ -z "$refresh_dump_name" ]]; then

Or you can define the location of a dump and this script will download it for you:

export CFGOV_PROD_DB_LOCATION=http://some-bucket.s3.amazonaws.com/wherever/production_django.sql.gz
export CFGOV_PROD_DB_LOCATION=https://some-bucket.com/wherever/production_django.sql.gz
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're removing references to our explicit use of S3, how would you feel about removing the word "bucket" here? Since in theory the production dumps could live anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to example.com

@chosak chosak dismissed their stale review April 11, 2018 19:27

feedback

Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

Thanks for responding to my feedback @anselmbradford. This generally looks good to me, as there isn't much that would impact backend code besides the one change to the mortgage utilities code.

@anselmbradford anselmbradford merged commit 582f0ac into master Apr 11, 2018
@anselmbradford anselmbradford deleted the convert_http_to_https branch April 11, 2018 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants