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.

Sign up for GitHub

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

added callback specs #176

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

added callback specs #176

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee7e1bd
added callback specs
sgalpha01 Jun 27, 2022
07d8791
Merge branch 'ga4gh:develop' into draft
sgalpha01 Sep 10, 2022
b0f5964
added callback specs
sgalpha01 Sep 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion openapi/task_execution_service.openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,23 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/tesCreateTaskResponse'
callbacks:
statusChange:
'{$request.body#/callback_url}':
Copy link

Choose a reason for hiding this comment

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

This cool feature was mentioned at the GA4GH call, so I took a peek... I'm curious, how is authentication supposed to work for the callback url?

Copy link

Choose a reason for hiding this comment

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

github allows a simple password for webhooks but does not require it. I wonder if the callback should be an object instead of a single url:

"callback": {
  "url": "your url goes here",
  "headers": [
   { "key":"value"}
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here was not to put callback url behind extra authentication (the URL itself might be a secret?) and additionally advise consumers of the callback to verify the information by hitting the GET endpoint.

Copy link

@patmagee patmagee Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@aniewielska thanks for the clarification. I wonder if it would still potentially make sense to make this an object to future proof in the case of wanting to add additional access mechanisms in the future

Copy link

Choose a reason for hiding this comment

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

The idea here was not to put callback url behind extra authentication (the URL itself might be a secret?) and additionally advise consumers of the callback to verify the information by hitting the GET endpoint.

I think it would be nice to have your clarification in the spec.

I'm still slightly concerned about needing to expose a public callback url without auth, and I think Patrick's proposal addresses that, but I don't feel too strongly (and I'm not a reviewer anyway, just lurking :) ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points here, thanks. Agree with future proofing by making it an object and extending the description to clarify that clients should verify the info from the callback and that the callback URL may include a secret

post:
description: |-
The TES server will send notifications to the callback URL, if provided,
when the task status changes.
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/tesCallbackStatus'
responses:
204:
description: |-
Listener should return this HTTP status code if
the payload was received successfully.
x-codegen-request-body-name: body
/tasks/{id}:
get:
Expand Down Expand Up @@ -219,6 +236,19 @@ components:
- FULL

schemas:
tesCallbackStatus:
Copy link

Choose a reason for hiding this comment

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

How will the the callback know which TES server is making the request? Is the callback expected to look at the Origin header (but I don't think that's guaranteed to be set)?

type: object
required:
- id
- state
properties:
id:
type: string
description: Task identifier assigned by the server.
example: job-0012345
state:
$ref: '#/components/schemas/tesState'
description: Notification that a task has changed state.
tesCancelTaskResponse:
type: object
description: CancelTaskResponse describes a response from the CancelTask endpoint.
Expand Down Expand Up @@ -654,6 +684,12 @@ components:
type: string
description: |-
Optional user-provided description of task for documentation purposes.
callback_url:
type: string
format: uri
description: |-
Optional user-provided callback URL.
example: https://myserver.com/send/callback/here
inputs:
type: array
description: |-
Expand Down Expand Up @@ -740,7 +776,7 @@ components:
This is set by the system, not the client.
example: 2020-10-02T10:00:00-05:00
readOnly: true
description: Task describes an instance of a task.
description: Task describes an instance of a task.
tesTaskLog:
required:
- logs
Expand Down