-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
validate plotly_(api)_domain - HTTPS, not HTTP #997
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
Changes from 8 commits
a9f0c95
556ef45
6b12bf2
0c189f2
6e4622c
c063f23
fe31851
e55e81c
efc36a2
46d3cc9
2a0872c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,12 +186,14 @@ def set_config_file(plotly_domain=None, | |
:param (bool) world_readable: True = public, False = private | ||
|
||
""" | ||
# import ipdb; ipdb.set_trace() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove this commented code as it is unnecessary 🔪 : ) |
||
if not check_file_permissions(): | ||
raise exceptions.PlotlyError("You don't have proper file permissions " | ||
"to run this function.") | ||
ensure_local_plotly_files() # make sure what's there is OK | ||
utils.validate_world_readable_and_sharing_settings({ | ||
'sharing': sharing, 'world_readable': world_readable}) | ||
|
||
settings = get_config_file() | ||
if isinstance(plotly_domain, six.string_types): | ||
settings['plotly_domain'] = plotly_domain | ||
|
@@ -218,6 +220,11 @@ def set_config_file(plotly_domain=None, | |
elif auto_open is not None: | ||
raise TypeError('auto_open should be a boolean') | ||
|
||
# validate plotly_domain and plotly_api_domain | ||
utils.validate_plotly_domains( | ||
{'plotly_domain': plotly_domain, 'plotly_api_domain': plotly_api_domain} | ||
) | ||
|
||
if isinstance(world_readable, bool): | ||
settings['world_readable'] = world_readable | ||
settings.pop('sharing') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,12 @@ | |
""" | ||
from __future__ import absolute_import | ||
|
||
import decimal | ||
import os.path | ||
import re | ||
import sys | ||
import threading | ||
import decimal | ||
import warnings | ||
from collections import deque | ||
|
||
import pytz | ||
|
@@ -32,6 +33,30 @@ | |
lock = threading.Lock() | ||
|
||
|
||
http_msg = ( | ||
"The plotly_domain and plotly_api_domain of your config file must start " | ||
"with 'https', 'http'.\n If you are not using On-Prem then run the " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why include Also I would suggest using Also, this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was a typo. I meant to type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! Thanks. |
||
"following code to ensure your plotly_domain and plotly_api_domain start " | ||
"with 'https':\n\n\n" | ||
"import plotly\n" | ||
"plotly.tools.set_config_file(\n" | ||
" plotly_domain='https://plot.ly',\n" | ||
" plotly_api_domain='https://api.plot.ly'\n" | ||
")\n\n\n" | ||
"If you are using On-Prem then you will need to use your company's " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
"domain and api_domain urls:\n\n\n" | ||
"import plotly\n" | ||
"plotly.tools.set_config_file(\n" | ||
" plotly_domain='https://plotly.your-company.com',\n" | ||
" plotly_api_domain='https://plotly.your-company.com'\n" | ||
")\n\n\n" | ||
"Make sure to replace `your-company.com` with the URL of your Plotly " | ||
"On-Premise server.\nSee " | ||
"https://plot.ly/python/getting-started/#special-instructions-for-plotly-onpremise-users " | ||
"for more help with getting started with On-Prem." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
) | ||
|
||
|
||
### general file setup tools ### | ||
|
||
def load_json_dict(filename, *args): | ||
|
@@ -297,6 +322,7 @@ def encode_as_decimal(obj): | |
else: | ||
raise NotEncodable | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this line needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it keeps the file consistent with 2 spaces between functions |
||
### unicode stuff ### | ||
def decode_unicode(coll): | ||
if isinstance(coll, list): | ||
|
@@ -436,6 +462,16 @@ def validate_world_readable_and_sharing_settings(option_set): | |
) | ||
|
||
|
||
def validate_plotly_domains(option_set): | ||
domains_not_none = [] | ||
for d in ['plotly_domain', 'plotly_api_domain']: | ||
if d in option_set and option_set[d]: | ||
domains_not_none.append(option_set[d]) | ||
|
||
if not all(d.lower().startswith('https') for d in domains_not_none): | ||
warnings.warn(http_msg, category=UserWarning) | ||
|
||
|
||
def set_sharing_and_world_readable(option_set): | ||
if 'world_readable' in option_set and 'sharing' not in option_set: | ||
option_set['sharing'] = ( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Could you also include a test to check that no warning is issued when
https
is used?