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

Dont check locks grants #4464

Merged
merged 2 commits into from Jan 22, 2024
Merged

Conversation

aduffeck
Copy link
Contributor

No description provided.

@aduffeck aduffeck requested a review from a team as a code owner January 19, 2024 07:15
Copy link

update-docs bot commented Jan 19, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic
Copy link
Contributor

butonic commented Jan 22, 2024

this needs a test against the ocis testsuite. When we change the grants we do not change the actual file content. so our etag should not change. but we explicitly made grants change the etag so clients would pick up the metadata change in case a file becomes readonly ... hm ...

in posix a file has an atime (access time), mtime (modification time which only changes when the file content changes), ctime (change time, which includes metadata) and the btime (birth time). a chmod / setfacl only changes the ctime, but not the mtime.

Currently, we derive the etag from the tmtime (tree modification time), which uses the user.ocis.mtime node property. If it is not set it falls back to the nodes mtime.

But then a grant change should not change the etag ...

I need to run the code.

@butonic
Copy link
Contributor

butonic commented Jan 22, 2024

The etag of a file does not change when we add or remove a grant. However, we do propagate an etag change: the containing folders tmtime is updated when the grants of a contained file change.

So, IMO, we could omit the CheckLock() call to allow changing grantds when files are locked.

Let me check the RFCs

@butonic
Copy link
Contributor

butonic commented Jan 22, 2024

https://datatracker.ietf.org/doc/html/rfc4918#section-7

Clients MUST submit a lock-token they are authorized to use in any
request that modifies a write-locked resource. The list of
modifications covered by a write-lock include:

  1. A change to any of the following aspects of any write-locked
    resource:

    • any variant,

    • any dead property,

    • any live property that is lockable (a live property is
      lockable unless otherwise defined.)

So, let's check what dead and live properties are: https://datatracker.ietf.org/doc/html/rfc4918#section-4.1

There are two categories of properties: "live" and "dead". A live
property has its syntax and semantics enforced by the server. Live
properties include cases where a) the value of a property is
protected and maintained by the server, and b) the value of the
property is maintained by the client, but the server performs syntax
checking on submitted values. [...]

The getlastmodified property e.g. is a live property https://datatracker.ietf.org/doc/html/rfc4918#section-15.7 as it is managed by the server. Now, we don't change it when changing grants, as we SHOULD:

Description: The last-modified date on a resource SHOULD only
reflect changes in the body (the GET responses) of the resource.

So, the question is what WebDAV property are we affecting by adding grants?

Only oc:permissions comes to mind. It is managed by the server, so it is a live property. However, we can define it as unlockable.

I don't see a reason why we would need to allow locking the oc-permissions property and in consequence changing grants.

There is also oc:shareid but same rule applies IMO. We define it and can declare it unlockable.

Furthermore, the permission to change grants is limited by permission checks as well. If an edit wer over permissions occurs and someone asks to be able to lock the permissions we can add an api for it. Until then: grants cannot be locked.

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also remove the CheckLock() call from RemoveGrant()

@aduffeck aduffeck changed the title [WIP] Dont check locks grants Dont check locks grants Jan 22, 2024
@dragonchaser dragonchaser merged commit d79f163 into cs3org:edge Jan 22, 2024
9 checks passed
@aduffeck aduffeck deleted the dont-check-locks-grants branch January 23, 2024 07:18
@micbar micbar mentioned this pull request Feb 26, 2024
71 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants