X Tutup
Skip to content

Unify API responses when setting permissions for public links#39194

Merged
jvillafanez merged 3 commits intomasterfrom
issues/36442-36443
Feb 15, 2022
Merged

Unify API responses when setting permissions for public links#39194
jvillafanez merged 3 commits intomasterfrom
issues/36442-36443

Conversation

@JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Sep 9, 2021 •
edited
Loading

Description

Setting (and changing) the permissions of public links via the OCS API will now return proper and unified API responses. Adding create permissions while public uploading is disabled globally will always return a 403 response.

Before & after changes

The following table shows the changes before and after this PR. The rows highlighted in bold are the scenarios which now return a different response.

public uploads allowed

Action response before response after
Create link for folder with permission 1 200 200
Create link for folder with permission 4 200 200
Create link for folder with permission 5 200 200
Create link for folder with permission 7 200 200
Create link for folder with permission 15 200 200
Create link for folder with permission 31 200 200
Create link for file with permission 1 200 200
Create link for file with permission 4 404 404
Create link for file with permission 5 200 (will default to permission 1) 200 (will default to permission 1)
Create link for file with permission 7 200 (will default to permission 1) 200 (will default to permission 1)
Create link for file with permission 15 200 (will default to permission 3) 200 (will default to permission 3)
Create link for file with permission 31 200 (will default to permission 1) 200 (will default to permission 1)
Update link for folder with permission 1 200 200
Update link for folder with permission 4 200 200
Update link for folder with permission 5 200 200
Update link for folder with permission 7 200 200
Update link for folder with permission 15 200 200
Update link for folder with permission 31 400 400
Update link for file with permission 1 200 200
Update link for file with permission 4 400 400
Update link for file with permission 5 200 (will default to permission 1) 400
Update link for file with permission 7 400 400
Update link for file with permission 15 400 400
Update link for file with permission 31 400 400

public uploads not allowed

Action response before response after
Create link for folder with permission 1 200 200
Create link for folder with permission 4 403 403
Create link for folder with permission 5 403 403
Create link for folder with permission 7 200 (will default to permission 1) 403
Create link for folder with permission 15 403 403
Create link for folder with permission 31 200 (will default to permission 1) 403
Create link for file with permission 1 200 200
Create link for file with permission 4 403 403
Create link for file with permission 5 200 (will default to permission 1) 403
Create link for file with permission 7 200 (will default to permission 1) 403
Create link for file with permission 15 403 403
Create link for file with permission 31 200 (will default to permission 1) 403
Update link for folder with permission 1 200 200
Update link for folder with permission 4 403 403
Update link for folder with permission 5 400 403
Update link for folder with permission 7 403 403
Update link for folder with permission 15 403 403
Update link for folder with permission 31 400 400
Update link for file with permission 1 200 200
Update link for file with permission 4 403 403
Update link for file with permission 5 200 (will default to permission 1) 403
Update link for file with permission 7 403 403
Update link for file with permission 15 403 403
Update link for file with permission 31 400 400

Related Issue

How Has This Been Tested?

See reproduction steps in the above mentioned tickets.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@ownclouders
Copy link
Contributor

ownclouders commented Sep 9, 2021

💥 Acceptance tests pipeline apiSharePublicLink2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32228/71/1

JammingBen added a commit that referenced this pull request Sep 9, 2021
@JammingBen
Fix changelog item name for PR #39194, adjust failing acceptance test
2629c1f
jvillafanez
jvillafanez reviewed Sep 9, 2021
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2021

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

80.0% 80.0% Coverage
0.0% 0.0% Duplication

@jvillafanez
Copy link
Member

jvillafanez commented Sep 9, 2021

Code-wise it looks fine, but I'm still a bit unsure with the change of the behavior for the public uploads.
Maybe either QA or PM could verify if the behavior is right and there isn't any side effect.

phil-davis
phil-davis reviewed Sep 9, 2021
View reviewed changes
@phil-davis phil-davis self-requested a review September 9, 2021 10:43
@JammingBen
Copy link
Contributor Author

JammingBen commented Sep 9, 2021

I added a before & after-table in the first comment of this PR. Although some return codes still seem kinda random, it's much more streamlined with this "fix".

@pmaier1 @micbar When public share uploads are not allowed via config, I would expect an API request with create permissions to return a forbidden response. That goes for the create permissions alone, but also for combinations like read+create+update. Is my assumption correct?

@JammingBen
Copy link
Contributor Author

JammingBen commented Dec 22, 2021

Now that 10.9 is out, I'd like to catch up on this again. @pmaier1 What do you think about it? To summarize it:

The API responses when creating/editing public links are kinda random at the moment. Like for example when public uploading is forbidden, creating a link with the permissions create and read will throw a 403, whereas creating a link with permissions create, read and update will succeed, but silently converted to read only. The table in the top post gives detailed information about what has changed.

Assuming the clients are fine with it (which I will re-test), I think it would be good to streamline that.

@phil-davis
Copy link
Contributor

phil-davis commented Dec 22, 2021

It will be nice to get this more consistent. I expect (hope) that some API test combinations will fail and need to be adjusted to match the changed responses.

@JammingBen JammingBen added php Pull requests that update Php code technical debt labels Dec 22, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 22, 2021

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

80.0% 80.0% Coverage
0.0% 0.0% Duplication

@JammingBen
Copy link
Contributor Author

JammingBen commented Dec 22, 2021

I expect (hope) that some API test combinations will fail and need to be adjusted to match the changed responses.

I adjusted some acceptance test scenarios to work with the new response code, pipeline seems to be fine with it now.

@jvillafanez jvillafanez merged commit 8ac5682 into master Feb 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the issues/36442-36443 branch February 15, 2022 11:55
@jvillafanez
Copy link
Member

jvillafanez commented Feb 15, 2022

@mmattel in case there are docs to be updated.

@mmattel mmattel mentioned this pull request Feb 15, 2022
Open
12 tasks
@phil-davis phil-davis mentioned this pull request Feb 17, 2022
Merged
11 tasks
@jesmrec jesmrec mentioned this pull request May 12, 2022
Open
@jnweiger
Copy link
Contributor

jnweiger commented May 13, 2022

Confirmed fixed in 10.10.0 RC2

When Sharing settings are without "Allow public uploads"

  • All combinations containing 4 (aka PERMISSION_CREATE) cause the same message "Public upload disabled by the administrator"
  • I wonder what the meaning of 2 (aka PERMISSION_UPDATE) should be, when we don't allow public uploads....
    (inplace modification is not counted as upload?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis approved these changes

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

@jvillafanez jvillafanez Awaiting requested review from jvillafanez

Assignees

No one assigned

Labels

3 - To Review php Pull requests that update Php code technical debt

Projects

None yet

Milestone

No milestone

5 participants

@JammingBen @ownclouders @jvillafanez @phil-davis @jnweiger
X Tutup