Add authorization header in webhooks stored in secrets table #941
Conversation
Authorizarion wuth capital A and add it in header call only if there is one Co-authored-by: Florent <[email protected]>
|
@CamilleLegeron Probably worth to take a look at the failures, I see some of them are related to Webhooks. |
| @@ -4889,6 +4891,7 @@ function testDocApi() { | |||
|
|
|||
| const expectedFields = { | |||
| url: `${serving.url}/foo`, | |||
| authorization: '', | |||
There was a problem hiding this comment.
Maybe also add a specific tests to ensure that a filled authorization header is passed to the webhook consumer?
There was a problem hiding this comment.
Good remark, adding this test I find a bug when calling patch call : before when we used patch call to update URL without giving information of authorization it removed it
…d for webhooks when using patch call
|
Hi, I send you a notif @paulfitz to know if you can review this PR or maybe other person in Gristlabs |
Co-authored-by: Florent <[email protected]>
| 'tableId', 'isReadyColumn', | ||
| 'watchedColIdsText', 'webhookId', | ||
| 'enabled', 'status' | ||
| 'eventTypes', 'tableId', |
There was a problem hiding this comment.
Can you explain why the order has changed? The comment mentions that the order matters here. Maybe it is outdated.
There was a problem hiding this comment.
Because it's the order on the frontend too and I find this order more logical.
- Having the authorization parameter next to the url
- Having the table id next to the eventTypes
- etc ..
But if you thinks differently we can change it
There was a problem hiding this comment.
No it looks fine. I'm just asking as this change wasn't mentioned anywhere (PR description or issue).
| webhookSecret.url = url; | ||
| // As we want to patch the webhookSecret object, only set the url and the authorization when they are defined. | ||
| // When the user wants to empty the value, we are expected to receive empty strings. | ||
| if (url !== undefined) { |
There was a problem hiding this comment.
2 things:
- I'd convert arguments to an object, as there are too many strings there
- I'd throw an exception when both those things are not defined.
If you take a look at the line 1939 updateSecret method, it will throw an exception if the update won't do anything, but with completely misleading error message - that the secret wasn't found, when in fact it was found but just not updated.
Btw: can you update that error message as well, to something like:
secret with given id not found or nothing was updated.
| 'Filter for changes in these columns (semicolon-separated ids)', | ||
| 'Ready Column', |
There was a problem hiding this comment.
Can you add/update one of this test to update the Authorization column and check that it is persisted ok.
|
Hi @CamilleLegeron, it looks that something went wrong in the merge as there are lots of unrelated files changed. |
Yes in fact, thanks. I saw the tests errors, I put the PR in draft to look at it |
CamilleLegeron
force-pushed
the
webhooks-with-headers
branch
from
02899b1 to
67cb040
Compare
CamilleLegeron
force-pushed
the
webhooks-with-headers
branch
from
d9344fa to
c3ec72d
Compare
Summary: Adding authorization header support for webhooks. Issue: #827 --------- Co-authored-by: Florent <[email protected]>
#827