-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handle accept decline with invalid share id #34622
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34622 +/- ##
============================================
+ Coverage 65.23% 65.24% +<.01%
- Complexity 18438 18446 +8
============================================
Files 1203 1203
Lines 69819 69850 +31
Branches 1280 1280
============================================
+ Hits 45548 45572 +24
- Misses 23899 23906 +7
Partials 372 372
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #34622 +/- ##
============================================
+ Coverage 65.23% 65.24% +<.01%
- Complexity 18438 18446 +8
============================================
Files 1203 1203
Lines 69819 69850 +31
Branches 1280 1280
============================================
+ Hits 45548 45572 +24
- Misses 23899 23906 +7
Partials 372 372
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #34622 +/- ##
============================================
+ Coverage 65.25% 65.25% +<.01%
- Complexity 18458 18467 +9
============================================
Files 1207 1207
Lines 69895 69928 +33
Branches 1280 1280
============================================
+ Hits 45608 45633 +25
- Misses 23915 23923 +8
Partials 372 372
Continue to review full report at Codecov.
|
the code looks fine to me, I'm only worried about the STATUS_GONE addition. @VicDeo can you evaluate the risk for this change of behavior for existing federation clients that might have expected a 200 OK response with no body instead of an error ? |
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.
That's what we have from OCM spec https://rawgit.com/GEANT/OCM-API/v1/docs.html
400 Bad request due to invalid parameters, e.g. when type is invalid or missing.
401 Client cannot be authenticated as a trusted service.
403 Trusted service is not authorized to create notifications.
501 The receiver doesn't support notifications.
503 The receiver is temporary unavailable (e.g. due to planned maintenance).
As long as RequestHandlerController is used for our legacy endpoint it should be fine.
@PVince81 according to the implementation this notification will be resent from a background job with TTL = 20. |
@VicDeo the list does not have code 410. |
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.
Fine by me. Just one note.
use OCP\Lock\ILockingProvider; | ||
use OCP\Lock\LockedException; | ||
use OCP\Share; | ||
use function Sodium\crypto_sign_ed25519_pk_to_curve25519; |
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.
@phil-davis strange things are going on here :)
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.
taken care of .... will merge once drone is green and take care of backporting
5050903
to
c0926c7
Compare
😱 |
@DeepDiver1975 feel free to keep rebasing... this PR and/or just cherry-pick the useful bits into #34568 or whatever. (I will keep my hands off, so we don't stomp on each other) |
ohoh ....
|
2afc28e
to
275cf43
Compare
@DeepDiver1975 there is a fail with the |
275cf43
to
c8b1941
Compare
The code looks fine. Weird drone error and we can't see the log file. I force pushed a rebase of the last commit to make drone start fresh. |
Running
these scenarios fail:
and
and
and
and
It is not happy. |
c8b1941
to
c05c178
Compare
Rebased - we can see drone logs today, so that will give us test output to look at. |
c05c178
to
96f66cf
Compare
Acceptance tests revealed another issue ... fixed |
Thx |
I am looking at the backport... |
@DeepDiver1975 were #34664 and #34679 also addressed ? |
Backport |
Description
Catch the case when an accept or decline for a non-existent federated share id is received. Return a
STATUS_GONE
410
response. Previously this was not caught inRequestHandlerController
and ended up becoming a500
status. That can cause issues for the remote end, which could think that there was an internal server error for the request - the remote end might then throw an exception itself and not cleanup its view of the now non-existent share id.Catch the case when an accept or decline for a share id has an invalid shared secret provided. Return a
STATUS_FORBIDDEN
403
so that the remote end can at least handle/report that, rather than returning a500
that might cause the remote server grief.Related Issue
This is on top of PR #34568 and issue #34566
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
Open tasks: