Skip to content

Conversation

@jvillafanez
Copy link
Member

Description

Trying to disable a space without enough permissions was throwing a 404 error despite the space was visible. A 403 error is more suitable

Related Issue

#11781

Motivation and Context

Better error reporting

How Has This Been Tested?

Manually tested. Disabling the space was done using curl because it's tricky to do it from the web.

  1. Create a space
  2. Give user1 viewer access to the space
  3. User1 tries to disable the space (via curl)

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:

@jvillafanez jvillafanez self-assigned this Nov 27, 2025
@update-docs
Copy link

update-docs bot commented Nov 27, 2025

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.

@jvillafanez jvillafanez force-pushed the space_permission_disable branch 2 times, most recently from 755f2aa to eaf9474 Compare November 28, 2025 09:08
@jvillafanez jvillafanez force-pushed the space_permission_disable branch from eaf9474 to df94646 Compare November 28, 2025 12:30
@jvillafanez
Copy link
Member Author

@nirajacharya2 could you check https://drone.owncloud.com/owncloud/ocis/49440/21/6 , the scenario below?

Scenario Outline: space admin user tries to disable the personal space     # /drone/src/tests/acceptance/features/apiSpaces/spaceManagement.feature:153
    When user "<user>" disables a space "Alice Hansen" owned by user "Alice" # SpacesContext::userDisablesSpace()
    Then the HTTP status code should be "403"                                # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    Examples:
      | user  |
      | Brian |
      | Carol |
        Failed step: Then the HTTP status code should be "403"
        HTTP status code 404 is not the expected value 403
        Failed asserting that 404 matches expected '403'.

If I revert the test and use 404 instead of 403, then Brian will complain (https://drone.owncloud.com/owncloud/ocis/49437/21/6):

  Scenario Outline: space admin user tries to disable the personal space     # /drone/src/tests/acceptance/features/apiSpaces/spaceManagement.feature:153
    When user "<user>" disables a space "Alice Hansen" owned by user "Alice" # SpacesContext::userDisablesSpace()
    Then the HTTP status code should be "404"                                # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    Examples:
      | user  |
      | Brian |
        Failed step: Then the HTTP status code should be "404"
        HTTP status code 403 is not the expected value 404
        Failed asserting that 403 matches expected '404'.
      | Carol |

In theory, we should expect a 403 because the user shouldn't have enough permissions to disable a personal space (correct me if I'm wrong).
Or maybe we should expect a 404 because a space admin shouldn't be able to see a personal space. In this case, maybe one of the users (likely Brian) is also an admin? Being an admin might grant him privileges to see other people's personal spaces.

To clarify, I'm fine if we decide that the expected status code is 404 because the space admin shouldn't seen personal spaces, but if both users have the same role, the expected value should be the same for both.

@nirajacharya2
Copy link
Contributor

To clarify, I'm fine if we decide that the expected status code is 404 because the space admin shouldn't seen personal spaces, but if both users have the same role, the expected value should be the same for both.

If i understand correctly this will the new expected behavior.

role role code
User User 403 (because same role)
Space Admin User 404
Space Admin Space Admin 403
Admin User 403

if that's the case it makes sens. i have also updated the test.

@jvillafanez
Copy link
Member Author

I'm confused with the test. space admin user tries to disable the personal space For me, the personal space is the one that appears as "personal" in the left menu.
From my understanding, both test users (Brian and Carol) should be space admins, so the result should be the same for both.
If Brian is an admin, then he should have his own test: "admin tries to disable the personal space"

@nirajacharya2 nirajacharya2 force-pushed the space_permission_disable branch from db1c337 to 48576a2 Compare December 3, 2025 09:18
@nirajacharya2
Copy link
Contributor

I'm confused with the test. space admin user tries to disable the personal space For me, the personal space is the one that appears as "personal" in the left menu. From my understanding, both test users (Brian and Carol) should be space admins, so the result should be the same for both. If Brian is an admin, then he should have his own test: "admin tries to disable the personal space"

carol was normal user in this scenario. i have removed carol.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 3, 2025

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