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

Deleting a user doesn't delete their personal space #4195

Closed
ScharfViktor opened this issue Jul 13, 2022 · 9 comments · Fixed by #4528 or #4532
Closed

Deleting a user doesn't delete their personal space #4195

ScharfViktor opened this issue Jul 13, 2022 · 9 comments · Fixed by #4528 or #4532
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Type:Regression

Comments

@ScharfViktor
Copy link
Contributor

ocis 2.0.0-beta.5 local

Steps:

  • user Katherine uploads file and creates folder to personal space
  • admin deletes user Katherine
  • admin get all spaces: curl -vk 'https://localhost:9200/graph/v1.0/drives' -u admin:admin -vk

Expected: space with "driveAlias": "personal/katherine" doesn't exist
make sure that no data in: tree -a /.ocis/storage/users/spaces/katherineSpaceId/

Actual: space with all data still exist.

@micbar micbar added Priority:p2-high Escalation, on top of current planning, release blocker Type:Regression labels Jul 13, 2022
@micbar
Copy link
Contributor

micbar commented Jul 13, 2022

This used to work with the accounts service. Therefore regression.

@micbar micbar added this to the 2.0.0 General Availability milestone Jul 13, 2022
@rhafer
Copy link
Contributor

rhafer commented Jul 13, 2022

This used to work with the accounts service. Therefore regression.

AFAIR we decided (in some refinement) that deleting a user and deleting its files should be separate things? And deleting user should not automatically remove all the user's personal space.

IIRC @butonic had a pretty strong opinion on that.

But yeah, either way. We need some way to delete a user's personal space.

@rhafer
Copy link
Contributor

rhafer commented Aug 8, 2022

Result from refinement:

  • for now it's ok to delete the personal space of a user, when deleting the user
  • we need to introduce a new permission DeleteAllSpaces that the user who's deleting another is user needs to have assigned in order to delete the personal space
  • The DeleteUser call in the GraphAPI should check for all required permission prior to deleting a user. (in order to avoid "half-deleted" users)

@rhafer
Copy link
Contributor

rhafer commented Sep 7, 2022

@ScharfViktor cs3org/reva#3202 should fix the permission error you having been seeing. There is still another problem left, was currently Deleting a user will only more the personal space to trash and not purge it.

Will look into that next.

@ScharfViktor
Copy link
Contributor Author

@ScharfViktor cs3org/reva#3202 should fix the permission error you having been seeing. There is still another problem left, was currently Deleting a user will only more the personal space to trash and not purge it.

Will look into that next.

Thanks. it will looks like disabling space? "deleted": {"state": "trashed"}, in the root section
Do we have to achieve complete deletion or is it okay?

rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
previously the homespace was just marked as trashed

Fixed: owncloud#4195
rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
previously the homespace was just marked as trashed

Fixed: owncloud#4195
rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
previously the homespace was just marked as trashed

Fixes: owncloud#4195
@rhafer
Copy link
Contributor

rhafer commented Sep 7, 2022

Thanks. it will looks like disabling space? "deleted": {"state": "trashed"}, in the root section

Yes.

Do we have to achieve complete deletion or is it okay?

I think we want complete deleting. The fix is in #4528

@ScharfViktor
Copy link
Contributor Author

sorry for the persistence, but for me it still doesn't work.

log, when I delete user:
2022-09-07T16:15:12+02:00 ERR failed to delete storage space error="error: permission denied: user is not allowed to delete spaces 52f998c3-9c58-498b-bfc8-0cd9fc135ad7" pkg=rgrpc service=storage-users status={"code":8,"message":"permission denied","trace":"00000000000000000000000000000000"} storage_space_id={"opaque_id":"1284d238-aa92-42ce-bdc4-0b0000009157$52f998c3-9c58-498b-bfc8-0cd9fc135ad7!52f998c3-9c58-498b-bfc8-0cd9fc135ad7"} traceid=00000000000000000000000000000000 2022-09-07T16:15:12+02:00 ERR failed to delete storage space error="error: permission denied: user is not allowed to delete spaces 52f998c3-9c58-498b-bfc8-0cd9fc135ad7" pkg=rgrpc service=storage-users status={"code":8,"message":"permission denied","trace":"00000000000000000000000000000000"} storage_space_id={"opaque_id":"1284d238-aa92-42ce-bdc4-0b0000009157$52f998c3-9c58-498b-bfc8-0cd9fc135ad7!52f998c3-9c58-498b-bfc8-0cd9fc135ad7"} traceid=00000000000000000000000000000000

I understand that the admin is trying to delete in two steps personal space of the user but he has not permissions

if we check admins permissions
curl -s -k -u admin:admin -X POST https://localhost:9200/api/v0/settings/roles-list -d '{}' -H 'Content-Type: application/json'

looks like that admin can delete and it should work. Am I missing something?

"id":"5de9fe0a-4bc5-4a47-b758-28f370caf169","name":"delete-all-home-spaces","displayName":"Delete All Home Spaces","description":"This permission allows to delete home spaces."

@micbar
Copy link
Contributor

micbar commented Sep 7, 2022

needs a reva update. I am working on it in #4522

@ScharfViktor
Copy link
Contributor Author

Thanks, checked again on update-reva-edge branch. All works fine

rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
DELETE requess on /graph/v1.0/users also work when specifing a user by
name. For deleting the home space in that case we need to get the User's
id from the backend first.

Fixes: owncloud#4195
butonic added a commit that referenced this issue Sep 8, 2022
* Add support for the jsoncs3 share manager

* WIP: point to the wip-reva with the jsoncs3 share manager

* migration fixes

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* use common config for jsoncs3 defaults

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* lint

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* add home space deletion on user delete

Signed-off-by: Christian Richter <[email protected]>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* add delete home space permission to admin role

Signed-off-by: Christian Richter <[email protected]>

* Bump github.com/tus/tusd from 1.9.0 to 1.9.1

Bumps [github.com/tus/tusd](https://github.com/tus/tusd) from 1.9.0 to 1.9.1.
- [Release notes](https://github.com/tus/tusd/releases)
- [Commits](tus/tusd@v1.9.0...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/tus/tusd
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/onsi/gomega from 1.20.0 to 1.20.1

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.0 to 1.20.1.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.0...v1.20.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* part1

* update reva

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* [test-only] apiTest. change own password (#4480)

* apiTest. change own password

* fix

* Bump github.com/gookit/config/v2 from 2.1.2 to 2.1.4

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.2...v2.1.4)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* move graph api methods from Space context to GraphHelper

* fix after rebase

* Bump github.com/rs/zerolog from 1.27.0 to 1.28.0

Bumps [github.com/rs/zerolog](https://github.com/rs/zerolog) from 1.27.0 to 1.28.0.
- [Release notes](https://github.com/rs/zerolog/releases)
- [Commits](rs/zerolog@v1.27.0...v1.28.0)

---
updated-dependencies:
- dependency-name: github.com/rs/zerolog
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/onsi/ginkgo/v2 from 2.1.4 to 2.1.6

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.1.4 to 2.1.6.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.1.4...v2.1.6)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/coreos/go-oidc/v3 from 3.2.0 to 3.3.0

Bumps [github.com/coreos/go-oidc/v3](https://github.com/coreos/go-oidc) from 3.2.0 to 3.3.0.
- [Release notes](https://github.com/coreos/go-oidc/releases)
- [Commits](coreos/go-oidc@v3.2.0...v3.3.0)

---
updated-dependencies:
- dependency-name: github.com/coreos/go-oidc/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Added /Shares related tests for lock properties on ocis

* Update expected to failure for local api for copy

* Bump github.com/grpc-ecosystem/grpc-gateway/v2 from 2.11.2 to 2.11.3

Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.11.2 to 2.11.3.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/master/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.11.2...v2.11.3)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* refactor proxy code

I refactored the proxy so that we execute the routing before the
authentication middleware. This is necessary so that we can determine
which routes are considered unprotected i.e. which routes don't need
authentication.

* add unprotected flag to the proxy routes

I added an unprotected flag to the proxy routes which is evaluated by
the authentication middleware. This way we won't have to maintain a
hardcoded list of unprotected paths and path prefixes and we will
hopefully reduce the times we encounter the basic auth prompt by web
browsers.

* add missing unprotected flag and fix proxy test

* fix linting issues

* fix default policy and add changelog

* update tests

* Automated changelog update [skip ci]

* Bump google.golang.org/grpc from 1.48.0 to 1.49.0

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.48.0 to 1.49.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.48.0...v1.49.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/onsi/gomega from 1.20.1 to 1.20.2

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.1 to 1.20.2.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.1...v1.20.2)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/urfave/cli/v2 from 2.11.2 to 2.14.0

Bumps [github.com/urfave/cli/v2](https://github.com/urfave/cli) from 2.11.2 to 2.14.0.
- [Release notes](https://github.com/urfave/cli/releases)
- [Changelog](https://github.com/urfave/cli/blob/main/docs/CHANGELOG.md)
- [Commits](urfave/cli@v2.11.2...v2.14.0)

---
updated-dependencies:
- dependency-name: github.com/urfave/cli/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump github.com/owncloud/libre-graph-api-go from 0.16.0 to 0.17.0

Bumps [github.com/owncloud/libre-graph-api-go](https://github.com/owncloud/libre-graph-api-go) from 0.16.0 to 0.17.0.
- [Release notes](https://github.com/owncloud/libre-graph-api-go/releases)
- [Commits](owncloud/libre-graph-api-go@v0.16.0...v0.17.0)

---
updated-dependencies:
- dependency-name: github.com/owncloud/libre-graph-api-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Evaluate routing rules ordered by path-length

This is a quickfix for #4497. Before evaluating, we now sort the rules
of a specific type by the length of the endpoints and start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.

* Bump commit id for tests

* Improve login screen design

* Move background-size after the background css prop

* add returns after rendering errors and simplify loop condition

* Hardcode header in GraphHelper::deleteSpace

* Add 'Inter' font, change placeholder color to grey

* Use cv11 as font feature setting

* Automated changelog update [skip ci]

* adjust REPORT to PROPFIND endpoint

Signed-off-by: jkoberg <[email protected]>

* regenerate protogen

Signed-off-by: jkoberg <[email protected]>

* adjust expected failures

Signed-off-by: jkoberg <[email protected]>

* Automated changelog update [skip ci]

* Fix translations on login page

* Automated changelog update [skip ci]

* removed search test from expected failures

* Reuse code of code from core at most

* Add commit and branch related to this PR to work

* Reuse locking method from core

* fix search test

* Add api tests for tus upload when quota is set

* Add tests removed by core pr-40276

* graph: Fix Status code when updating the password

Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See #4480

* Bump github.com/gookit/config/v2 from 2.1.4 to 2.1.5

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.4 to 2.1.5.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.4...v2.1.5)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* auto orientate pictures for thumbnails

* Automated changelog update [skip ci]

* update buf lock

* graph: purge home space when deleting a user

previously the homespace was just marked as trashed

Fixes: #4195

* proxy: Initialize logger for router

* proxy: Fix archiver for public links

Allows /archiver to be used the "public-token" auth middleware. The
archiver is a bit of a special case, because it can be uses in several
ways: using 'normal' authentication (basic, oidc), using signed-urls or
using sharetokens. As only the "sharetoken" part is handled by the
"PublicShareAuth" middleware, we needed to special-case it a bit.

* proxy: Clarify comment

* proxy: Avoid sorting endpoints for every single request

The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: #4497

* Automated changelog update [skip ci]

* update reva to latest edge

* remove personal spaces as admin in the graph test suite

* use id to delete project spaces

* Add context in behat.yml

* Fix home space deletion when deleting user by name

DELETE requess on /graph/v1.0/users also work when specifing a user by
name. For deleting the home space in that case we need to get the User's
id from the backend first.

Fixes: #4195

* Add new "delete-all-spaces" permission

This is assigned to the Admin role by default and allows to cleanup
orphaned spaces (e.g. where the owner as been deleted)

Fixes: #4196

* Automated changelog update [skip ci]

* Automated changelog update [skip ci]

* update reva to latest edge

* Automated changelog update [skip ci]

* delete users with their personal spaces

* update reva to latest edge

* delete method

* Remove tests from unrelated issue and add actual one

* REPORT: add permissions to personal space

Signed-off-by: jkoberg <[email protected]>

* changelog and bump reva

Signed-off-by: jkoberg <[email protected]>

* Automated changelog update [skip ci]

* update reva and jsoncs3 share manager config

* use experimental reva

Signed-off-by: jkoberg <[email protected]>

* fix search service after merge

Signed-off-by: jkoberg <[email protected]>

* praise the linting god

Signed-off-by: jkoberg <[email protected]>

* use cs3 share manager

Signed-off-by: jkoberg <[email protected]>

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Co-authored-by: André Duffeck <[email protected]>
Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
Co-authored-by: Christian Richter <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Viktor Scharf <[email protected]>
Co-authored-by: David Christofas <[email protected]>
Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Ralf Haferkamp <[email protected]>
Co-authored-by: Swikriti Tripathi <[email protected]>
Co-authored-by: Jannik Stehle <[email protected]>
Co-authored-by: Phil Davis <[email protected]>
Co-authored-by: Swikriti Tripathi <[email protected]>
Co-authored-by: Jannik Stehle <[email protected]>
Co-authored-by: Sagar Gurung <[email protected]>
Co-authored-by: Amrita <[email protected]>
Co-authored-by: Michael Barz <[email protected]>
Co-authored-by: amrita <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Type:Regression
Projects
Archived in project
4 participants