gateway: add api to redelivery a run webhook delivery #456
Conversation
| } | ||
|
|
||
| if runWebhookDelivery.DeliveryStatus == types.DeliveryStatusNotDelivered { | ||
| return util.NewAPIError(util.ErrBadRequest, errors.Errorf("can't redelivery a not delivered runwebhookdelivery")) |
There was a problem hiding this comment.
Is this really necessary? One user could always create tons of new delivery when the state is delivered. Or you should also check that only one redelivery can be created for a specific webhoo.
There was a problem hiding this comment.
So you choosed to let the user create tons of redeliveries?
There was a problem hiding this comment.
I can add a check that only one redelivery can be created for a specific webhook, but I seen in github there is not this limit.
There was a problem hiding this comment.
I'll prefer to let the user create a new redelivery only if the previous redelivery has already been delivered with any outcome (ok or failed).
| return util.NewAPIError(util.ErrForbidden, errors.Errorf("user not authorized")) | ||
| } | ||
|
|
||
| _, err = h.notificationClient.RunWebhookRedelivery(ctx, req.RunWebhookDeliveryID) |
There was a problem hiding this comment.
the gateway api requires the project id but the notification api doesn't require it. You are checking that the user can do the action on the provided project id but you aren't checking that the delivery belongs to that project causing a security issue.
The tests should also cover such case to avoid regressions.
There was a problem hiding this comment.
I can change ta actual redelivery api(notificaiton) to
projects/{projectid}/runwebhookdeliveries/{runwebhookdeliveryid}/redeliverythe notification will return a notexists error if the delivery not belongs to the project.
Notification currently has a single api to retrieve the deliveries by project id. So it makes sense to also do the redelivery by project id.
The tests for both notification and gateway should cover the case of a redelivery id not belonging to the project.
alessandro-sorint
force-pushed
the
redelivery
branch
2 times, most recently
from
e95c5d0
to
4e1b800
Compare
|
@sgotti can you do the review please? |
tests/setup_test.go
Outdated
| } | ||
| ` | ||
|
|
||
| t.Run("test redelivery project run webhook deliveriy with deliverystatus = deliveryError", func(t *testing.T) { |
There was a problem hiding this comment.
deliveriy typos in all tests messages.
| } | ||
|
|
||
| if runWebhookDelivery.DeliveryStatus == types.DeliveryStatusNotDelivered { | ||
| return util.NewAPIError(util.ErrBadRequest, errors.Errorf("can't redelivery a not delivered runwebhookdelivery")) |
There was a problem hiding this comment.
So you choosed to let the user create tons of redeliveries?
alessandro-sorint
force-pushed
the
redelivery
branch
2 times, most recently
from
08e2631
to
1abedd1
Compare
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| if runWebhookDeliveries[0].DeliveryStatus == types.DeliveryStatusNotDelivered { |
There was a problem hiding this comment.
add a comment on the reason why only the first delivery is going to be checked?
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| if runWebhookDeliveries[0].DeliveryStatus == types.DeliveryStatusNotDelivered { |
There was a problem hiding this comment.
also if this won't happen a check on len(runWebhookDeliveries) == 0 should be done.
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| if runWebhookDeliveries[0].DeliveryStatus == types.DeliveryStatusNotDelivered { |
There was a problem hiding this comment.
I added the test test project run webhook redelivery with the last delivery that hasn't been delivered in notification_test.go. If you mean the integration tests I need to add the runWebhookDeliveriesInterval in the notification conf?
There was a problem hiding this comment.
In notification tests you should test that when calling the action with a delivery not yet delivered it should fail with the right error.
For the integration tests it isn't needed since this is already covered by the notification test.
alessandro-sorint
force-pushed
the
redelivery
branch
2 times, most recently
from
326616d
to
a2f98e2
Compare
tests/setup_test.go
Outdated
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| sc := setup(ctx, t, dir, withGitea(true), withWebhooks(webhookURL, "secretkey")) |
There was a problem hiding this comment.
why this "webhookURL" with a fake value used only here?
alessandro-sorint
force-pushed
the
redelivery
branch
from
a2f98e2
to
0b30abe
Compare
tests/setup_test.go
Outdated
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| sc := setup(ctx, t, dir, withGitea(true), withWebhooks("testWebhookURL", webhookSecret)) |
There was a problem hiding this comment.
is this done to make the delivery fail? So, this should be clearer since now it's not documented and the url doesn't explain this.
There was a problem hiding this comment.
Yes it is this done to make the delivery fail. I added a comment and changed the webhookURL value
alessandro-sorint
force-pushed
the
redelivery
branch
from
0b30abe
to
8fe6f6d
Compare
add an api to redelivery a run webhook delivery by project id and run webhook delivery id.
add an api to redelivery a run webhook delivery by project ref and run webhook delivery id.
alessandro-sorint
force-pushed
the
redelivery
branch
from
8fe6f6d
to
b910309
Compare
I changed the notification commit, adding by project id, since the api take projectid |
This patch adds an api to redelivery a run webhook delivery by project ref and run webhook delivery id.
fix #424