Some https related things and Echo Dialog directives #44

rohitggarg · 2018-07-16T04:57:12Z

Better errors, print the error due to which stuff failed on server
Fallback to insecureSkipVerify for cert errors
EchoDirectives, Dialog, ConfirmationStatus of Intent

… system is missing

harrisonhjones · 2018-07-18T17:57:07Z

skillserver/echo.go

@@ -171,6 +171,11 @@ func (this *EchoResponse) EndSession(flag bool) *EchoResponse {
 	return this
 }

+func (this *EchoResponse) Dialog(name string, intent *EchoIntent) *EchoResponse {


Would be good to document and/or rename this function. From the name, parameters, and return value alone I don't understand what it does.

Umm, I was kind of confused. Neither does this method only start a dialog, nor does it end it, its kind of, all states. I can't also say respond becayse this one is a different kind of response.
Like you have kept .Card func, I thought I could keep .Dialog, can you please suggest a name here ?

Happy to suggest a name once I understand what it does :) Can you give me some more details? It looks like it appends a name and an intent to a list of response directives. What does doing that do? Maybe "AddDirective"? That being said why do we need this helper? It's a wrapper around a single line of code.

Okay, so the choice of name is from 4 values namely: Dialog.Delegate, Dialog.ElicitSlot, Dialog.ConfirmSlot, Dialog.ConfirmIntent.
All of these 4 have specific purpose when alexa talks on the same intent to the user. Basically, dialog api is for getting "slot" values from the chit chat. Slot values can be stuff like dates, time, singers, authors etc. The response to the opening of intent generally can either be a voice or a dialog response out of the above 4.
If the user has given all slot values, there is no point opening a dialog, hence this is only set if stuff is missing.
Eg., in a engaging conversation, like questionnaire, alexa will ask a question and expect a response. If alexa would like to hint the user, she would say Dialog.Delegate { updatedSlot: }

harrisonhjones · 2018-07-18T17:59:26Z

skillserver/skillserver.go

 	if err != nil {
-		return nil, errors.New("Could not download Amazon cert file.")
+		return nil, errors.New("Could not download Amazon cert file. Reason: " + err.Error())


Go style is to join errors with : so (ideally) this should be errors.New("could not download cert file: " + err.Error(). Alternately we could import github.com/pkg/errors and use errors.WithMessage(err, "could not download cert file")

A valid follow up question: why would you ever want to skip ssl verification here?

Thanks, I will add that.
Insecure skip verify is definitely dangerous, but I was thinking if somebody got a warning, they could atleast go about testing the the thing they made. Definitely not recommended for production cases. There are sometimes cases with docker containers built from scratch that root ca certs aren't present. So I thought if you're only testing, it would be okay to just log the error but not stop the whole thing altogether. I like the idea of keeping a flag and saying explicitly, if you wanted to do insecure skip verify, like curl does it

harrisonhjones · 2018-07-18T18:00:26Z

skillserver/skillserver.go

 	}
 	defer cert.Body.Close()
 	certContents, err := ioutil.ReadAll(cert.Body)
 	if err != nil {
-		return nil, errors.New("Could not read Amazon cert file.")
+		return nil, errors.New("Could not read Amazon cert file. Reason: " + err.Error())


Similar comment here about joining errors and/or using github.com/pkg/errors. Also, generally errors don't have capitalized first letters.

harrisonhjones · 2018-07-18T18:04:09Z

skillserver/skillserver.go

+	certPool, err := x509.SystemCertPool()
+	insecureSkipVerify := false
+	if err != nil || certPool == nil {
+		log.Println("Can't open system cert pools, doing insecure skip verify instead")


this seems dangerous to me. I think a better option would be to either take a new argument which allows readCert to skip ssl verification OR keep this logic but, if ssl verification is skipped but a cert is retrieved, return a typed error to indicate this. The second option maintains backwards compatibility the best. Consumers of readCert function could then check the error type and proceed (or not) if they care about ssl verification being skipped.

Please see above, thanks

rking788 · 2018-07-19T22:08:54Z

If this ends up getting merged then we should probably close this pull request that I opened a little while ago: #21

rohitggarg · 2018-07-20T02:49:19Z

I just looked at #21, it definitely has more features, do all of them work the way pull request is at the moment?

mikeflynn · 2018-07-21T07:48:37Z

#21 has conflicts now anyway, so we lets just make sure this has both features in it and I can merge.

(Also, feel free to ping me on Twitter [@thatmikeflynn] when I fall behind on my insane list of GitHub notifications.)

rohitggarg · 2018-07-21T12:07:51Z

Okay, I have added things from #21 and some of my own interpretation of what alexa had to offer..

Kindly review!

Thanks,
RohitG

harrisonhjones · 2018-07-23T05:19:13Z

skillserver/dialog/default.go

@@ -0,0 +1,16 @@
+package dialog
+
+type Type string


Based on the wording in the docs I would call this a Directive instead of a Type.

My thought process was, the Directive is the entire thing, like with intents and all, its a DialogType or DirectiveType
I created another package so that we could refer to this as dialog.Type
The json also is sending this thing as "type" hence I thought of keeping it as just camelized type

harrisonhjones · 2018-07-23T05:20:06Z

skillserver/echo.go

@@ -4,6 +4,14 @@ import (
 	"encoding/json"
 	"errors"
 	"time"
+	"github.com/mikeflynn/go-alexa/skillserver/dialog"


I forget which but either go fmt or goimports will automatically group this (and other 3p imports) in a new section (ie, just with a simple extra newline after the std imports.

harrisonhjones · 2018-07-23T05:20:39Z

skillserver/echo.go

+	"github.com/mikeflynn/go-alexa/skillserver/dialog"
+)
+
+type ConfirmationStatus string


Where is this used in this commit?

harrisonhjones · 2018-07-23T05:23:59Z

Update generally looks good. I'd love to see some tests but I can understand if that doesn't make it into this code change.

mikeflynn · 2018-07-24T05:02:56Z

What's the consensus here? @rking788 @harrisonhjones? I don't have time to fully vet this, so I'll merge on your advisement.

Thanks for the PR, @rohitggarg!

rohitggarg · 2018-07-24T05:04:21Z

Hey Mike,

Please wait for a little while, I will drop in a change for the insecure flag and let you know.

rking788 · 2018-07-24T12:15:25Z

I will try to look at this later tonight.

mikeflynn · 2018-07-26T04:29:04Z

Got it. Holding.

rohitggarg · 2018-07-26T04:43:04Z

This should be good now.

Thanks,
RohitG

harrisonhjones · 2018-07-26T17:34:41Z

skillserver/skillserver.go

@@ -199,16 +206,10 @@ func verifyJSON(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {

 // Run all mandatory Amazon security checks on the request.
 func validateRequest(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
-	// Check for debug bypass flag
-	devFlag := r.URL.Query().Get("_dev")


Why are we removing this check? How do we know no one is using it?

Maybe we could move this into IsValidAlexaRequest?

This validation is a certificate check, i have moved it to insecure skip verify which is the first condition in the IsValidAlexaRequest so two checks in here weren't making sense

I get the desire but what if we have users who are actively using this flag? We should preserve it for backwards compatibility.

Ok, so i will put a or condition then.
Thanks for the review

harrisonhjones · 2018-07-26T17:35:59Z

skillserver/skillserver.go

-			return
-		}
+	isRequestValid := IsValidAlexaRequest(w, r)
+	if !isRequestValid {


we can inline this unless we use isRequestValid elsewhere. Ie:

if !IsValidAlexaRequest { return }

We might also want to log if the request isn't valid.

harrisonhjones · 2018-07-26T17:37:50Z

skillserver/skillserver.go

 	if err != nil || certPool == nil {
-		log.Println("Can't open system cert pools, doing insecure skip verify instead")
-		insecureSkipVerify = true
+		log.Println("Can't open system cert pools")


If this occurs the tls.Config below will have a nil RootCA. Is that ok?

Yea, nil is fine below, thats how the systemcert pool has been coded

harrisonhjones · 2018-07-28T16:34:06Z

Looks good to me. Thanks @rohitggarg for the contribution!

Rohit Garg added 2 commits July 16, 2018 10:23

Added echo directive and dialog state for talking in the same intent

7dbc08a

Some logging, resorting to insecure skip verify in case cert chain in…

ba74954

… system is missing

harrisonhjones reviewed Jul 18, 2018

View reviewed changes

Rohit Garg added 2 commits July 21, 2018 17:27

Add new dialog stuff as per support

c8f9d3a

Added Confirmation status as a type

8ade3c6

harrisonhjones reviewed Jul 23, 2018

View reviewed changes

Rohit Garg added 3 commits July 26, 2018 10:02

Added flag parsing for insecure skip verify

006e5c7

Added insecure skip verify to the amazon cert validator also

b08fa28

Updated README

4fd8889

harrisonhjones reviewed Jul 26, 2018

View reviewed changes

Review feedback

ed8b373

mikeflynn merged commit ee5f23c into mikeflynn:master Jul 30, 2018

rking788 mentioned this pull request Aug 3, 2018

Start adding support for dialogs. #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some https related things and Echo Dialog directives #44

Some https related things and Echo Dialog directives #44

rohitggarg commented Jul 16, 2018

harrisonhjones Jul 18, 2018

rohitggarg Jul 19, 2018

harrisonhjones Jul 19, 2018

rohitggarg Jul 19, 2018

harrisonhjones Jul 18, 2018

harrisonhjones Jul 18, 2018

rohitggarg Jul 19, 2018

harrisonhjones Jul 18, 2018

harrisonhjones Jul 18, 2018

rohitggarg Jul 19, 2018

rking788 commented Jul 19, 2018

rohitggarg commented Jul 20, 2018

mikeflynn commented Jul 21, 2018

rohitggarg commented Jul 21, 2018

harrisonhjones Jul 23, 2018

rohitggarg Jul 23, 2018 •

edited

harrisonhjones Jul 23, 2018

harrisonhjones Jul 23, 2018

harrisonhjones commented Jul 23, 2018

mikeflynn commented Jul 24, 2018

rohitggarg commented Jul 24, 2018

rking788 commented Jul 24, 2018

mikeflynn commented Jul 26, 2018

rohitggarg commented Jul 26, 2018

harrisonhjones Jul 26, 2018

rohitggarg Jul 27, 2018

harrisonhjones Jul 27, 2018

rohitggarg Jul 27, 2018

harrisonhjones Jul 26, 2018

harrisonhjones Jul 26, 2018

rohitggarg Jul 27, 2018

harrisonhjones commented Jul 28, 2018

Some https related things and Echo Dialog directives #44

Some https related things and Echo Dialog directives #44

Conversation

rohitggarg commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rking788 commented Jul 19, 2018

rohitggarg commented Jul 20, 2018

mikeflynn commented Jul 21, 2018

rohitggarg commented Jul 21, 2018

Choose a reason for hiding this comment

rohitggarg Jul 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrisonhjones commented Jul 23, 2018

mikeflynn commented Jul 24, 2018

rohitggarg commented Jul 24, 2018

rking788 commented Jul 24, 2018

mikeflynn commented Jul 26, 2018

rohitggarg commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrisonhjones commented Jul 28, 2018

rohitggarg Jul 23, 2018 •

edited