Add a ClientVPN connection handler request/response definition

[gpoul]

Also added a sample connection handler and linked it from the README

[harrisonhjones]

Thanks for the contribution. I'm added some of my thoughts. Let me know what you think.

[harrisonhjones]

LGTM. @bmoffatt what do you think?

[harrisonhjones]

small nitpick: do you need to set either of these? The nil value for a string is "" and it might be ok for PostureComplianceStatuses to be nil? I only ask because omitting them might be cleaner.

[edit] https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/connection-authorization.html suggests these two fields are "required" so I definitely wouldn't omitempty them but it might still be ok for them to be "" and nil (their zero values).

[gpoul]

I've written a test like this to see how I can make this happen:

func TestClientVPNConnectionHandlerResponseConstructorValidation(t *testing.T) {
        // read json from file
        inputJSON, err := ioutil.ReadFile("./testdata/clientvpn-connectionhandler-response.json")
        if err != nil {
                t.Errorf("could not open test file. details: %v", err)
        }

        // construct response
        outputEvent := ClientVPNConnectionHandlerResponse{
                Allow: true,
                SchemaVersion: "v1",
        }

        // serialize to json
        outputJSON, err := json.Marshal(outputEvent)
        if err != nil {
                t.Errorf("could not marshal event. details: %v", err)
        }

        assert.JSONEq(t, string(inputJSON), string(outputJSON))
}

for the string value that is easy; I can just drop it and it will be "", but the array is not so easy to get marshalled as {}; if I understand golang/go#37711 correctly the choice is either a func NewClientVPNConnectionHandlerResponse or a custom marshaller that handles this; both feel like they depart from how the rest of the library is used a bit, although there seems to be functions for DynamoDB attributes as well.

Anything you'd suggest? I somehow feel it's easier to keep this in the sample and hopefully people will use the sample to then end up with the right data structure? The alternative that feels clean as well is the function that creates an "empty" object as it could also create the SchemaVersion as well and we can drop the version from the sample code; that would gain something at least.

What do you think?

[harrisonhjones]

 ClientVPNConnectionHandlerResponse{
                Allow: true,
                SchemaVersion: "v1",
        }

should marshal as:

{
	"allow": true,
	"error-msg-on-failed-posture-compliance": "",
	"posture-compliance-statuses": null,
        "schema-version": "v1",
}

The question is: is this ok? Or does it cause failures. Does it need to be:

{
	"allow": true,
	"error-msg-on-failed-posture-compliance": "",
	"posture-compliance-statuses": [],
        "schema-version": "v1",
}

If it does then yea, we'll need to keep the:

PostureComplianceStatuses: []string{},

[gpoul]

It tested with

                return events.ClientVPNConnectionHandlerResponse{
                        Allow: true,
                        ErrorMsgOnFailedPostureCompliance: "",
                        SchemaVersion: "v1",
                }, nil

and

                return events.ClientVPNConnectionHandlerResponse{
                        Allow: true,
                        SchemaVersion: "v1",
                }, nil

and they both fail with an "internal" error. I think the {} is indeed required to be returned to ClientVPN.

Would you think a func NewClientVPNConnectionHandlerResponse would be good or should we leave the example as-is?

[harrisonhjones]

Ah, that stinks. It's [] not {} right?

[gpoul]

Yes, sorry; it's an empty array. Even in JSON. The behavior with ClientVPN I've tested is that both code snippets above in #343 (comment) don't result in a successful authentication; whereas if I use the original snippet that initializes an empty array instead of nil it works.

So I think we either need to include this in the sample or provide a function that correctly initializes the response struct.

[bmoffatt]

thanks for the contribution!