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

[full-ci] Enable machine auth in ocdav #4076

Merged
merged 5 commits into from
Jul 5, 2022

Conversation

aduffeck
Copy link
Contributor

@aduffeck aduffeck commented Jul 1, 2022

This PR is the ocis counterpart to cs3org/reva#3009. It enables machine auth in ocdav so that reva can prevent recursive copy/move operations.

@update-docs
Copy link

update-docs bot commented Jul 1, 2022

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.

@mmattel
Copy link
Contributor

mmattel commented Jul 1, 2022

It enables machine auth in ocdav so that reva can prevent recursive copy/move operations.

Reading the code:
OCDAV_MACHINE_AUTH_API_KEY" desc:"Machine auth API key used for validating requests from other services when impersonating users."

Q: copy/move vs impersonating? Isn't that something different?

@aduffeck
Copy link
Contributor Author

aduffeck commented Jul 4, 2022

@mmattel the higher level purpose of this change is to detect and prevent recursive operations being performed by the user. In order to do that we internally need to impersonate users, i.e. the owners of shared resources when working with shares they created in the context of the grantee.
This is what the machine authentication scheme does and what we need the config option for, so the strings make sense to me the way they are. But maybe they need more explaining or something?

@mmattel
Copy link
Contributor

mmattel commented Jul 4, 2022

But maybe they need more explaining or something?

While technically correct, I do not think it is necessary for non developers to know how it is made (ok as a note in dev docs), but explain what it is good for. Suggestion:
Machine auth API key used for validating requests from other services when impersonating users. ->
Machine auth API key used to validate internal requests necessary for the access to resources from other services.

@aduffeck
Copy link
Contributor Author

aduffeck commented Jul 5, 2022

@mmattel sounds great 👍 I also streamlined the wording across all occurences of that setting.

@aduffeck aduffeck force-pushed the enabled-machine-auth-in-ocdav branch from dc753e0 to c2b163a Compare July 5, 2022 09:05
@aduffeck aduffeck marked this pull request as ready for review July 5, 2022 09:11
@aduffeck aduffeck changed the title Enable machine auth in ocdav [full-ci] Enable machine auth in ocdav Jul 5, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jul 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@butonic butonic merged commit 2e64042 into owncloud:master Jul 5, 2022
@butonic butonic deleted the enabled-machine-auth-in-ocdav branch July 5, 2022 18:14
ownclouders pushed a commit that referenced this pull request Jul 5, 2022
Merge: 3374467 703b488
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Tue Jul 5 18:14:41 2022 +0000

    Merge pull request #4076 from aduffeck/enabled-machine-auth-in-ocdav

    [full-ci] Enable machine auth in ocdav
ownclouders pushed a commit that referenced this pull request Jul 6, 2022
Merge: 3374467 703b488
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Tue Jul 5 18:14:41 2022 +0000

    Merge pull request #4076 from aduffeck/enabled-machine-auth-in-ocdav

    [full-ci] Enable machine auth in ocdav
wkloucek added a commit to owncloud/ocis-charts that referenced this pull request Jul 19, 2022
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.

3 participants