search

digital-blueprint / webapppassword

Nextcloud app to generate temporary app passwords and allow webdav access for SPAs
https://github.com/digital-blueprint/webapppassword
GNU Affero General Public License v3.0
17 stars 6 forks source link

Files Sharing API breaks in Nc 28+ #197

Closed DanRiess closed 3 weeks ago

DanRiess commented 2 months ago

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

pbek commented 2 months ago

@aleixq, can you reproduce this? You can also spin up a Nextcloud instance with https://github.com/digital-blueprint/webapppassword/tree/main/docker.

pbek commented 2 months ago

https://github.com/digital-blueprint/webapppassword/issues/200 is even worse... 😬

aleixq commented 2 months ago

So it seems that parameters changed... again cat-and-mouse... the cost of living behind extends OCA\Files_Sharing\Controller\ShareAPIController . adding:

string $expireDate = null,

in https://github.com/digital-blueprint/webapppassword/blob/v24.1.1/lib/Controller/ShareAPIController.php#L42

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?

pbek commented 2 months ago

I released https://github.com/digital-blueprint/webapppassword/commit/8e619fe26d6409dceb0d23902a075dfed92c83c1, so best test version 24.6.0 first.

DanRiess commented 2 months ago

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.

aleixq commented 2 months ago

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?

DanRiess commented 2 months ago

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.

pbek commented 2 months ago

If it's about https://github.com/nextcloud/server/commit/5dbb96fc8a64fb6469aa085b27f80338e665538e, those changes will also be backported to Nextcloud 28, but they are not released yet.

aleixq commented 2 months ago

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): 1- The share api currentuser property type issues https://github.com/nextcloud/server/issues/46081 . It could be workarounded redeclaring the property in our class and removing the type : private $currentUser;

2 - After solving the first error there is this Reflector Internal error: Failed to retrieve the default value error:

{"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.
pbek commented 2 months ago

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)!

DanRiess commented 2 months ago

Any news on this issue?

aleixq commented 2 months ago

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 : https://github.com/nextcloud/server/issues/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 https://github.com/nextcloud/server/issues/37716 (and its PR in https://github.com/nextcloud/server/pull/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.

DanRiess commented 1 month ago

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

aleixq commented 1 month ago

@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.

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 commented 1 month ago

We are currently using 29.0.4.

Madrich commented 1 month ago

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.

DanRiess commented 1 month ago

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.

1) The issue that has started in NC29 has now been backported to v28 and v27, as you had previously mentioned already. This means that it is now impossible to support for example v27.0.0 and v27.1.11 in the same webapppassword version.

2) As @aleixq has mentioned, the issue stems from a type change to the currentUser variable in NC's own ShareAPIController. I don't know exactly how NC initializes sessions under the hood, but if you send an OPTIONS request (which cannot have an Auth header), NC is unable to extract a user variable, currentUser will be null and the type error is thrown. The workaround I have come up with is to add a constructor to your ShareAPIController that enforces userId to be a string (empty if it can't be extracted from the session) and then call the parent constructor with this. An empty string is acceptable to the parent controller and will resolve.

3) The changes you made to the 'createShare' function with the argument unpacking breaks file sharing on every single NC version that I have tested with a ReflectorError.

Check out this table to see which WAP version works for which NC version that I have tested

WAP 24.4 WAP 24.6 My changes to WAP 24.6
NC 26 (all minor versions) works X X
NC 27.0.0 works X X
NC 27.1.11 X X works
NC 28.0.0 works X X
NC 28.0.8 X X works
NC 29 X X works

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.

pbek commented 1 month ago

Thank you, you were very diligent in your testing! What do the X mean? "The sharing API doesn't work" or "The whole app is broken" (#200)? Because the latter must not happen, because people are relying on the app (e.g. TU Graz is at Nextcloud 27.1.10.2)!

pbek commented 1 month ago

I could still grab a password and use /remote.php/dav/files endpoints with it.

Ok, but surely not in WebAppPassword 24.4, right (see #200)?

DanRiess commented 1 month ago

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.

pbek commented 1 month ago

Took a look at the error in https://github.com/digital-blueprint/webapppassword/issues/200 and could not reproduce it. For me, this only occured when I specifically called the sharing api.

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. 🤔

DanRiess commented 1 month ago

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...

DanRiess commented 1 month ago

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.

WAP 24.4 WAP 24.6 WAP 24.6 with my changes
NC 26 works X X
NC 27.0.0 works X X
NC 27.1.10.2 X X works
NC 27.1.11 X X works
NC 28.0.0 works X X
NC 28.0.8 X X works
NC 29 breaks entire app! X works

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?

pbek commented 1 month ago

Ok, great then. 👍️ Can you please create a PR?

aleixq commented 1 month ago

Thanks for your work, ¿could i test @DanRiess patch to double check before committing to main branch?

pbek commented 1 month ago

@aleixq, yes, please test https://github.com/digital-blueprint/webapppassword/pull/214 😉

pbek commented 3 weeks ago

24.8.0

pbek commented 3 weeks ago

There now is a new release, could you please test it and report if it works for you?

DanRiess commented 3 weeks ago

I have tested it and it works fine! Thanks for merging. Aleix has already mentioned that Nextcloud will change the controller constructor arguments in version 30 again. I'm not sure if that will break your entire app or just the file sharing. A few days before v30 gets released, webapppasswords file sharing api should be updated to deal with this. Do you by any chance know their release dates and the version numbers for the backported versions?

aleixq commented 3 weeks ago

Things are moving on https://github.com/nextcloud/server/pull/40537 :rocket:

pbek commented 3 weeks ago

Hey, that's awesome!

pbek commented 3 weeks ago

Do you by any chance know their release dates and the version numbers for the backported versions?

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.