fixed oauth custom expressReceiver bugs

[stevengill]

Summary

When declaring a custom ExpressReceiver, OAuth should still be handled correctly if it exists.

This fixes two bugs:

Firstly, the following code was successfully getting through our if statement check to see if the Oauth installer exists on the receiver. Which in this case it would not as clientID, clientSecret and stateSecret aren't being passed to ExpressReceiver

const expressReceiver = new ExpressReceiver({ 
  signingSecret: '',
})

const app = new App({
  receiver: expressReceiver,
  clientId: '',
  clientSecret: '',
  stateSecret: '',
})

Second bug is that we weren't properly supporting an extended ExpressReceiver. The following code was failing in the same If statement. In this case, it was failing because clientId, clientSecret and stateSecret were all undefined in App since they were passed directly to ExpressReceiver.

const expressReceiver = new ExpressReceiver({ 
  signingSecret: '',
  clientId: '',
  clientSecret: '',
  stateSecret: '',
})

const app = new App({
  receiver: expressReceiver,
})

This PR fixes both cases above and theoretically should allow any custom receivers to implement their own OAuth if they follow the OAuth interface exposed in ExpressReceiver.

Requirements (place an x in each [ ])

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[stevengill]

Thanks @marks for reporting these to me!

[aoberoi]

i think this is a great idea! it might be helpful at some later point to document what the requirements are for a custom receiver that intends to offer installation support. right now, i think the expectation is "the receiver must have an InstallationProvider instance at the property named .installer", which is good. but it might be worth generalizing even more.

[aoberoi]

just trying to play out a scenario in my head: let's say someone passes in a custom receiver that happens to have some property called installer, which also has some property called authorize that has nothing to do with installation support in the way that we describe, what happens?

are we intentionally leaving the implementation of those properties open to developers that bring their own custom receiver? or, do we expect those developers to use our specific InstallationProvider class (which has no dependencies on any framework, since it only uses node's req and res)? if the latter is true, then could we settle for a more strict check, maybe typeof this.receiver.installer === 'InstallProvider'?

[stevengill]

Yeah I was worried that people could 'fake' their way through these checks. Tying it to actually use InstallProvider is a good way to ensure it is using the OAuth library and functions we expect under the hood.

I guess the question here is are we okay with folks implementing OAuth without our OAuth package?

I'm going to merge this in for now but I think it is worth discussing.