Generic timestamp request

[kkocel]

Description

Instead of using a full request just created an essential request class that has fields that are only needed.

Motivation and Context

When an unknown request type comes to the validator it fails because it cannot be deserialized.
By introducing GenericTimestampRequest this problem is avoided.
For validation, a timestamp is only needed and GenericTimestampRequest provides only that information.

Testing

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed

License

• By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[doiron]

I feel this belongs in the model package. We'd have to create some sort of base class and have RequestEnvelope inherit and extend. The models are autogenerated so we would have to make the change I believe.

[kkocel]

the problem is that when you deal with requests that are not present in the model package. I think that request validation should not rely on the model package - you should have anemic request class that should deserialize to fields required only by validation logic

[doiron]

I agree to some degree, I think this function should only extract what it needs. But that should be defined as a generic object in the model. Otherwise, we have two packages where a request is defined. i.e however unlikely, if the timestamp field moves/renamed, this would be hard to catch in the future.

so not entirely against this just need to think about this a bit more. Currently I see more pros in having this in the model than cons.

[kkocel]

the biggest problem here is that I cannot edit your template files since they're private. Also having a base class can be tricky when relying on builders to create objects.

I know that there is a defaultImpl property in Jackson: https://fasterxml.github.io/jackson-annotations/javadoc/2.6/com/fasterxml/jackson/annotation/JsonTypeInfo.html#defaultImpl()

So a request could be mapped to UnknownRequest that would have locale, type, request-id, and timestamp.

[doiron]

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

[kkocel]

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

[doiron]

thank you kkocel for making this change and improving the experience for others. :)

[doiron]

After syncing with the team we decided not to go ahead with this solution as it does validate that the request is in-fact a RequestEnvelope. This add extra layers of protection and will deny requests that have extra ( potentially malicious ) json parameters.

[doiron]

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

In this case, you would need to have the updated SDK that adds the Beta feature support. Otherwise the skill should not be accepting the request.

[kkocel]

@doiron

After syncing with the team we decided not to go ahead with this solution as it does validate that the request is in-fact a RequestEnvelope. This add extra layers of protection and will deny requests that have extra ( potentially malicious ) json parameters.

There are two types of vaildators in the SDK - one that validates request signature
and the one that validates the timestamp. Both are used to validate the request. So there is no possibility to pass a request that is malicious (unless there is a request signed by Amazon that is malicious).

Even if the request has extra json parameters they will be discarded by Jackson deserialization - because Jackson deserializes json into a well defined Java object.

Having this knowledge maybe team can reconsider?

[kkocel]

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

In this case, you would need to have the updated SDK that adds the Beta feature support. Otherwise the skill should not be accepting the request.

Ok, so I would like to get the thin jar containing model package only for the beta we are participating in. We do not want uber/fat jar since dealing with it is maintainability nightmare. We will pass this request through our solution architect as well.

[doiron]

There are two types of validators in the SDK - one that validates request signature
and the one that validates the timestamp. Both are used to validate the request. So there is no possibility to pass a request that is malicious (unless there is a request signed by Amazon that is malicious).

Even if the request has extra json parameters they will be discarded by Jackson deserialization - because Jackson deserializes json into a well defined Java object.

Having this knowledge maybe team can reconsider?

Furthermore, this change would need to:

1. Be updated so that the changes are backward compatible, or bump up the major version with a breaking change, forcing consumers to upgrade.
   1. It's possible we could just introduce a new Constructor instead of replacing the RequestEnvelope object in the ServletRequest class. This will allow Skills that solely rely on the Verifiers to still function as is and pull in the latest bits. Some consumers have their own implementation/equivalent to the SkillServlet class.
   2. potentially a new backward compatible implementation of the AlexaHttpRequest interface. This could require some extra conversion logic from RequestEnvelope to GenericTimestampRequestEnvelope. ??
2. Update the Python SDK as well.
3. For our node SDK, it doesn't perform any deserialization so most likely no work required for that package.

Since this would take a while for us to prioritize and sort out, I've compiled a jar file to unblock you in the interim, let us know how that works out.

thanks again for working on this.

[kkocel]

I left an issue for this: #326