Files Sharing API breaks in Nc 28+ #197
Comments
|
@aleixq, can you reproduce this? |
|
#200 is even worse... 😬 |
|
So it seems that parameters changed... again cat-and-mouse... the cost of living behind extends OCA\Files_Sharing\Controller\ShareAPIController .
seems to solve this issue. Also, I think that the way @pbek unpacks args will solve future changes. Do you need a patch for this minor change or better bet for your workaround? |
|
I released 8e619fe, so best test version 24.6.0 first. |
|
Upgraded to 24.6.0 and also slowly upgraded Nextcloud versions from 26 to 29.0.2 locally. The app and sharing worked until v28, but sharing won't work with v29.0.2. Same error as before, somehow the 'currentuser' variable seems to be empty. I think I was wrong about claiming that file sharing wasn't working on NC v28. My local NC v28 still used webapppassword 24.4.0 and it was fine. I suppose everything just broke with NC 29, like you mentioned in the other issue. |
|
I'll check it out ASAP. By now, Could you please check in browser console if is failing the preflight OPTIONS request, when trying to query or posting things to webappassword share api endpoint? |
|
Yes, it's the OPTIONS part of the request that is failing. The error in the console is the standard CORS error. The request itself is a POST to http://localhost:8080/index.php/apps/webapppassword/api/v1/shares with a shareType, path, expireDate and password in the body. |
|
If it's about nextcloud/server@5dbb96f, those changes will also be backported to Nextcloud 28, but they are not released yet. |
|
I had inspected a little more, and I suspect that there are some issues there. Webapppassword Share api is not working in 29 too, with this error (in nextcloud logs): 2 - After solving the first error there is this Reflector {"file":"web/index.php","line":49,"function":"handleRequest","class":"OC","type":"::"}],"File":"web/lib/private/AppFramework/Utility/ControllerMethodReflector.php","Line":101,"message":"Internal error: Failed to retrieve the default value","exception":{},"CustomMessage":"Internal error: Failed to retrieve the default value"}}{
"reqId": "qMEVVEzV1AgBLr6zKrFT",
"level": 3,
"time": "2024-06-24T19:53:10+00:00",
"remoteAddr": "x.x.x.x",
"user": "someuser",
"app": "index",
"method": "POST",
"url": "/index.php/apps/webapppassword/api/v1/shares",
"message": "Internal error: Failed to retrieve the default value",
"userAgent": "Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0",
"version": "29.0.2.2",
"exception": {
"Exception": "ReflectionException",
"Message": "Internal error: Failed to retrieve the default value",
"Code": 0,
"Trace": [
{
"file": "web/lib/private/AppFramework/Utility/ControllerMethodReflector.php",
"line": 101,
"function": "getDefaultValue",
"class": "ReflectionParameter",
"type": "->"
},
{
"file": "web/lib/private/AppFramework/Http/Dispatcher.php",
"line": 128,
"function": "reflect",
"class": "OC\\AppFramework\\Utility\\ControllerMethodReflector",
"type": "->"
},
{
"file": "web/lib/private/AppFramework/App.php",
"line": 184,
"function": "dispatch",
"class": "OC\\AppFramework\\Http\\Dispatcher",
"type": "->"
},
{
"file": "web/lib/private/Route/Router.php",
"line": 338,
"function": "main",
"class": "OC\\AppFramework\\App",
"type": "::"
},
{
"file": "web/lib/base.php",
"line": 1050,
"function": "match",
"class": "OC\\Route\\Router",
"type": "->"
},
{
"file": "web/index.php",
"line": 49,
"function": "handleRequest",
"class": "OC",
"type": "::"
}
],
"File": "web/lib/private/AppFramework/Utility/ControllerMethodReflector.php",
"Line": 101,
"message": "Internal error: Failed to retrieve the default value",
"exception": {},
"CustomMessage": "Internal error: Failed to retrieve the default value"
}
}So second issue is about args unpacking, it is introduced with last @pbek commit... Maybe this could be solved more elegantly to avoid parameters changes in the future, but if I declare again the parameters and call the parent method with the same parameters it runs ok... So finally if I patch with next changes it runs as it should: diff --git a/lib/Controller/ShareAPIController.php b/lib/Controller/ShareAPIController.php
index 43b2863..e8d1572 100644
--- a/lib/Controller/ShareAPIController.php
+++ b/lib/Controller/ShareAPIController.php
@@ -11,32 +11,56 @@ use OCP\AppFramework\Http\DataResponse;
class ShareAPIController extends FilesSharingShareAPIController
{
- use AccessControl;
-
- /**
- * @NoAdminRequired
- *
- * @param string $path
- * @param int $permissions
- * @param string $shareWith
- * @param string $sendPasswordByTalk
- * @param string $attributes
- *
- * @throws NotFoundException
- * @throws OCSBadRequestException
- * @throws OCSException
- * @throws OCSForbiddenException
- * @throws OCSNotFoundException
- * @throws InvalidPathException
- *
- * @suppress PhanUndeclaredClassMethod
- */
- public function createShare(...$args
- ): DataResponse {
- $response = parent::createShare(...$args);
-
- return $this->checkOrigin($response);
- }
+ use AccessControl;
+ private $currentUser;
+
+ /**
+ * @NoAdminRequired
+ *
+ * @param string $path
+ * @param int $permissions
+ * @param string $shareWith
+ * @param string $sendPasswordByTalk
+ * @param string $attributes
+ *
+ * @throws NotFoundException
+ * @throws OCSBadRequestException
+ * @throws OCSException
+ * @throws OCSForbiddenException
+ * @throws OCSNotFoundException
+ * @throws InvalidPathException
+ *
+ * @suppress PhanUndeclaredClassMethod
+ */
+ public function createShare(
+ ?string $path = null,
+ ?int $permissions = null,
+ int $shareType = -1,
+ ?string $shareWith = null,
+ string $publicUpload = 'false',
+ string $password = '',
+ ?string $sendPasswordByTalk = null,
+ ?string $expireDate = null,
+ string $note = '',
+ string $label = '',
+ ?string $attributes = null
+ ): DataResponse {
+ $response = parent::createShare(
+ $path,
+ $permissions,
+ $shareType,
+ $shareWith,
+ $publicUpload,
+ $password,
+ $sendPasswordByTalk,
+ $expireDate,
+ $note,
+ $label,
+ $attributes
+ );
+
+ return $this->checkOrigin($response);
+ }
/**
* The getShares function.
|
|
You can create a PR, but it needs to work with both Nextcloud 29.0.1 and 29.0.2 (and the same for NC 28 and NC 27)! |
|
Any news on this issue? |
|
Hi @DanRiess , I am a little bit confused about how to proceed. I could propose conditions to make it work with 27 28 and 29 , but sincerely It's not a future-proof... I am waiting for an answer in upstream : nextcloud/server#46081 (or proposals here) to decide. I don't know if you understand all the context of this issue about public controllers, cors,... , so maybe it's difficult for you, but maybe you could ask about news there. For the long-long-term solution, I am still convinced that nextcloud may support a flexible way to manage cors to allow public file sharing or public preview api, so please participate in nextcloud/server#37716 (and its PR in nextcloud/server#37896 ) to explain your use case. The more use cases that are detected and explained, the more it will be seen if it is useful to be incorporated into the core. |
|
Hey @aleixq, excuse the late reply. I only have a high level understanding of Nextcloud internals. The way I understand is that Nextcloud's ShareAPIController requires a valid user (even though this variable is typed as string|undefined). Which makes sense I guess, because someone needs to share it. So in the current implementation, Nextcloud's ShareAPIController for some reason cannot extract the currentuser anymore, even though we are using a webapppassword bearer token, is that it? If you have a monkey patch that works as a short term solution for this problem, I'd be happy if you could release that for now. Sharing functionality is quite important for my company's app. Maybe we could figure out a long term solution, if sharing breaks again in future NC releases ;D |
|
@DanRiess there will be different patches depending on the nc version, I can tell you how to proceed if you tell me which nc are you using.
|
|
We are currently using 29.0.4. |
|
This bug is critical. I am frustrated about more and more basic features in NC are broken or not supported anymore. From the business side I must say that NC one can not trust anymore. |
|
Hey @pbek, I did some digging and I do have a couple more insights. I also have a fix that I can provide as a PR, but I do need some guidance regarding the following points.
Check out this table to see which WAP version works for which NC version that I have tested
As you can see, this is kind of annoying and it's probably impossible to support every minor NC version. In case you're ok with supporting only the latest minor versions, I'd suggest to release a new WAP version that supports NC versions from 22 to 26 and uses the code from WAP 24.4, and then release another WAP version (maybe 25?) that supports NC from v27 and up and uses the code from WAP 24.6 with my changes. Let me know what you think. |
|
Thank you, you were very diligent in your testing! |
Ok, but surely not in WebAppPassword 24.4, right (see #200)? |
|
It did work for me in 24.4 as well, which is strange. Took a look at the error in #200 and could not reproduce it. For me, this only occured when I specifically called the sharing api. If you are experiencing this, you would also experience it with my fix, because the expected type from expireDate is '' in v27.0.0 and null in 27.1.11. In that case, I suppose we would need to accept that file sharing will be broken in v27 and v28, which WAP 24.6 should support. Then, with v29, we could deploy my fix, as the expected type should now be the same for all minor versions. |
I was just using the password API. I would not really be able to test a PR with Nextcloud 27.1.10.2. And I'm very hesitant to drop support for Nextcloud < 29 with the next release of webapppassword just for the sake of the file sharing API, because this would mean I can't even support my own employer. 🤓 The file sharing and DAV API were never in the scope of webapppassword anyway. 🤔 |
|
The only other solution I see is to query the current NC version in the createShare function and then define the necessary default value. Would be ugly as hell but enable you to keep supporting previous versions. I'd need you to test this though, since apparently this error doesn't trigger in my NC setup. I really don't understand why changes like that get backported... |
|
Update: My bad, I thought the app breaking error from #200 occurred your university's live version, 27. I just saw that you tested with v29, and there, I could reproduce it as well. I hadn't tested this properly, since I already knew that the filesharing didn't work. Sorry about that. I'll provide an updated table. I also included NC 27.1.10.2.
The X means only file sharing doesn't work, but password generation does. So, in conclusion:
I believe my patch only brings benefits at this point. You won't have to drop support for older NC versions and it enables file sharing for all current NC instances. Is that acceptable? |
|
Ok, great then. 👍️ Can you please create a PR? |
|
Thanks for your work, ¿could i test @DanRiess patch to double check before committing to main branch? |
* Changed share api controller to re-enable sharing on recent NC versions while keeping the functionality to retrieve a password in any NC version. * Revert "Changed share api controller to re-enable sharing on recent NC versions while keeping the functionality to retrieve a password in any NC version." This reverts commit bb4f0df. * Changed some comments removed the svg from the commit
Signed-off-by: Patrizio Bekerle <[email protected]>
|
There now is a new release, could you please test it and report if it works for you? |
|
I have tested it and it works fine! Thanks for merging. |
|
Things are moving on nextcloud/server#40537 🚀 |
|
Hey, that's awesome! |
I don't know if there is a specific date. Usually a new release is done every six month. And I don't know when / if things are backported. |
Explain the Problem
There is a related issue with pretty much the exact same problem, but it was last active a year ago, so I figured I open a new one. The files sharing API seems to be broken in Nextcloud Version 28 and 29 and throws CORS errors. Authorized file access is not affected and works as intended.
The files sharing API works fine in my local 26 version and it used to work in our production environment until we upgraded to v28. I'm not 100% sure however if our live Nextcloud was upgraded over time or all at once to v28 at some point, so there's a slight chance the breaking change might have been introduced in v27 instead.
Steps to Reproduce
I used the test suite that @aleixq has provided in the related issue about CORS errors in files sharing (https://gitlab.com/communia/nc-webapppassword-share-test).
I created the required test file and added the origin to both webdav access and files sharing in the settings.
The webdav access outputs directory metadata, the share test fails. The other 2 cases fail as expected.
In my local environment with NC 26, the share test passes.
Contents of nextcloud/data/nextcloud.log
{ "reqId":"IX66HIjeyKVHhi4y9ff7", "level":3,"time":"2024-06-12T09:45:25+00:00", "remoteAddr":"172.17.0.1", "user":"--", "app":"index", "method":"OPTIONS", "url":"/index.php/apps/webapppassword/api/v1/shares", "message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string", "userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36", "version":"29.0.1.1", "exception":{"Exception":"TypeError","Message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string","Code":0,"Trace":[{"function":"__construct","class":"OCA\\Files_Sharing\\Controller\\ShareAPIController","type":"->","args":["webapppassword",["OC\\AppFramework\\Http\\Request"],["OC\\Share20\\Manager"],["OC\\Group\\Manager"],["OC\\User\\Manager"],["OC\\Files\\Node\\LazyRoot"],["OC\\URLGenerator",["OC\\User\\Session"]],["OC\\L10N\\LazyL10N"],["OC\\AllConfig"],["OC\\App\\AppManager"],["OC\\AppFramework\\DependencyInjection\\DIContainer"],["OC\\UserStatus\\Manager"],["OC\\PreviewManager"],["OC\\DateTimeZone"],["OC\\AppFramework\\ScopedPsrLogger"],null]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":83,"function":"newInstanceArgs","class":"ReflectionClass","type":"->","args":[["webapppassword",["OC\\AppFramework\\Http\\Request"],["OC\\Share20\\Manager"],["OC\\Group\\Manager"],["OC\\User\\Manager"],"And 11 more entries, set log level to debug to see all entries"]]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":128,"function":"buildClass","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":[["ReflectionClass","OCA\\WebAppPassword\\Controller\\ShareAPIController"]]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":146,"function":"resolve","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":470,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":442,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/App.php","line":163,"function":"query","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/Route/Router.php","line":338,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController","preflightedCors",["OC\\AppFramework\\DependencyInjection\\DIContainer"],["webapppassword.shareapi.preflighted_cors"]]},{"file":"/var/www/html/lib/base.php","line":1050,"function":"match","class":"OC\\Route\\Router","type":"->","args":["/apps/webapppassword/api/v1/shares"]},{"file":"/var/www/html/index.php","line":49,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"/var/www/html/apps/files_sharing/lib/Controller/ShareAPIController.php","Line":124,"message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string","exception":{},"CustomMessage":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string"}}Could you guys please check if this is reproducible on your end?
Thanks,
Dan
The text was updated successfully, but these errors were encountered: