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] fix public link update #7862

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Conversation

2403905
Copy link
Contributor

@2403905 2403905 commented Nov 30, 2023

Description

We fixed enforce password when normal users can update the public link to delete its password if permission is not sent in data

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • 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:

@2403905 2403905 force-pushed the issue-7821 branch 2 times, most recently from 9d9e932 to ab3310c Compare December 1, 2023 12:51
@2403905 2403905 changed the title [full-ci] fix enforce password [full-ci] fix public link update Dec 1, 2023
@SwikritiT
Copy link
Contributor

SwikritiT commented Dec 4, 2023

Hi @2403905 I tried your changes locally the failing test needs a little update

the test in acceptance/features/coreApiSharePublicLink3/updatePublicLinkShare.feature:104

  Scenario Outline: creating a new public link share with password and removing (updating) it to make the resources accessible without password using public API
    Given using OCS API version "<ocs_api_version>"
    And user "Alice" has uploaded file with content "Random data" to "/randomfile.txt"
    And user "Alice" has created a public link share with settings
      | path     | randomfile.txt |
      | password | %public%       |
    When user "Alice" updates the last public link share using the sharing API with
      #removing password is basically making password empty
      | password | %remove% |
    Then the OCS status code should be "<ocs_status_code>"
    And the HTTP status code should be "200"
    And the public should be able to download the last publicly shared file using the old public WebDAV API without a password and the content should be "Random data"
    And the public should be able to download the last publicly shared file using the new public WebDAV API without a password and the content should be "Random data"
    Examples:
      | ocs_api_version | ocs_status_code |
      | 1               | 100             |
      | 2               | 200             |

Previously the user could remove the password of a link but now they cannot so I think this update is okay based on what is expected of the server. One little change though is that the http status code is different now depending on the ocs version for /ocs/v2.php/apps/files_sharing/api/v1/shares the http status is 403 while it's 200 for /ocs/v1.php/apps/files_sharing/api/v1/shares. Now the scenario is something like this

  Scenario Outline: creating a new public link share with password and removing (updating) it
    Given using OCS API version "<ocs_api_version>"
    And user "Alice" has uploaded file with content "Random data" to "/randomfile.txt"
    And user "Alice" has created a public link share with settings
      | path     | randomfile.txt |
      | password | %public%       |
    When user "Alice" updates the last public link share using the sharing API with
      #removing password is basically making password empty
      | password | %remove% |
    Then the OCS status code should be "104"
    And the HTTP status code should be "<http_status_code>"
    Examples:
      | ocs_api_version | http_status_code |
      | 1               | 200              |
      | 2               | 403              |

I think we can remove this scenario as this case is covered here in this scenario below

Scenario Outline: normal user tries to remove password of a public link share (change/create permission)
, I'll make a update PR to include both ocs versions in the test.

Furthermore , can you please rebase this PR with the master to get the changes from this PR #7828 ? You should get the unexpected pass if the changes work as expected.

@SwikritiT
Copy link
Contributor

SwikritiT commented Dec 4, 2023

The changes fixes the issue #7821 however tests need a little adjustment in the error message and status code here

And the OCS status code should be "400"
And the OCS status message should be "missing required password"

These two lines need to be updated to

    And the OCS status code should be "104"
    And the OCS status message should be "user is not allowed to delete the password from the public link"

@2403905 2403905 force-pushed the issue-7821 branch 2 times, most recently from 0782842 to f5f2354 Compare December 4, 2023 10:32
@SwikritiT
Copy link
Contributor

SwikritiT commented Dec 5, 2023

API tests should pass now after the removal of

#### [Normal users can update the public link to delete its password if permission is not sent in data](https://github.com/owncloud/ocis/issues/7821)
- [coreApiSharePublicLink1/changingPublicLinkShare.feature:154](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiSharePublicLink1/changingPublicLinkShare.feature#L154)

Comment on lines 193 to 194
And the OCS status code should be "104"
And the OCS status message should be "user is not allowed to delete the password from the public link"
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed

Suggested change
And the OCS status code should be "104"
And the OCS status message should be "user is not allowed to delete the password from the public link"

Copy link
Contributor

Choose a reason for hiding this comment

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

These as well

#### [Normal users can update the public link to delete its password if permission is not sent in data](https://github.com/owncloud/ocis/issues/7821)

Copy link
Member

Choose a reason for hiding this comment

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

^
@2403905 tests are passing. Please, remove passing tests from the expected failure list.

Copy link

sonarqubecloud bot commented Dec 8, 2023

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@2403905 2403905 merged commit 40da95e into owncloud:master Dec 8, 2023
4 checks passed
ownclouders pushed a commit that referenced this pull request Dec 8, 2023
* the tests were modified

* Update tests/acceptance/features/coreApiSharePublicLink1/changingPublicLinkShare.feature

Co-authored-by: Sawjan Gurung <saw.jan.grg3e@gmail.com>

* the expected failures removed

* change log added, reva bumped.

---------

Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>
Co-authored-by: Sawjan Gurung <saw.jan.grg3e@gmail.com>
2403905 added a commit to 2403905/ocis that referenced this pull request Jan 24, 2024
* the tests were modified

* Update tests/acceptance/features/coreApiSharePublicLink1/changingPublicLinkShare.feature

Co-authored-by: Sawjan Gurung <saw.jan.grg3e@gmail.com>

* the expected failures removed

* change log added, reva bumped.

---------

Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>
Co-authored-by: Sawjan Gurung <saw.jan.grg3e@gmail.com>
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.

Normal users can update the public link to delete its password if permission is not sent in data
4 participants