Enable Token Authorization by default #3163
Conversation
frontend/server/src/main/java/org/pytorch/serve/http/TokenAuthorizationHandler.java
Show resolved
Hide resolved
| | InstantiationException | ||
| | InvocationTargetException | ||
| | ClassNotFoundException e) { | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| logger.error("TOKEN CLASS IMPORTED UNSUCCESSFULLY"); |
There was a problem hiding this comment.
Nit: We could remove this since we no longer use the plugin approach.
There was a problem hiding this comment.
Kept it so that when TorchServe starts users can see if the keyfile was created successfully if enabled.
There was a problem hiding this comment.
They will be able to see this through line 98
There was a problem hiding this comment.
Whats more important is that we fail in case the token auth setup could not be completed. The exception message need to be adapted here.
There was a problem hiding this comment.
Also better to be more specific about which exception to catch.
I feel that if we are implementing token authorization as default it makes sense to integrate it into the frontend rather than as an external plugin as there is no need for that. It would also make future changes painful since we would have to build the plugin jar and publish to maven. It makes sense to integrate and remove the plugin completely so rather then duplication of code its just moving. |
Added the test and added more clarification in the docs. |
| } else { | ||
| checkTokenAuthorization(req, "management"); | ||
| } | ||
| } else if (tokenType == TokenType.INFERENCE) { | ||
| checkTokenAuthorization(req, "inference"); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Why do we need this clause? Would we not get a ResourceNotFoundException() even if we don't handle this?
There was a problem hiding this comment.
This is to prevent the token api from being called when token authorization is disabled.
There was a problem hiding this comment.
I think @namannandan is referring to that the last element in the chain will throw the exception:
as not other Handler should handle "token".
There was a problem hiding this comment.
The tests are not sufficient. We need to add disable through env variable and the different priorities. We also need to test updating the expired and not expired tokens (of both types).
I've left a bunch of other comments which are more related to a refactoring as we're e.g. introducing dependency cycles here. I'll create a follow up issue where we need to address these on a short timeline. We can ignore these for now as this is time sensitive but the testing needs to be improved before we can merge this.
frontend/server/src/main/java/org/pytorch/serve/http/TokenAuthorizationHandler.java
Show resolved
Hide resolved
| } else { | ||
| checkTokenAuthorization(req, "management"); | ||
| } | ||
| } else if (tokenType == TokenType.INFERENCE) { | ||
| checkTokenAuthorization(req, "inference"); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I think @namannandan is referring to that the last element in the chain will throw the exception:
as not other Handler should handle "token".| @@ -1203,6 +1230,10 @@ public String getWorkflowStore() { | |||
| return workflowStore; | |||
| } | |||
|
|
|||
| public String isTokenDisabled() { | |||
There was a problem hiding this comment.
Why do we use a string here instead of boolean?
There was a problem hiding this comment.
Using this function to set prop and prop takes strings not booleans.
|
Added new tests
|
Description
Setting token authorization by default #3157
Users can disable using environment variables, command line, or with config.properties.
Check Documentation for examples
Tests
Updating all existing tests to disable token authorization except for token authorization tests in pytest