-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Feat(webhook_listeners): add auth tokens to webhook call #55790
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
Conversation
e804cd1 to
5b188b6
Compare
julien-nc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From you branch, at the top of server, you can run
composer i
# you might wanna revert what it does to lib/composer
git checkout ./lib/composer/
composer run cs:check ./apps/webhook_listeners/lib
# or if you have Php 8.4 like me and it's refused by php-cs
PHP_CS_FIXER_IGNORE_ENV=1 composer run cs:check ./apps/webhook_listeners/lib
# and if you want to fix the syntax issues:
composer run cs:fix ./apps/webhook_listeners/lib(for later) To implement the token expiration mechanism you might need a new table where you store
- the token ID
- the webhook ID
- if needed, the token creation timestamp
33585c0 to
78ea408
Compare
78ea408 to
8dae570
Compare
8dae570 to
12747b1
Compare
12747b1 to
8572f9a
Compare
|
Who will revoke generated token once it is not needed? Will we revoke it in the server repo or in the Windmill integration app itself? Another question related to previous: If the token was generated for user |
IMO the tokens should be revoked by a background job in webhook_listeners.
I think it's clearer and cleaner if we have independent tokens for each webhook "call". If the token generated for the first run leaks for some reason, it's bad if we make it live longer. |
This definetly should be added as a comment to the code. |
|
Yep, this PR is still WIP. This is planned. |
0c0a98a to
7847d3b
Compare
2d030cc to
f53ad98
Compare
647a122 to
a6c77a7
Compare
0ff523e to
954d099
Compare
959d97d to
be08ce8
Compare
a38ab95 to
5f507e7
Compare
5f507e7 to
c5a2888
Compare
come-nc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nitpick.
| foreach ($tokensToDelete as $token) { | ||
| try { | ||
| $this->tokenMapper->delete($this->tokenMapper->getTokenById($token->getTokenId())); // delete token itself | ||
| $this->deleteByTokenId($token->getTokenId()); // delete db row in webhook_tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $this->deleteByTokenId($token->getTokenId()); // delete db row in webhook_tokens | |
| $this->delete($token); // delete db row in webhook_tokens |
I think we do not need to delete by token id? I think the method deleteByTokenId can even be removed.
On big instances it may matter to delete by the entity id which is more likely to be indexed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, didn't know that this delete method exists magically when writing that. Will change and test, thanks!
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
4042b70 to
694ecce
Compare
|
@AndyScherzinger can you force-merge? Cypress setup not behaving |
Summary
Needed for Windmill integration.
This code adds an option to ask for authentication tokens when registering a webhook. The requested tokens will be added to the dispatched request to the defined endpoint.
All tokens generated with this have a lifetime of 1 hour and will be deleted after that in a background job every 5 min.
Tokens can be requested in the tokenNeeded parameter, which accepts listed values in the two fields "user_ids" and "user_roles".
"user_ids" is a list of user uids for which tokens are needed, "user_roles" is a list of roles (users not defined by their ID but by the role they have in the webhook event) for which tokens can be included. Possible roles: "owner" for the user creating the webhook, "trigger" for the user triggering the webhook call.
Checklist
3. to review, feature component)stable32)