SharingNg. sharing invitation

[ScharfViktor]

1. share to same user/group - 500
   log:

2023-12-13T14:30:37+01:00 DBG share creation failed line=/Users/scharfviktor/Work/ocis/services/graph/pkg/service/v0/driveitems.go:561 service=graph
2023-12-13T14:30:37+01:00 DBG bytes=180 duration=87.1695 line=/Users/scharfviktor/Work/ocis/ocis-pkg/middleware/logger.go:27 method=POST path=/graph/v1beta1/drives/d738470e-d561-48ca-aefb-f8686ebb2e52$ad6f4ad2-b863-4b1b-aadb-b63d094361e8/items/d738470e-d561-48ca-aefb-f8686ebb2e52$ad6f4ad2-b863-4b1b-aadb-b63d094361e8!864d5538-57b3-445d-9deb-b0d7e02de90d/invite proto=HTTP/1.1 request-id= service=graph status=500
2023-12-13T14:30:37+01:00 INF access-log bytes=180 duration=406.08425 line=/Users/scharfviktor/Work/ocis/services/proxy/pkg/middleware/accesslog.go:31 method=POST path=/graph/v1beta1/drives/d738470e-d561-48ca-aefb-f8686ebb2e52$ad6f4ad2-b863-4b1b-aadb-b63d094361e8/items/d738470e-d561-48ca-aefb-f8686ebb2e52$ad6f4ad2-b863-4b1b-aadb-b63d094361e8!864d5538-57b3-445d-9deb-b0d7e02de90d/invite proto=HTTP/1.1 remote-addr=[::1]:64109 request-id=VIKTORS-AIR.local/DWRfO3XfoI-000001 service=proxy status=500

2. wrong or empty libre.graph.recipient.type in the body - 500
3. share to disabled user - 200. Do we need forbid it?
4. share to deleted user - 500
5. share to deleted group -200.
6. multiple share (if one is wrong) -207 response Code

"value": [
        {
            "grantedToV2": {
                "group": {
                    "displayName": "radium-lovers",
                    "id": "7b87fd49-286e-4a5f-bafd-c535d5dd997a"
                }
            },
            "id": "d738470e-d561-48ca-aefb-f8686ebb2e52:ad6f4ad2-b863-4b1b-aadb-b63d094361e8:d0526643-7d58-45fd-ba6f-e07774b02fb3",
            "roles": [
                "1c996275-f1c9-4e71-abdf-a42f6495e960"
            ]
        },
        {
            "error": {
                "code": "generalException",
                "innererror": {
                    "date": "2023-12-11T20:04:05Z",
                    "request-id": "VIKTORS-AIR.local/ophX0sJyIG-014711"
                },
                "message": "Internal Server Error"
            }
        }
    ]

СС @rhafer

[rhafer]

1. share to same user/group - 500

Yes, I can reproduce that. So we have a bit of a problem here as we allow to create invitations with more than one receipient e.g.

{
  "recipients": [
    {
      "objectId": "4c510ada-c86b-4815-8820-42cdf82c3d51",
      "@libre.graph.recipient.type": "user"
    },
		{
      "objectId": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c",
      "@libre.graph.recipient.type": "user"
    }
  ],
  "roles": [
    "b1e2218d-eef8-4d4c-b82d-0f1a1b48f3b5"
  ]
}

If all of that succeeds we return a 200. However if one of the recipients already has a share for that resource. We return a 207 (Multistatus) with multiple values like this:

{
	"value": [
		{
			"grantedToV2": {
				"user": {
					"displayName": "Albert Einstein",
					"id": "4c510ada-c86b-4815-8820-42cdf82c3d51"
				}
			},
			"id": "6e7423b1-c7d4-4039-8526-f261a08e64de:7a4a3d7a-66db-4a91-b9e3-b437d3273264:4374bf71-8cd5-43b0-ae32-c9646206b326",
			"roles": [
				"b1e2218d-eef8-4d4c-b82d-0f1a1b48f3b5"
			]
		},
		{
			"error": {
				"code": "generalException",
				"innererror": {
					"date": "2023-12-14T14:22:02Z",
					"request-id": "6b73857ea941/0vNPTFOa7D-000012"
				},
				"message": "Internal Server Error"
			}
		}
	]
}

And if all of them fail, or there was just one recipiet, we return a 500 regardless of why the CreateShare failed. I guess we could at least set the error in the body to something more helpful. But setting a proper statuscode for the whole request seem difficult. We could of course do some special casing when there was just one recipient in the request and map the error of that single request to a proper status code.

Maybe we're making our lifes unnecessarily hard by allowing multiple recipients in a single request in the first place. (I think it also hard for client implementations, i.e. you can't really match with error is for which recipient).
@micbar @butonic comments? Do we really need this complexity?

2. wrong or empty libre.graph.recipient.type in the body - 500

I can't reproduce this one. I guess you used userid in id that already had a share?
We just assume user if libre.graph.recipient.type is set to anything else than group. I guess we can make that check more explicit and reject anything that is not user or group

3. share to disabled user - 200. Do we need forbid it?

Good question how does ocs behave currently?

4. share to deleted user - 500

Yeah, same as for 1. we could set a better error in the body, but I am not sure yet what to do with the status code. (Especially in the when multiple recipients are in the request).

5. share to deleted group -200.

Really? I get a 500 here as well.

6. multiple share (if one is wrong) -207 response Code

Yes 207 is Multi-Status. And I really wonder if we should go down that path. (AFAIK 207 is not even part of the HTTP RFC)

[rhafer]

After discussing with @butonic and @micbar we agreed to forbid requests with multiple recipients for now and return a 400 when receiving such a request. Mainly to keep it as simple as possible.

cc @individual-it

[ScharfViktor]

2. wrong or empty libre.graph.recipient.type in the body - 500

I can't reproduce this one. I guess you used userid in id that already had a share? We just assume user if libre.graph.recipient.type is set to anything else than group. I guess we can make that check more explicit and reject anything that is not user or group

Steps:

• please try to send request with groupId and empty or wrong libre.graph.recipient.type

3. share to disabled user - 200. Do we need forbid it?

Good question how does ocs behave currently?

it's not possible <status>error</status><statuscode>998</statuscode><message>user not found</message>

5. share to deleted group -200.

Really? I get a 500 here as well.

Yes, sorry. I also gel 500

[fschade]

@rhafer, @ScharfViktor,
please have a look at #8019

[individual-it]

After discussing with @butonic and @micbar we agreed to forbid requests with multiple recipients for now and return a 400 when receiving such a request. Mainly to keep it as simple as possible.

Will that be a permanent change? And how will the webUI handle sharing with multiple recipients at the same time? Send multiple requests?

[micbar]

That will stay this way until we find something related to batch processing.

[micbar]

Disabled users should not be able to be share receivers.

Please check that and fix if still possible.

[rhafer]

I'll take a look into the share with disabled user problem. I think it's a bug in the userprovider/manager

[rhafer]

Fix for the disabled user issue: cs3org/reva#4426

I guess we should also adapt the graph/users?$search implementation to no return disabled users for users that lack the usermanagement permission. (I'll look into that as well)

[grgprarup]

Fix for the disabled user issue: cs3org/reva#4426

I guess we should also adapt the graph/users?$search implementation to no return disabled users for users that lack the usermanagement permission. (I'll look into that as well)

@rhafer Is the issue for sharing with a disabled user fixed? I am adding a test to cover that scenario and still we can send share invitations to disabled users.

[rhafer]

Is the issue for sharing with a disabled user fixed? I am adding a test to cover that scenario and still we can send share invitations to disabled users.

@grgprarup No. Unfortunately we needed to revert the original fix. I am trying to come up with a better solution.

[rhafer]

Disabled users should not be able to be share receivers.

Please check that and fix if still possible.

We (@micbar @butonic and I) had a discussion about this again. And somehow the assumption that one cannot share with a disabled user seems wrong. The main driver of the "disable user" feature was to temporary forbid a user to use ocis (disallow the user to login). The admin UI for usermanagement actually also does not talk about enable/disable user, but allow/disallow login. So if you go by that definition allowing a user to create a share with a disabled user (if he knows the ID if that user) should be fine (there is no harm done). We'd prefer to keep that behavior as it is for sharing NG currently (we might need to revisit that decision in the future)

@grgprarup For the tests that means sharing with a disabled user is allowed to succeed when using the new sharing API.

However, while digging into this I found some pretty inconsistent behavior in the CS3 users API (the LDAP driver specifically) with regards to the enabled/disabled state of users. (I'll open up a separate issue to discuss that).