Skip to content
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

wrong notification. User is notified even if share doesn't create #6368

Closed
ScharfViktor opened this issue May 23, 2023 · 13 comments
Closed

wrong notification. User is notified even if share doesn't create #6368

ScharfViktor opened this issue May 23, 2023 · 13 comments
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug
Milestone

Comments

@ScharfViktor
Copy link
Contributor

ScharfViktor commented May 23, 2023

related #6197

ownCloud
3.0.0-rc.4+c08439f23
7.0.0-rc.37

ocis against wopi using https://github.com/owncloud/ocis/blob/master/deployments/examples/ocis_wopi/docker-compose.yml
or open ocis.ocis-wopi.released.owncloud.works

Steps:

  • share, delete or edit opening in the onlyOffice file to user einstein

Actual: share didn't create/edit/deleted but einstein gets notification

Screen.Recording.2023-05-23.at.13.42.23.mov

Expected: no notification for einstein

@micbar micbar moved this from Qualification to Prio 2 in Infinite Scale Team Board May 24, 2023
@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label May 24, 2023
@rhafer rhafer self-assigned this May 30, 2023
@rhafer rhafer moved this from Prio 2 to In progress in Infinite Scale Team Board May 30, 2023
@rhafer
Copy link
Contributor

rhafer commented May 30, 2023

Hm, this is a tricky one. The gateway creates user shares in a two step process. First is issues a CreateShare request to the shareprovider and second is issues a AddGrant request to the storage provided.

When a file is locked (e.g. because it is opened in onlyoffice) CreateShare request succeeds and the AddGrant request fails. The ShareCreated event (which results the user notification) is however already fired by the shareprovider after the CreateShare request succeeded 😢

Ideally we would fire the event only after both requests succeeded. This isn't easily possible as the shareprovider (which is sending the event doesn't about the additional AddGrant call). I am still investigating possible solutions ...

rhafer added a commit to rhafer/ocis that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of owncloud#904

Fixes owncloud#6368
@rhafer rhafer removed their assignment Jul 10, 2023
@rhafer rhafer moved this from In progress to Prio 2 in Infinite Scale Team Board Jul 10, 2023
@micbar
Copy link
Contributor

micbar commented Jul 11, 2023

maybe interesting in the context of the CS3 Apis cs3org/cs3apis#165

@micbar
Copy link
Contributor

micbar commented Jul 11, 2023

@tbsbdr Can you qualify? P3 or P2?

@tbsbdr
Copy link

tbsbdr commented Jul 12, 2023

P3
what happeny if you click on the notification (via the bell)?

@ScharfViktor
Copy link
Contributor Author

P3 what happeny if you click on the notification (via the bell)?

Nothing terrible, no crashes no error message. The user just can't find his resource

Screen.Recording.2023-07-12.at.16.49.28.mov

@micbar micbar added Priority:p3-medium Normal priority and removed Priority:p2-high Escalation, on top of current planning, release blocker labels Jul 17, 2023
@micbar micbar moved this from Prio 2 to Prio 3 or less in Infinite Scale Team Board Jul 17, 2023
@micbar
Copy link
Contributor

micbar commented Oct 23, 2023

@kobergj @aduffeck we should try to allow that behavior, if possible.

currently it is a „no go“ because we need to write the new metadata to the mpk file when the office editor closes before we can write the grant.

How would normal file systems handle that? Can you add acls to a file that is locked? I doubt that.

If we allow that, we would create the possibility that people also remove exactly the share which is needed to store the outcome of the editing session which would cause data loss.

@2403905
Copy link
Contributor

2403905 commented Jan 15, 2024

@micbar Could we send a notification only if the userlog got the sequence of two events CreateShare and AddGrant?

@micbar micbar moved this from Prio 3 or less to Prio 2 in Infinite Scale Team Board Jan 16, 2024
@micbar micbar added Priority:p2-high Escalation, on top of current planning, release blocker and removed Priority:p3-medium Normal priority labels Jan 16, 2024
@micbar
Copy link
Contributor

micbar commented Jan 16, 2024

Raising prio because of SLAs from Project.

@micbar
Copy link
Contributor

micbar commented Jan 16, 2024

@butonic @rhafer @kobergj @aduffeck

IMHO we should really invest time to make sharing and setting the grant of a locked file possible. This would close many Tickets where users are confused.

From a product POV it is also very hard to add more people to an exisiting WebOffice Session. You would need to directly contact all participants and ask them to leave the session 🙈 That is not easy.

@micbar
Copy link
Contributor

micbar commented Jan 22, 2024

@dragonchaser and @aduffeck will connect on the Draft PR.

@butonic can you please share your opinion on this?

@aduffeck
Copy link
Contributor

draft PR with passing CI: cs3org/reva#4464

@butonic
Copy link
Member

butonic commented Jan 22, 2024

We don't change the mtime, tmtime or etag when modifying grants on a file: cs3org/reva#4464 (comment) (we do change the parent tmtime and propagate it).
We can declare oc:permissionsand oc:share-id as unlockable to be RFC compliant: cs3org/reva#4464 (comment)

No objections to allow modifing shares while a file is locked.

@micbar micbar added this to the Release 5.0.0 milestone Jan 22, 2024
@dragonchaser
Copy link
Contributor

fixed with #8264

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug
Projects
Archived in project
Development

No branches or pull requests

8 participants