validate plotly_(api)_domain - HTTPS, not HTTP #997

Kully · 2018-04-25T18:35:19Z

Addresses #996

Notes:

This error message does not get triggered if the original url in the request starts with zhttp://.... A separate error message is thrown for that.
The original url sent by a user is getting coerced into all lowercase, so a lowercase to lowercase comparison is not necessary.

Kully · 2018-04-25T18:35:46Z

BRONSOLO · 2018-04-25T20:19:15Z

@Kully @cldougl what do y'all think of approaching this fix as a validation on the .config file as opposed to error handling on a failed request?

Perhaps a helper function like validated_config_file can be written and called in the get/set_config_file helper functions to make sure the structure of the domains are valid (in this case, set to use HTTPS).

Kully · 2018-04-25T20:43:56Z

what do y'all think of approaching this fix as a validation on the .config file as opposed to error handling on a failed request?

I like this approach. What about we have both just in case? I don't know why the helper function could be bypassed if we set it up correctly though

BRONSOLO · 2018-04-25T20:55:54Z

What about we have both just in case?

I think the validation will be sufficient, personally!

cldougl · 2018-04-25T21:02:09Z

Yep I think the config validation could be more beneficial for the user.
Also we suggest that on-premise users set only their plotly_domain rather than both plotly_domain and plotly_api_domain (https://plot.ly/python/getting-started/#special-instructions-for-plotly-onpremise-users) so I think it would be confusing to mention the plotly_api_domain in a message returned to the user.

BRONSOLO · 2018-04-25T21:02:18Z

@Kully I think this should just be issued as a warning for now and not a full error. Though SSL/TLS is now required for On-Prem, there are some clients that still may be running older versions without SSL/TLS enabled.

Kully · 2018-04-26T20:50:36Z

@BRONSOLO I've deleted the error msg and made a warning instead.

I only placed the validate_config_file() in set_config_file() as get_config_file() appears in the set function.

There is the possibility of added a validate param in get_config that calls the validate_config_file within.

I personally think that we need to validate get_config_file without a bunch of the same error message popping up

BRONSOLO

Thanks @Kully! Could you please add a test for these changes?

BRONSOLO · 2018-04-30T14:09:21Z

plotly/tools.py

    else:
-        return FILE_CONTENT[CONFIG_FILE]
+        returned_obj = FILE_CONTENT[CONFIG_FILE]
+    return returned_obj


Is this change necessary? Could you add it to a separate commit with a commit msg that explains why if so?

BRONSOLO · 2018-04-30T14:12:33Z

plotly/tools.py

+)
+
+
+def validate_config_file(*domains):


This is really just validating domains - what do you think about renaming this as validate_domains?

…bbed

Kully · 2018-05-01T20:43:21Z

notes:

moved test to plotly.utils
new test should be passing 🎉
only raise UserWarning on set_config_file, not get_config_file

Waiting for tests to pass.

Kully · 2018-05-01T21:07:32Z

Looks like only streaming errors. We are ignoring these for now @cldougl ?

cldougl · 2018-05-02T14:54:17Z

@Kully, the streaming tests should pass now

BRONSOLO · 2018-05-02T15:48:31Z

plotly/tools.py

@@ -186,12 +186,14 @@ def set_config_file(plotly_domain=None,
    :param (bool) world_readable: True = public, False = private

    """
+    # import ipdb; ipdb.set_trace()


please remove this commented code as it is unnecessary 🔪 : )

BRONSOLO · 2018-05-02T17:50:18Z

plotly/utils.py

@@ -297,6 +322,7 @@ def encode_as_decimal(obj):
        else:
            raise NotEncodable

+


Is this line needed?

it keeps the file consistent with 2 spaces between functions

BRONSOLO · 2018-05-02T17:54:56Z

@Kully could you please provide a screenshot of what that warning message looks like when hit?

Kully · 2018-05-02T18:16:52Z

BRONSOLO · 2018-05-03T17:43:08Z

plotly/utils.py

@@ -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 "


Why include http here? ---> "with 'https', 'http'.

Also I would suggest using On-Premise instead of On-Prem to be consistent with the rest of the codebase.

Also, this .\n If... is introducing an unnecessary space at the start of the line that could be cleaned up.

That was a typo. I meant to type 'https', not 'http'. Is it fine like that?

Yep! Thanks.

BRONSOLO · 2018-05-03T17:43:27Z

plotly/utils.py

+    "    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 "


same here On-Prem ---> On-Premise

BRONSOLO · 2018-05-03T17:44:05Z

plotly/utils.py

+    "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."


BRONSOLO · 2018-05-03T17:46:44Z

plotly/tests/test_core/test_tools/test_file_tools.py

+            assert len(w) == 1
+            assert issubclass(w[-1].category, UserWarning)
+            assert "plotly_domain" in str(w[-1].message)
+


Nice! Could you also include a test to check that no warning is issued when https is used?

Kully · 2018-05-03T17:54:37Z

@BRONSOLO all comments addressed. Waiting for tests again.

Kully · 2018-05-03T19:12:44Z

yay tests are passing 🎉

BRONSOLO · 2018-05-04T18:39:07Z

plotly/utils.py

@@ -35,15 +35,15 @@

 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 "
+    "with 'https', not 'http'.\n If you are not using On-Premise then run the "


I believe the \n If ... spacing still needs to be fixed. I think it would look cleaner without a new line actually.

BRONSOLO · 2018-05-04T18:39:31Z

One small comment and then 💃 after that.

error message that catches responses with HTTP and not HTTPS

a9f0c95

make sure history is not an empty list

556ef45

made a warning instead

6b12bf2

Kully changed the title ~~error message that catches responses with HTTP and not HTTPS~~ validate plotly_(api)_domain - HTTPS, not HTTP Apr 27, 2018

BRONSOLO requested changes Apr 30, 2018

View reviewed changes

Kully added 5 commits April 30, 2018 11:33

changed validate_config_file to validate_domains

0c189f2

refactor get_config_file guts so returned_objs is validated after gra…

6e4622c

…bbed

validation warning can be triggered on get_config and set_config

c063f23

started on tests

fe31851

moved validation function to utils//test working

e55e81c

BRONSOLO reviewed May 2, 2018

View reviewed changes

removed commented line

efc36a2

BRONSOLO reviewed May 3, 2018

View reviewed changes

chuck's requestsed changes from review

46d3cc9

BRONSOLO reviewed May 4, 2018

View reviewed changes

remove \n from msg

2a0872c

Kully merged commit ba5975f into master May 4, 2018

Kully deleted the error-msg-for-http branch May 4, 2018 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate plotly_(api)_domain - HTTPS, not HTTP #997

validate plotly_(api)_domain - HTTPS, not HTTP #997

Kully commented Apr 25, 2018 •

edited

Loading

Kully commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018 •

edited

Loading

Kully commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018

cldougl commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018

Kully commented Apr 26, 2018

BRONSOLO left a comment

BRONSOLO Apr 30, 2018

BRONSOLO Apr 30, 2018

Kully Apr 30, 2018

Kully commented May 1, 2018

Kully commented May 1, 2018

cldougl commented May 2, 2018

BRONSOLO May 2, 2018

BRONSOLO May 2, 2018

Kully May 2, 2018

BRONSOLO commented May 2, 2018

Kully commented May 2, 2018

BRONSOLO May 3, 2018 •

edited

Loading

Kully May 3, 2018 •

edited

Loading

BRONSOLO May 3, 2018

BRONSOLO May 3, 2018

BRONSOLO May 3, 2018

BRONSOLO May 3, 2018

Kully commented May 3, 2018

Kully commented May 3, 2018

BRONSOLO May 4, 2018

BRONSOLO commented May 4, 2018

		@@ -297,6 +322,7 @@ def encode_as_decimal(obj):
		else:
		raise NotEncodable

validate plotly_(api)_domain - HTTPS, not HTTP #997

validate plotly_(api)_domain - HTTPS, not HTTP #997

Conversation

Kully commented Apr 25, 2018 • edited Loading

Kully commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018 • edited Loading

Kully commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018

cldougl commented Apr 25, 2018

BRONSOLO commented Apr 25, 2018

Kully commented Apr 26, 2018

BRONSOLO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully commented May 1, 2018

Kully commented May 1, 2018

cldougl commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BRONSOLO commented May 2, 2018

Kully commented May 2, 2018

BRONSOLO May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Kully May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully commented May 3, 2018

Kully commented May 3, 2018

Choose a reason for hiding this comment

BRONSOLO commented May 4, 2018

Kully commented Apr 25, 2018 •

edited

Loading

BRONSOLO commented Apr 25, 2018 •

edited

Loading

BRONSOLO May 3, 2018 •

edited

Loading

Kully May 3, 2018 •

edited

Loading