Closed cgore1 closed 3 years ago
Hello @cgore1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Line 34:99: E501 line too long (102 > 98 characters) Line 43:99: E501 line too long (113 > 98 characters)
I think it would be good to add a note here: https://github.com/tableau/TabPy/blob/master/docs/security.md
There is no description for the change - what is being changed and why.
There is safer and cleaner way for the improvement IMO:
With that the check will only be done once and not on every /evaluate request. Plus it won't be possible to make any script to be evaluated until TabPy is restarted with config change.
There is safer and cleaner way for the improvement IMO:
- Add new handler who only returns the error
- On TabPy start up check the flag and use the new handler if the feature is disabled (at https://github.com/tableau/TabPy/blob/master/tabpy/tabpy_server/app/app.py#L153)
With that the check will only be done once and not on every /evaluate request. Plus it won't be possible to make any script to be evaluated until TabPy is restarted with config change.
I am not sure why separate handler would be safer, since in both cases (with or without the separate handler) we will need to restart the app to toggle evaluate api ON/OFF
I feel having inline check for disabled check is simple and effective in this case, we can create a separate handler in future if needs to do extra things. (Again, I am new here, so correct me if I am wrong :) )
I am not sure why separate handler would be safer, since in both cases (with or without the separate handler) we will need to restart the app to toggle evaluate api ON/OFF
It is safer because it is possible to have script or model which deliberately or by mistake modifies the flag in memory (see the first point at https://github.com/tableau/TabPy/blob/master/docs/security.md). Having a handler who just returns the error without even looking at the flag avoids that problem.
I am not sure why separate handler would be safer, since in both cases (with or without the separate handler) we will need to restart the app to toggle evaluate api ON/OFF
It is safer because it is possible to have script or model which deliberately or by mistake modifies the flag in memory (see the first point at https://github.com/tableau/TabPy/blob/master/docs/security.md). Having a handler who just returns the error without even looking at the flag avoids that problem.
Oh interesting! Didn't think of that possibility! Well, creating a separate handler then
One another test you may want to add is for /info handler to return the new setting to a client - https://github.com/tableau/TabPy/blob/master/tests/unit/server_tests/test_service_info_handler.py
One another test you may want to add is for /info handler to return the new setting to a client - https://github.com/tableau/TabPy/blob/master/tests/unit/server_tests/test_service_info_handler.py
I am trying to add test condition to TestServiceInfoHandlerWithAuth, but the unit test fails with "RuntimeError: Failed to read passwords file ./tests/integration/resources/pwdfile.txt"
It expects path to pwd file to be absolute, any way to fix it? (Changing pwd file path to absolute path in unit test works) I wonder how the current test is successful in first place
For security reasons, some clients want to turn off ad hoc python scripts.
Added a new parameter "TABPY_EVALUATE_ENABLE" to TabPy’s configuration file (True by default) It will serve as a toggle control to TabPy’s evaluate endpoint