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

Fix exception on external storages public links when encryption enabled #41246

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pako81
Copy link

@pako81 pako81 commented May 6, 2024

Description

Having masterkey encryption enabled, when trying to upload into a public link created on an external storage (i.e. of type "Local" or "WND") being assigned to at least one user/group, an exception Attempted to initialize mount points for null user and no user in session is being thrown and no upload is possible.

Related Issue

Motivation and Context

There was a problem around https://github.com/owncloud/core/blob/master/lib/private/Encryption/Util.php#L303 as the uid provided to the method is false in this case. This happens because the external storage doesn't really have an owner and there is no user in the session. Therefore, when checking for isMountPointApplicableToUser, the provided user is false which will never find a match, so the mount point isn't considered system wide.

How Has This Been Tested?

  1. enable masterkey encryption
  2. create an external storage through the "local" option (also reproducible with WND) and mount it for user admin
  3. as user admin creates a public link on the external storage root
  4. try to upload into the created public link: an exception Attempted to initialize mount points for null user and no user in session is shown and file cannot be uploaded
  5. try to download an existing file works. Try to download any other file added by user admin after the storage has been mounted into oC fails with File cannot be downloaded and same exception in logs
  6. With this change in place both exceptions at 4. and 5. are solved.

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

@pako81 pako81 added this to the development milestone May 6, 2024
@pako81 pako81 requested review from IljaN and jvillafanez May 6, 2024 07:36
@pako81 pako81 self-assigned this May 6, 2024
Copy link

sonarcloud bot commented May 6, 2024

@jvillafanez
Copy link
Member

There was an additional problem according to https://github.com/owncloud/enterprise/issues/6626#issuecomment-2084853496 , is it solved? or will it be fixed later?

@pako81
Copy link
Author

pako81 commented May 6, 2024

There was an additional problem according to https://github.com/owncloud/enterprise/issues/6626#issuecomment-2084853496 , is it solved? or will it be fixed later?

I guess we can try to fix it here. But I don't quite get why we are not getting into https://github.com/owncloud/core/blob/master/lib/private/Preview.php#L1229-L1231 as $user should be empty at that point.

@jvillafanez
Copy link
Member

https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1475
The user isn't empty. It's likely a RemoteUser.

The case where the user is false isn't being handled properly. Even if the storage returns a null or false owner, the view will consider it as a remote user. I guess that's what the Filesystem::getOwner is returning.

@pako81
Copy link
Author

pako81 commented May 7, 2024

I guess that's what the Filesystem::getOwner is returning.

AFAICT from https://github.com/owncloud/core/blob/master/lib/private/Preview.php#L1228 we get false back.

@phil-davis
Copy link
Contributor

phil-davis commented May 7, 2024

Note: a changelog was added by #41247 but was named https://github.com/owncloud/core/blob/master/changelog/unreleased/41246

So that is why there is a conflict here!

I made #41248 to fix that.

@owncloud owncloud deleted a comment from update-docs bot May 7, 2024
@jnweiger
Copy link
Contributor

jnweiger commented Jul 9, 2024

@pako81 is this ready to go into 10.15.0?

@jnweiger
Copy link
Contributor

jnweiger commented Jul 9, 2024

After 10.15.0 - converting to draft for now.

@jnweiger jnweiger marked this pull request as draft July 9, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants