server icon indicating copy to clipboard operation
server copied to clipboard

[Bug]: ShareApiController currentuser property type issue

Open aleixq opened this issue 1 year ago • 4 comments

⚠️ This issue respects the following points: ⚠️

Bug description

In https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/Controller/ShareAPIController.php#L63C2-L63C30 the currentUser property is typed as string, but in constructor the parameter is signed as ?string $userId = null , so in some circumstances there is this error: Exception":"TypeError, Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string

Steps to reproduce

  1. It happens when share api is used via webapppassword in nc 29

Expected behavior

Allow userId to be null when using createShare method setting the property type the same as the parameter type.

Installation method

None

Nextcloud Server version

29

Operating system

None

PHP engine version

None

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

  • [ ] Default user-backend (database)
  • [ ] LDAP/ Active Directory
  • [ ] SSO - SAML
  • [ ] Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

{
  "reqId": "2M3DAMpzQRlKeiad9X2L",
  "level": 3,
  "time": "2024-06-24T18:22:46+00:00",
  "remoteAddr": "188.77.44.131",
  "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 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0",
  "version": "29.0.2.2",
  "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": "->"
      },
      {
        "file": "web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 83,
        "function": "newInstanceArgs",
        "class": "ReflectionClass",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 128,
        "function": "buildClass",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 146,
        "function": "resolve",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/DependencyInjection/DIContainer.php",
        "line": 470,
        "function": "query",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/DependencyInjection/DIContainer.php",
        "line": 442,
        "function": "queryNoFallback",
        "class": "OC\\AppFramework\\DependencyInjection\\DIContainer",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/App.php",
        "line": 163,
        "function": "query",
        "class": "OC\\AppFramework\\DependencyInjection\\DIContainer",
        "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/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"
  }
}

Additional info

No response

aleixq avatar Jun 24 '24 20:06 aleixq

Allow userId to be null when using

How is that supposed to? We need the userId to create the share.

kesselb avatar Jun 25 '24 10:06 kesselb

excuse me if i'm wrong but ¿ why then the constructor allows it to be null? or I am missing something?

aleixq avatar Jun 26 '24 21:06 aleixq

You are not wrong.

We usually use ?string for the userId because the userId can be null, for example when a controller has the PublicPage annotation. That's not the case here, we need the userId otherwise the controller cannot work.

Would you mind sending us a pull request with your suggested fix? I guess it's fine to update the type hint.

kesselb avatar Jun 27 '24 15:06 kesselb

Well, let me show the big frame. I found this issue when adapting the controller that extends the ShareApiController in the app webapppassword to allow the use of share api outside of nextcloud.

If we are trying to use the current controller in https://github.com/digital-blueprint/webapppassword/blob/main/lib/Controller/ShareAPIController.php

It arises:

{
  "reqId": "lbDsFwFLxAyTN4QFQJzx",
  "level": 3,
  "time": "2024-06-27T14:23:23+00:00",
  "remoteAddr": "x.x.x.x",
  "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 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0",
  "version": "29.0.2.2",
  "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": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 88,
        "function": "newInstanceArgs",
        "class": "ReflectionClass",
        "type": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 133,
        "function": "buildClass",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/Utility/SimpleContainer.php",
        "line": 151,
        "function": "resolve",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/DependencyInjection/DIContainer.php",
        "line": 470,
        "function": "query",
        "class": "OC\\AppFramework\\Utility\\SimpleContainer",
        "type": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/DependencyInjection/DIContainer.php",
        "line": 442,
        "function": "queryNoFallback",
        "class": "OC\\AppFramework\\DependencyInjection\\DIContainer",
        "type": "->"
      },
      {
        "file": "/web/lib/private/AppFramework/App.php",
        "line": 163,
        "function": "query",
        "class": "OC\\AppFramework\\DependencyInjection\\DIContainer",
        "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/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"
  }
}

If I change the type of currentUser property in web/apps/files_sharing/lib/Controller/ShareAPIController.php to match the __construct signature ( so set to private ?string currentUser), with current webapppassword implementation it shows:

  Missatge: Internal error: Failed to retrieve the default value
   Fitxer: /web/lib/private/AppFramework/Utility/ControllerMethodReflector.php
   Línia: 101


Traeback

#0 /web/lib/private/AppFramework/Utility/ControllerMethodReflector.php(101): ReflectionParameter->getDefaultValue()
#1 /web/lib/private/AppFramework/Http/Dispatcher.php(128): OC\AppFramework\Utility\ControllerMethodReflector->reflect()
#2 /web/lib/private/AppFramework/App.php(184): OC\AppFramework\Http\Dispatcher->dispatch()
#3 /web/lib/private/Route/Router.php(338): OC\AppFramework\App::main()
#4 /web/lib/base.php(1050): OC\Route\Router->match()
#5 /web/index.php(49): OC::handleRequest()
#6 {main}

Keeping currentUser property set to private ?string currentUser , if I use the same signature as files_sharing shareapicontroller for createShare method in the webapppassword shareapicontroller it works as it should.

aleixq avatar Jun 27 '24 21:06 aleixq

This error also pops-out on CI logs. https://github.com/nextcloud/server/actions/runs/10289497926/job/28478362370?pr=47116#step:12:23

solracsf avatar Aug 07 '24 18:08 solracsf

@kesselb on Jun 27, 2024:

We usually use ?string for the userId because the userId can be null, for example when a controller has the PublicPage annotation. That's not the case here, we need the userId otherwise the controller cannot work.

Sorry for joining this discussion a little late. I have also been struggling with this error for a few weeks now (on NC29 and NC30, see #51541 and community#220128).

I understand that you declare the parameter userId as a nullable string. But if you then assign this nullable parameter to a local variable, ...

$this->currentUser = $userId;

... then this variable should also be declared as a nullable string. And it is not.

private string $currentUser;

As of NC31, this variable has disappeared ... and it should work again. But I don't want to switch to NC31 on my production system yet.

Regards, Friedbert

GitWidi avatar Mar 18 '25 08:03 GitWidi

I understand that you declare the parameter userId as a nullable string. But if you then assign this nullable parameter to a local variable, ...

Normally yes but in the constructor args the dependency injection type if string | null but in this case the controller should only be created if there is a user session so it must always be string. It seems there is a deeper issue that creates the controller before there is any user session.

susnux avatar Mar 18 '25 10:03 susnux

A small step to success: I found an answer to my issue on NC 30.0.7: I had activated 2-factor authentication (app: twofactor_totp) and still used my regular user password on the OCS-API-Request. A stupid user error!

In my opinion, this should give an authentication error (http response: 401) but not a successful response (http response: 200) without content.

However, this only applies to NC 30.0.7 - the problem persists under NC 29.0.12.

GitWidi avatar Mar 18 '25 15:03 GitWidi