Skip to content
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

Support for plugin only request during initialization #93

Merged
merged 2 commits into from
Nov 10, 2016

Conversation

ben181231
Copy link

@oursky-travis
Copy link

@ben181231, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rickmak to be a potential reviewer

rickmak
rickmak previously requested changes Nov 8, 2016
@@ -60,6 +60,7 @@ def _request_url(self, action_name):
def _payload(self, action_name, params):
payload = params.copy() if isinstance(params, dict) else {}
payload['action'] = action_name
payload['_from_plugin'] = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this attribute need master_key in place to take effects?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the field _from_plugin just indicates that the request is from plugin / cloud code. Whether the request should be accepted, it should be determined by Skygear server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I side with @rickmak here you need to authenticate the request with a master key. Otherwise the skygear-server would not know whether the request really comes from a plugin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the developer has to be explicit in saying that the request will have the _from_plugin attribute. This is because the attribute requires master key and the master key has other implication (such as allowing schema migration). I don’t want every request to be authenticated with master key.

If you authenticate all requests with master key, you might as well allow all requests with master key to go through regardless of whether the request comes from a plugin.

Copy link
Author

@ben181231 ben181231 Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t want every request to be authenticated with master key.

I got it. So I will make it explicit so that user may need to call it like:

master_container = SkygearContainer(api_key=options.master_key)
master_container.send_action("some:action", params, plugin_request=True)

Does it make sense ?

@rickmak @cheungpat

@@ -60,6 +60,7 @@ def _request_url(self, action_name):
def _payload(self, action_name, params):
payload = params.copy() if isinstance(params, dict) else {}
payload['action'] = action_name
payload['_from_plugin'] = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I side with @rickmak here you need to authenticate the request with a master key. Otherwise the skygear-server would not know whether the request really comes from a plugin.

@@ -30,6 +30,7 @@
UndefinedOperation = 117
PluginUnavailable = 118
PluginTimeout = 119
PluginInitializing = 120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the updated code list in skygear-server.

@@ -60,6 +60,7 @@ def _request_url(self, action_name):
def _payload(self, action_name, params):
payload = params.copy() if isinstance(params, dict) else {}
payload['action'] = action_name
payload['_from_plugin'] = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the developer has to be explicit in saying that the request will have the _from_plugin attribute. This is because the attribute requires master key and the master key has other implication (such as allowing schema migration). I don’t want every request to be authenticated with master key.

If you authenticate all requests with master key, you might as well allow all requests with master key to go through regardless of whether the request comes from a plugin.

@ben181231
Copy link
Author

Updated.

@cheungpat cheungpat dismissed rickmak’s stale review November 10, 2016 10:12

His comments are addressed in my review.

@cheungpat cheungpat merged commit 83195b2 into SkygearIO:master Nov 10, 2016
@ben181231 ben181231 deleted the plugin-init-state branch December 6, 2016 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants