It would be really good to have a 'Dev' state/mode which would allow you to develop in a local Dev environment, i.e. to POST JSON requests manually, but at the moment the Alexa\Request\Certification::validateRequest() always validate your requests and it - understandably - fails.

I was thinking about integrating this with the State API, and updating Drupal\alexa\Controller\AlexaCachedCertificate implementing:

  public function validateRequest($requestData) {
    if (!\Drupal::state()->get('dev_mode', FALSE)) {
      parent::validateRequest($requestData);
    }
  }
CommentFileSizeAuthor
#11 skip_certificate-2853528-11.patch1.56 KBhampercm
#9 skip_certificate-2853528-9.patch2.09 KBhampercm
#5 skip_certificate-2853528-5.patch3.85 KBgambry

Comments

gambry created an issue. See original summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Title: Always validate Certificate request when in Dev mode » Skip certificate validation when in Dev mode
gambry’s picture

gambry
Primary language Italian
Location Milan
commented

I forgot to mention I'm happy to suggest a patch, if you like the idea.

samuel.seide’s picture

samuel.seide commented

I think a local dev mode is a good idea. It's not good practice to continually push code up to production just to test if it's working. Though it appears the call is no longer being made in Drupal\alexa\Controller\AlexaCachedCertificate and is now being done inside AlexaEndpointController.php starting on line 63.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Active » Needs review
StatusFileSize
new skip_certificate-2853528-5.patch3.85 KB

Let's make some progress.

Adding a Development mode checkbox on configuration form, driven by the State API so it can have per-environment values.
Checking the checkbox set the switch alexa.dev_mode TRUE so the Alexa\Request\Certificate::validateRequest() is skipped.

@samuel.seide I'm on 8.x-1.x and the call is still as per IS details.

hampercm’s picture

hampercm commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patch! Good call on using the State API for this :)

There's one typo in your patch, which I'll just fix on commit.

  • hampercm committed 02acba1 on 8.x-1.x authored by gambry 
    Issue #2853528 by gambry: Skip certificate validation when in Dev mode
hampercm’s picture

hampercm commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
Assigned: Unassigned » hampercm
Status: Reviewed & tested by the community » Patch (to be ported)
hampercm’s picture

hampercm commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new skip_certificate-2853528-9.patch2.09 KB

Patch ported to D7

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Needs review » Needs work
Issue tags: +Needs manual testing 
  1. +++ b/alexa.module
@@ -21,8 +21,8 @@ function alexa_menu() {
-    'title' => 'Alexa Configuration',
-    'description' => 'Manage Application IDs for Alexa Skills',
+    'title' => 'Alexa',
+    'description' => 'Manage configuration for Alexa Skills integration.',

    As we are changing this bit, we can wrap the text with t() to facilitate translators life. 

  2. +++ b/alexa.module
@@ -114,14 +121,10 @@ function _alexa_handle_callback() {
+    drupal_add_http_header('Status', '200 OK');
     if ($response && !empty($response->outputSpeech)) {
-      print json_encode($response->render());
-
-      drupal_add_http_header('Status', '200 OK');
       drupal_add_http_header('Content-type', 'application/json');
-    }
-    else {
-      drupal_add_http_header('Status', '500 Internal Server Error');
+      print json_encode($response->render());

    Is this change related to this issue?

I haven't done any manual test.

hampercm’s picture

hampercm commented
Status: Needs work » Needs review
StatusFileSize
new skip_certificate-2853528-11.patch1.56 KB

Thanks for the review! Re #10:

1) Explicit use of t() in hook_menu is not necessary. See "title" and "description" in https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

2) No, that's a separate fix that got included by mistake. Removed.

I have manually tested this, but a second manual test would be ideal.

hampercm’s picture

hampercm commented
Assigned: hampercm » Unassigned
gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Needs review » Needs work

I get two Notices:

Notice: Undefined index: HTTP_SIGNATURE in _alexa_parse_request()

Notice: Undefined index: HTTP_SIGNATURECERTCHAINURL in _alexa_parse_request()

Should we check if Dev mode is ON or wrapping those 2 line with isset()?

Besides I haven't managed to get the library loading in D7.
I'll try building the codebase completely using composer and check again.

hampercm’s picture

hampercm commented

@gambry thanks for the review. I'll fix those PHP notices.

The D7 module can use Composer Manager to install its dependencies. If you have it installed and enabled before you install the Alexa module, it will take care of importing the library automatically.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented

@hampercm I was using Composer Manager, without luck. Just for the sake of testing this issue I've also included manually composer autoload.php, again without luck. I'll try again with next review.

  • hampercm committed 7a13188 on 7.x-1.x 
    Issue #2853528 by hampercm, gambry: Skip certificate validation when in...
hampercm’s picture

hampercm commented
Status: Needs work » Fixed

Fixed the log messages regarding unset array values, and committed. Thanks for the review and testing, @gambry

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.