-
Notifications
You must be signed in to change notification settings - Fork 56
App id verification #32
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,15 +13,20 @@ | |
| }""" | ||
|
|
||
|
|
||
| class InvalidAppIdException(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class Request(object): | ||
| """ | ||
| Simple wrapper around the JSON request | ||
| received by the module | ||
| """ | ||
| def __init__(self, request_dict, metadata=None): | ||
| def __init__(self, request_dict, metadata=None, supported_app_ids=None): | ||
| self.request = request_dict | ||
| self.metadata = metadata or {} | ||
| self.session = self.request.get('session', {}).get('attributes', {}) | ||
| self.supported_app_ids = supported_app_ids | ||
| if self.intent_name(): | ||
| self.slots = self.get_slot_map() | ||
|
|
||
|
|
@@ -50,6 +55,19 @@ def access_token(self): | |
| def session_id(self): | ||
| return self.request["session"]["sessionId"] | ||
|
|
||
| def application_id(self): | ||
| try: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please replace the generic exception handling with an explicit check, or at least catch the specific exception |
||
| return self.request['session']['application']['applicationId'] | ||
| except: | ||
| return None | ||
|
|
||
| def has_valid_app_id(self): | ||
| if not self.supported_app_ids: | ||
| return True | ||
| if not self.application_id(): | ||
| return False | ||
| return self.application_id() in self.supported_app_ids | ||
|
|
||
| def get_slot_value(self, slot_name): | ||
| try: | ||
| return self.request["request"]["intent"]["slots"][slot_name]["value"] | ||
|
|
@@ -192,11 +210,18 @@ def _handler(func): | |
|
|
||
| return _handler | ||
|
|
||
| def route_request(self, request_json, metadata=None): | ||
| def route_request(self, request_json, metadata=None, app_ids=None): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that having the app_ids as a construction parameter for the 'alexa' makes more sense than passing it into a parameter for route_request
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i agree, should i add it to the constructor somehow? or do |
||
|
|
||
| ''' Route the request object to the right handler function ''' | ||
| request = Request(request_json) | ||
| request.supported_app_ids = app_ids | ||
| request.metadata = metadata | ||
|
|
||
| # before we do anything, lets verify the application id | ||
| # if app_ids is not passed in, verification does not occur | ||
| if not request.has_valid_app_id(): | ||
| raise InvalidAppIdException('Invalid ApplicationId: %s' % request.application_id()) | ||
|
|
||
| # add reprompt handler or some such for default? | ||
| handler_fn = self._handlers[self._default] # Set default handling for noisy requests | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make more sense to do the supported app IDs verification in the VoiceHandler object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for late reply, been really busy, but yes it does i just made this on the quickness, didnt really think about it too much, ill make the changes.