mreinstein / alexa-verifier

✓ Verify HTTP requests sent to an Alexa skill are sent from Amazon
MIT License
76 stars 23 forks source link

Support for Cortana #22

Closed navzam closed 7 years ago

navzam commented 7 years ago

As part of Cortana's skills kit, developers can reuse code from their existing Alexa Skills. It'd be great if this library supported verification of Cortana requests as well. The verification process is similar, so the core logic shouldn't change much.

navzam commented 7 years ago

One thing we should decide on is how users will specify which mode they want (Alexa or Cortana). It makes sense to add a parameter to the main function exported by the module. But to ensure we don't break anything for current users, I guess it would have to be the last parameter:

module.exports = function verifier(cert_url, signature, requestBody, callback, mode)

That would work, though we'd break the convention of keeping the callback as the last parameter.

Or, if we're okay changing the parameters (and probably bumping up a major release), we could put all the parameters into an options object:

module.exports = function verifier(options, callback)

Thoughts?

mreinstein commented 7 years ago

Hi Nafis,

Thanks for opening an issue!

I'd like to understand the idea/usage a little better. Based on our previous chat, it sounds like the main goal is to allow people to take their Alexa skills and re-use that code in cortana. Is Cortana going to provide a nearly identical api as Alexa?

Perhaps you could walk me through how a developer would take their existing Alexa skill code and adopt it to work on Cortana? As a developer, do I maintain 2 codebases, 1 for my Alexa skill and 1 for Cortana? Or is the thought more that you'd build one app that works in both platforms?

I want to better understand the goals and thoughts behind this before we get into exact low level details like how API(s) would be modified.

navzam commented 7 years ago

Yep, one goal is to let people bring their existing Alexa Skills and reuse the code. Since the request/response format is nearly identical, a developer could maintain one codebase that works on both platforms. The platforms may offer different features though (i.e. Cortana could offer a feature that Alexa does not), so it may be advantageous to have two codebases, or at least some logic within the single codebase to differentiate between the two platforms.

mreinstein commented 7 years ago

thanks for the follow-up @navzam !

I've given it some more thought. A cortana specific module and repo for this verification would be better.

Part of what makes node/npm awesome is the philosophy of small modules with limited, specific purpose. alexa-verifier is inherently implying that this is related to amazon's alexa ecosystem.

Even if right now the cortana and alexa skill verifications are similar, they will probably diverge in the future. Making them separate modules (but with a hopefully almost identical API) allows each one to be built purposefully and not have a bunch of abstraction layers or other needless plumbing.

I've set up https://github.com/mreinstein/cortana-verifier and https://www.npmjs.com/package/cortana-verifier

Let's use this module as a starting point, and we can work together to customize it for Cortana's specifics. Happy to add you or anyone else on the cortana team as contributors. Ya dig?

tejashah88 commented 7 years ago

Just wanted to pop in and give my thoughts on this.

Since we'll be having a cortana-verifier module, it would make sense to have an expressjs middleware module called cortana-verifier-middleware (or something like that), similar to the alexa-verifier-middleware module.

dblock commented 7 years ago

My +1 on the discussion above. If you can "bring your alexa skill to cortana", verification should also work without changes, or by at the most whitelisting some domains from which requests came from. Certainly mode seems the opposite of what we want.

navzam commented 7 years ago

@dblock We could whitelist the additional domains, but then Cortana requests would be accepted by Alexa Skills (and vice versa). Something like mode would just switch between things like these strings. The underlying logic would remain the same.

@mreinstein I've considered a separate cortana-verifier module too. Ultimately I preferred having alexa-verifier support both b/c it would be much easier to add features or fix bugs common to both. Otherwise we'd be duplicating work on two repos that are essentially moving together.

mreinstein commented 7 years ago

I prefer having alexa-verifier support both b/c it would be much easier to add features or fix bugs common to both.

This module rarely changes. Assuming I maintain this I will continue to push back on pretty much all feature requests and bug fix PRs unless they fix something serious. This is another benefit of building small, very specific modules...eventually they are done! :)

Otherwise we'd be duplicating work on two repos that are essentially moving together.

Yes, today they happen to be moving together, because it is in Cortana's interests to make adopting Alexa skills as easy as possible. But Cortana and Alexa are 2 very different systems, and at some point they are going to diverge in numerous ways. Having some duplication up-front is actually a good thing, because it gives you room to breathe in terms of carving out your own API. Cortana likely already has some features that Alexa doesn't.

After some time has passed, when things stabilize, then I think it makes sense to extract functionality that is common to both alexa and cortana and move them into re-usable modules.

Related to this, I'm assuming there are similar efforts that the Cortana team will want to make in alexa-app and alexa-app-server. I'm not the primary maintainer for those modules, but I'd strongly recommend to them that you avoid turning these already too large modules into even larger ones. They end up taking something that is simple and built for a different purpose and turning them into these messy abstraction monsters.

navzam commented 7 years ago

Points taken. Let's continue on with cortana-verifier for now. You can add me to the repo for now. I'll let you know if we need to add anyone else.

tejashah88 commented 7 years ago

@navzam Can you provide us with some documentation for the verification specs?

mreinstein commented 7 years ago

@navzam I just added you to the github repo.

dblock commented 7 years ago

+1 on what @mreinstein says above

I think the switching should happen in alexa-verifier-middleware which can help you decide which actual verifier code to use. Notice we mount it in https://github.com/alexa-js/alexa-app/blob/498f6f97dc92e445565e5ddd98444d0bd3a63014/index.js#L8, that can probably be refactored to specify the verifier package name and passed into alexa-app.