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

Callback feature #763

Merged
merged 1 commit into from
Sep 30, 2016
Merged

Callback feature #763

merged 1 commit into from
Sep 30, 2016

Conversation

fehguy
Copy link
Contributor

@fehguy fehguy commented Aug 19, 2016

Addresses #716, #735, #736, #737.

To support webhooks, a proposed callback mechanism is added. Essentially, callbacks are out-of-band operations against some remote server. The callback subscription mechanism is not prescribed, as it is actually implementation dependent. The mechanism for responses, however, is created by added a callbacks section to any operation.

The callbacks section has named callbacks, which are essentially operations. The operations can be applied to a URL as extracted from the request options using a syntax similar to with the links technique.

An example below describes the GitHub webhook mechanism, with comments, for discussion.

paths:
  /repos/{owner}/{repo}/hooks/{id}:
    post:
      body: |
        description: Repository service hooks (like email or Campfire) can have at most one configured at a time. Creating hooks for a service that already has one configured will update the existing hook.
        Repositories can have multiple webhooks installed. Each webhook should have a unique config. Multiple webhooks can share the same config as long as those webhooks do not have any events that overlap.
        schema:
          $ref: '#/components/definitions/WebhookRequest'
      responses:
        201:
          description: Webhook created
          schema:
            $ref: '#/components/definitions/WebhookResponse'
      callbacks:
        createWebhook:
          $ref: '#/components/callbacks/createWebhook'
components:
  callbacks:
    createWebhook:
      # extract the url parameter from the request
      '$request.body.url':
        post:
          parameters:
          - in: header
            name: X-GitHub-Event
            type: string
            description: Name of the [event](https://developer.github.com/webhooks/#events) that triggered this delivery.
            required: true
          - in: header
            name: X-Hub-Signature
            type: string
            description: HMAC hex digest of the payload, using the [the hook's `secret`](https://developer.github.com/v3/repos/hooks/#create-a-hook) as the key (if configured).
          - in: header
            name: X-GitHub-Delivery
            type: string
            description: Unique ID for this delivery.
          body:
            name: payload
            schema:
              $ref: '#/components/definitions/Event'
          responses:
            200:
              description: webhook successfully processed an no retries will be performed
  definitions:
    WebhookRequest:
      required:
      - name
      - config
      properties:
        name:
          type: string
          description: Use web for a webhook or use the name of a valid service. (See /hooks for the list of valid service names.)
        config:
          $ref: '#/components/definitions/WebhookConfig'
        events:
          type: array
          items:
            type: string
            enum:
            - push
          default: [push]
          description: Determines what events the hook is triggered for.
        active:
          type: boolean
          description: Determines whether the hook is actually triggered on pushes.
    WebhookResponse:
      properties:
        id:
          type: string
        url:
          type: string
          format: url
        test_url:
          type: string
          format: url
        ping_url:
          type: string
          format: url
        name:
          type: name
        events:
          type: array
          items:
            type: string
            enum:
            - push
            - pull_request
        active:
          type: boolean
        config:
          $ref: '#/components/definitions/WebhookConfig'
        updated_at:
          type: string
          format: date-time
        created_at:
          type: string
          format: date-time
    WebhookConfig:
      type: object
        properties:
          url:
            type: string
            format: url
          content_type:
            type: string
            enum:
            - json
        additionalProperties:
          type: string
          description: Key/value pairs to provide settings for this hook. These settings vary between hooks and some are defined in the [github-services](https://github.com/github/github-services) repository. Booleans are stored internally as "1" for true, and "0" for false. Any JSON true/false values will be converted automatically.
    Event:
      anyOf:
      - $ref: '#/components/definitions/commit_comment'
      - $ref: '#/components/definitions/create'
      - $ref: '#/components/definitions/delete'
      - $ref: '#/components/definitions/deployment'
      - $ref: '#/components/definitions/deployment_status'
      - $ref: '#/components/definitions/fork'
      - $ref: '#/components/definitions/gollum'
      - $ref: '#/components/definitions/issue_comment'
      - $ref: '#/components/definitions/issues'
      - $ref: '#/components/definitions/member'
      - $ref: '#/components/definitions/membership'
      - $ref: '#/components/definitions/page_build'
      - $ref: '#/components/definitions/public'
      - $ref: '#/components/definitions/pull_request_review_comment'
      - $ref: '#/components/definitions/pull_request'
      - $ref: '#/components/definitions/push'
      - $ref: '#/components/definitions/repository'
      - $ref: '#/components/definitions/release'
      - $ref: '#/components/definitions/status'
      - $ref: '#/components/definitions/team_add'
      - $ref: '#/components/definitions/watch'

@fehguy
Copy link
Contributor Author

fehguy commented Aug 19, 2016

@OAI/tdc for discussion today ^^

@@ -1050,6 +1051,36 @@ Response with no return value:
description: object created
```

#### <a name="callbackObject"></a>Callback Object

A container for possible out-of band callbacks from an operation. A callback may be returned from an operation, calling back to the path specified in the operation object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

»A callback may be returned from an operation« sounds wrong. Maybe better »A callback may be caused/triggered by an operation«?

@ePaul
Copy link
Contributor

ePaul commented Aug 19, 2016

This $request.body.url syntax doesn't seem to be defined. Something similar is there for the variable substitution in the link object, but there it is in {...}. I suggest unifying this (and defining it a bit clearer).

@@ -1050,6 +1051,36 @@ Response with no return value:
description: object created
```

#### <a name="callbackObject"></a>Callback Object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called here »Callback Object«, but »Callback Responses Object« in the Operation object.

@DavidBiesack
Copy link

in the example, I think X-Hub-Signature should be X-GitHub-Signature

@DavidBiesack
Copy link

The proposal should address security considerations -- can a man in the middle or other attack alter the callback URL in the request in order to redirect sensitive data?

@fehguy
Copy link
Contributor Author

fehguy commented Aug 19, 2016

@DavidBiesack I'm not sure how you think this proposal introduces any security issues? Please explain.

@fehguy
Copy link
Contributor Author

fehguy commented Aug 19, 2016

@OAI/tdc please review and add your feedback. Would be great to get any other examples or feedback on this structure.

@darrelmiller
Copy link
Member

I'm all 👍 on this.

@RobPhippen
Copy link

RobPhippen commented Aug 23, 2016

A few comments;

  • Although it's 'just a name' (and so could be any valid string, I assume) the name createWebHook as the identifier/name for the callback seemed odd to me. I wanted to log this in case there is a deeper reason for the name to be what it is
    • I might have used the term eventCallback or gitHubEventHook
  • Setting aside the syntax used in '$request.body.url':, its placement in the structure of the document leads me to think that the entire callback is part of a structure that hangs off this item.
    • If this interpretation is correct, I think this means that this update does not model 'standalone' webhooks declarations.
    • In the last OAI TDC discussion (that I was on) we spent some time discussing that it is useful and necessary to enable this. As I recall, the proposal was to allow callback declarations to exist within the components section without needing to be attached to a subscription API
    • In your comment on 716 you had syntax like this
callbacks:
      # callbacks are sent to the url provided
      # this could also be a query param, etc.
      url: '$request.url'
      schema:
        type: object
        properties:
          a:
            type: string

... which could allow WebHooks to be specified standalone by allowing url: to be optional.

  • Discriminating between events: the anyOf: does a nice job of enumerating all of the different event structures that could occur. Can we also provide a way of specifying that X-GitHub-Event provides a discriminator?
  • On the syntax of '$request.body.url':, if I've understood the semantics, and checking the schemas, I think this should be '$request.body.config.url:'

@RobPhippen
Copy link

Hi @DavidBiesack github does in fact use x-hub-signature for this specific item;
https://developer.github.com/webhooks/securing/

@RobPhippen
Copy link

One more thought; could we add an element something like;

   subscriptionList: '$request.body.config.events'

Which uses your syntax to make it clear which element of the webhooks subscription interaction specifies the list of events required.

@whitlockjc
Copy link
Member

This implementation doesn't seem anything like what I'm see elsewhere. I think calling things what they are in the spec, webhooks or events, make more sense than callbacks. To me, I'd expect to define a set of events and then dictate which operations emitted those events. Each event would describe the payload sent via the webhook and the expected response to return.

Here is an example of GitHub's Webhook documentation: https://developer.github.com/webhooks/ I realize this feedback might be a little late.

@mhailstone
Copy link

mhailstone commented Aug 26, 2016

Here is a portion of our Event Hub definition updated with this callback proposal:

{
  "swagger": "3.0",
  "info": {
    "title": "Event Hub",
    "version": "2.0.0"
  },
  "paths": {
    "/webhooks": {
      "post": {
        "summary": "Register webhook",
        "tags": [
          "webhooks"
        ],
        "description": "Register webhook",
        "operationId": "registerWebhook",
        "parameters": [
          {
            "schema": {
              "$ref": "#/definitions/webhook_register"
            },
            "description": "Request Body",
            "name": "body",
            "required": true,
            "in": "body"
          }
        ],
        "responses": {
          "201": {
            "description": "Event raised"
          },
          "400": {
            "description": "Invalid Request - unable to interpret request"
          },
          "401": {
            "description": "Authentication required to add a webhook"
          },
          "403": {
            "description": "Access denied - not authorized to add a webhook"
          },
          "409": {
            "description": "Body of the request contains conflicting, malformed, or invalid data"
          }
        },
        "callbacks":{
          "webhook": {
            "$ref": "#/components/callbacks/webhook"
          }
        }
      }
    }
  },
  "components": {
    "callbacks": {
      "webhook": {
        "$request.body.webhook.endpoint": {
          "post": {
            "parameters": [
              {
                "in": "header",
                "name": "X-Byu-Eventhub-Hmac-Md5",
                "type": "string",
                "required": "true"
              }
            ],
            "body": {
              "name": "event",
              "schema": {
                "$ref": "#/definitions/events"
              }
            },
            "responses": {
              "200": {
                "description": "Succesfully invoked webhook"
              }
            }
          }
        }
      }
    }
  },
  "definitions": {
    "webhook_item": {
      "properties": {
        "identity_id": {
          "type": "string"
        },
        "security_option": {
          "type": "string"
        },
        "identity_name": {
          "type": "string"
        },
        "push_option": {
          "type": "string"
        },
        "content_type": {
          "type": "string"
        },
        "endpoint": {
          "type": "string"
        },
        "security_key": {
          "type": "string"
        }
      },
      "required": [
        "identity_id",
        "identity_name",
        "endpoint",
        "push_option",
        "security_option",
        "content_type"
      ],
      "type": "object"
    },
    "webhook_register": {
      "properties": {
        "webhook": {
          "$ref": "#/definitions/webhook_item"
        }
      },
      "required": [
        "webhook"
      ],
      "type": "object"
    },
    "events": {
      "properties": {
        "events": {
          "$ref": "#/definitions/event"
        }
      },
      "required": [
        "events"
      ],
      "type": "object"
    },
    "event": {
      "properties": {
        "event": {
          "$ref": "#/definitions/event_array"
        }
      },
      "required": [
        "event"
      ],
      "type": "object"
    },
    "event_array": {
      "items": {
        "$ref": "#/definitions/event_item"
      },
      "type": "array"
    },
    "event_item": {
      "properties": {
        "history": {
          "$ref": "#/definitions/history"
        },
        "event_header": {
          "$ref": "#/definitions/event_header"
        },
        "event_body": {
          "$ref": "#/definitions/event_body"
        },
        "filters": {
          "$ref": "#/definitions/filters"
        }
      },
      "required": [
        "event_header",
        "filters",
        "event_body",
        "history"
      ],
      "type": "object"
    },
  }
}

@fehguy fehguy merged commit b1430f9 into OpenAPI.next Sep 30, 2016
@fehguy fehguy deleted the feature/callbacks branch October 20, 2016 17:12
AndersDJohnson pushed a commit to AndersDJohnson/OpenAPI-Specification that referenced this pull request Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants