Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callbacks #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

michaelmcandrew
Copy link
Contributor

@michaelmcandrew michaelmcandrew commented Mar 12, 2018

This PR adds callback handling to CIviProxy.

Callbacks should be configured in the third party with a URL in the proxy in the following format:

{$proxy_base}/callback.php?source=CALLBACK_KEY&secret=CALLBACK_SECRET

Where:

  • CALLBACK_KEY is a key of an array in the $callbacks array that defines a callback
  • and CALLBACK_SECRET is the value of the secret key in the defined callback

Other validations you can define include

  • The request method
  • The expected content type
  • The body (not yet implemented)

Some notes on the implementation:

  1. The callback feature is turned on by setting $callbacks_enabled to true. This is a slightly different mechanism to other CiviProxy features that are turned on and off by the presence of a target url. I didn't think that made sense in this case as callbacks define multiple target urls. I appreciate that this isn't great for usability. It might be better to refactor how features are turned on and off but I didn't want to go there.

  2. There is a civiproxy_callback_validate_body() function that could be used to validate the payload. I haven't implemented this but can see that you might want to if you are very security conscious. Not sure what the best way to validate that would be. Some form of json schema validation might work for most json based callbacks, though that would involved the inclusion of another library, and might get complicated if the service is posting lots of different flavours of callback. Though this could be handled in a similar way to $rest_allowed_actions where we define a series of definitions per callback. Another approach would be to allow users to define (or select a predefined) extra validation functions for callbacks.

  3. At the moment, the only supported request methods are GET and POST. I think this covers 99% of callbacks thought I guess some might want to use DELETE or PUT or something else. We can extend it if so.

  4. At the moment, I am using my own redirect functions specifically for callbacks - one for POST and one for GET. I wrote these because the civiproxy_redirect() did not handle the body of post requests (even when the method used was POST). @systopia - you may want to refactor civiproxy_redirect so it can handle the body of post requests. Up to you how you want to handle it.

  5. The CALLBACK_SECRET is another layer of security that I have implemented to allow the proxy to fail early. This might interfere with callbacks that pass data via query params but in my experience these are few and far between (the CiviCRM rest API being a notable exception to the rule, and not good practice IMO, but nothing we have to worry about here since it is handled separately by the rest target).

  6. Note that I did not add any IP restrictions. In my experience, these services are operating at 'cloud scale' and do not guarantee an originating IP. Instead they use methods like addings secrets/tokens to the http headers, etc.

bjendres added a commit that referenced this pull request Mar 21, 2018
@bjendres bjendres added this to the 0.6 milestone Mar 21, 2018
@bjendres bjendres mentioned this pull request Mar 21, 2018
@bjendres bjendres mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants