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

Use the file owner from the share object, if available #88

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

pranavk
Copy link
Collaborator

@pranavk pranavk commented Jun 1, 2017

Fixes #85

When a link is shared on external storage and user is not logged in, the
owner returned by the $file object is null. Use owner information from
the $share object whenever available.

Fixes nextcloud#85

When a link is shared on external storage and user is not logged in, the
owner returned by the $file object is null. Use owner information from
the $share object whenever available.
@busted-it-guy
Copy link

Just to make sure I am understanding, I should make sure the owner of the files/directories in Windows ntfs permissions is the user I am mounting the share folder with?

I just tried it and I am still getting the Unauthorized WOPI host even after reboot.

I went to the security tab in the windows folder I am sharing. I set the Owner of the folder and everything below it to a username named filetransfer. That is the domain credential I use to mount the SMB share via the External Storage Support application. I have also tried to use the External Storage Support application to mount local storage that is mounted to the smb location. The domain is domain.local in this example. The server is servername.

External Storage Support app settings:
Folder name = Folder
External Storage = SMB/CIFS
Authentication = Username and Password
Host = servername.domain.local
Share = Folder
Domain = domain.local
Username = filetransfer
Password = (Password from Active Directory confirmed working)

In the Security tab, under Advanced button> Owner tab> Owner = filetransfer ([email protected])
All Files and folders below that folder also reflect this

My permissions in the mount I connected to /mnt/folder is

-rwxr-xr-x 1 www-data www-data

..but that shouldn't matter because I get the same results whether I point to a local mount point or if I use the SMB/CIFS option and put in the settings above.

What do you think?

@busted-it-guy
Copy link

Well here is something to narrow it down.

I tried to use the "External Storage Support" application to attach local storage. The local storage I created was at /home/test. I gave chmod 777 to it and chowner is www-data:www-data.

I can edit the files logged in with no issue. If I "Share Link" and attempt to use that link anywhere else I get the UNAUTHORIZED WOPI HOST.. message.

So with no SMB/CIFS in the mix, attached local storage from /home/test will give the same error if you try to access the share link without being logged in.

@pranavk
Copy link
Collaborator Author

pranavk commented Jun 2, 2017

I quickly tried it in my local setup last night by enabling the external storage application and adding a local directory there. I shared a public link for one of the files in the directory and tried to access that when user is not logged-in and I got an error message.

With this patch, it fixed it for me.

Share owner as mentioned in the commit message is simply the owner who shared the file; it has nothing to do with actual share owner.

Since, it still doesn't fix it for you, your loolwsd logs would be helpful. Can you please get that file and attach it here ? And please bump the logging level there to the highest which is trace. If you are using the docker image, then you need to go inside the docker, edit /etc/loolwsd/loolwsd.xml and change the logging level under xml tag to trace, and restart the container. docker logs -f <containerid> will start giving you more elaborate logs then. Attach those here while you reproduce the situation at your end.

@busted-it-guy
Copy link

It doesn't look like I have a directory /etc/loolwsd

And I don't think I am following you. Are you saying that this patch fixed your issue to "share link" to local storage connected via the External Storage. So once you changed permissions or owner you received no error?

When you mean owner are you talking about the username of the person who clicked "share link"?

@pranavk
Copy link
Collaborator Author

pranavk commented Jun 2, 2017

And I don't think I am following you. Are you saying that this patch fixed your issue to "share link" to local storage connected via the External Storage. So once you changed permissions or owner you received no error?

Yeah, this fixed my issue to share link to local storage. I didn't change any permission or owner.

When you mean owner are you talking about the username of the person who clicked "share link"?

Yes.

@busted-it-guy
Copy link

I am working on the logs right now, but I think I might not have applied the same "patch" as you did.

All I did was change ownership of the files on the windows server to the same user account I was using to access the share.

How specifically did you apply this "patch"? Do I have to take anything from your code and modify anything on my end? This is the first time I am working with a developer on github so sorry for my lack of knowledge.

@busted-it-guy
Copy link

And to expound upon the fix you ran, I have a user named "admin" in Nextcloud that shares out the file. Are you saying the person who shared the fikle should be listed in file permissions as the owner? so for that specific file or folder the owner should not be www-data but the owner who shared the file?

Unfortunately I will have many users sharing files they either did or did not create so I have no real way of trying your "patch". I can create a username in Ubuntu as "admin" and give that user ownership but I'm not sure if that is what you want me to test.

@pranavk
Copy link
Collaborator Author

pranavk commented Jun 2, 2017

How specifically did you apply this "patch"? Do I have to take anything from your code and modify anything on my end?

Yes, you need to apply that patch in your /apps/richdocuments/ directory obviously. Did you not do that earlier ?

@busted-it-guy
Copy link

I did not know that earlier. As you can see , I am not familiar.

I have found the actual patch code and applied it to the TokenManager.php

Now it seems to work perfectly! Local storage with no SMB mount.. local storage mounted to SMB location, and using SMB/CIFS to directly mount work with the "Share Link" option.

Thanks so much! Sorry for the confusion

@pranavk
Copy link
Collaborator Author

pranavk commented Jun 2, 2017

No problems. As you can see this is still a pull request ( not yet merged to master ). It will first be merged to master branch. Even then you won't be able to use it directly. Only developers with a git clone will be able to use it. Then @timar will release next version of richdocuments, upload it to app store and then it will show up in the app store which you can then update. So it takes some time for code to travel all the way through to your box ;-) unless of course you are quick enough to apply the patch yourself ;)

@busted-it-guy
Copy link

Thanks! I'll keep fixing until it has made it to the next version of richdocuments

@pranavk pranavk merged commit 7fce081 into nextcloud:master Jun 5, 2017
@pranavk pranavk deleted the extshare branch June 26, 2017 10:37
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.

2 participants