Support plugin only request during plugin initialization #221
Conversation
|
@ben181231, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rickmak to be a potential reviewer |
ben181231
force-pushed
the
plugin-init-state
branch
from
ee8c871
to
219787e
Compare
| skyerr.PluginUnavailable, | ||
| "plugins are unavailable at the moment", | ||
| ) | ||
| return http.StatusServiceUnavailable |
There was a problem hiding this comment.
I think a test case for this preprocessor is needed as it become more complicated.
| return http.StatusOK | ||
|
|
||
| if p.PluginContext.IsInitialized() { | ||
| if !payload.HasMasterKey() || !payload.IsFromPlugin() { |
There was a problem hiding this comment.
The isFormPlugin() should only have effect if there is present of master_key.
There was a problem hiding this comment.
Okay. Maybe I should rename it to IsPluginRequest()
ben181231
force-pushed
the
plugin-init-state
branch
from
219787e
to
e0f74a6
Compare
|
I will leave more comments |
| @@ -119,6 +119,9 @@ const ( | |||
| // PluginTimeout occurs when an operation carried by a plugin is timed out | |||
| PluginTimeout | |||
|
|
|||
| // PluginInitializing occurs when any of the plugins are initializing | |||
There was a problem hiding this comment.
“Error codes for expected error condition should be placed above this line.”
i.e. do not insert errors in the middle of a list
| // IsValidPluginRequest returns true when the payload indicate that the | ||
| // request is from plugin AND the master key is used | ||
| func (p Payload) IsValidPluginRequest() bool { | ||
| fromPlugin, _ := p.Data["_from_plugin"].(bool) |
There was a problem hiding this comment.
It is the job of a preprocessor to decide whether the request is a valid plugin request, not the Request struct. Move this function out of here.
There was a problem hiding this comment.
Especially that the “HasMasterKey” function only returns valid value after a auth preprocessor update it.
There was a problem hiding this comment.
Does it conflict with this comment ?
There was a problem hiding this comment.
Follow UserAuthenticator in preprocessor/auth.go
| } else if h.AccessKeyRequired { | ||
| h.preprocessors = h.PreprocessorList.GetByNames( | ||
| "plugin", "authenticator") | ||
| "authenticator", |
There was a problem hiding this comment.
do you need access_key here? (and above)
There was a problem hiding this comment.
The access_key is needed since the plugin handler is setting AccessKeyRequired to be true.
There was a problem hiding this comment.
Ah I see now, either accesskey or authenticator is required. So authenticator fits the bill here.
ben181231
force-pushed
the
plugin-init-state
branch
from
e0f74a6
to
135042b
Compare
cheungpat
dismissed
rickmak’s stale review
His comments are addressed in my review.
connect #219