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

Dont assume everyone ace exists #11

Merged

Conversation

telamonian
Copy link
Contributor

Fixes #9

Pretty much does what I suggested in #9 (comment):

New suggestion:

- If an ACE does not exist, set its mask to `0x0`

- Elevate all rights to at least those of "everyone" by combining masks using bitwise OR `|`

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #11   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          366       367    +1     
=========================================
+ Hits           366       367    +1     
Impacted Files Coverage Δ
fs/smbfs/smbfs.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4881635...cf4f966. Read the comment docs.

@telamonian
Copy link
Contributor Author

telamonian commented Apr 14, 2020

@althonos This one is going to be kind of tricky to set up tests for. What would you suggest?

Update

I can at least confirm that this PR does indeed solve the permissions issue I was running into on my CI:

https://ci.appveyor.com/project/telamonian/jupyter-fs/builds/32177461

Fully sucessful example of using a tool based on jupyter -> fs.smbfs -> pysmb on Windows. Wooo!

@telamonian telamonian closed this Apr 14, 2020
@telamonian telamonian reopened this Apr 14, 2020
- also elevates all rights to at least those of everyone by combining
  masks via bitwise or
@telamonian
Copy link
Contributor Author

telamonian commented May 11, 2020

Hey @althonos. So it turns out that I can't do the next release of jupyter-fs until I resolve this PR; I can't release a package on pypi that contains a dependency on a github branch (ie this PR branch).

What are your thoughts on this PR? Any chance we can get it pulled in? What's the current blocker?

@telamonian
Copy link
Contributor Author

Also, I recently had a chance to chat with one of the Gnome VFS devs. They too have a SMB backend, and they've been working on it for quite a while. Sadly, he didn't have any deep advice to offer about the ACL -> posix-like permissions translation issue. There doesn't seem to be any standard lib for this, and, even worse, apparently different implementations of SMB treat ACLs somewhat differently. So one way or the other I think we're stuck with inferring what we can and rolling our own permissions translation implementation.

@althonos
Copy link
Owner

@telamonian : sorry, real work was hitting hard so I didn't check this. I don't have much ways to check this, since I don't known a lot about Windows permissions (I only know they allow too much combinations to be modelled by UNIX permissions), so I guess I can merge this; I thought this was still WIP.

@althonos althonos merged commit 2df1e4c into althonos:master May 13, 2020
@telamonian
Copy link
Contributor Author

telamonian commented May 13, 2020

sorry, real work was hitting hard so I didn't check this

No worries. I am deeply familiar with the ways of the PhD candidate life.

At least w.r.t. my own code this PR is fairly well battled tested at this point. I'm looking into adding windows ci to this repo. It looks like it will be a fairly complex process, so I think it's best saved for another PR. Once that's in place we'll be able to add some windows-specific permissions tests to the ci

@telamonian
Copy link
Contributor Author

Thanks for merging! Could I possibly impose on you to also do a new release?

@althonos
Copy link
Owner

The release is uploaded (v0.6.2)!

I have experience with setting up an AppVeyor in some other projects, but the current test harness relies on Docker so they would probably not play well together. But if AppVeyor VMs spawn a SMB server, that could be feasible!

@telamonian
Copy link
Contributor Author

But if AppVeyor VMs spawn a SMB server

Yeah, I already learned how to set up that exact thing in appveyor for the ci in jupyter-fs. There's a couple of powershell scripts that make the windows smb magic happen and expose a live share:

https://github.com/jpmorganchase/jupyter-fs/blob/master/ci/enable_os_share_windows.ps1
https://github.com/jpmorganchase/jupyter-fs/blob/master/ci/create_os_share_windows.ps1

telamonian added a commit to telamonian/jupyter-fs that referenced this pull request May 13, 2020
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.

_make_access_from_sd assumes that an "everyone" ACE exists, which sometimes isn't true on Windows
2 participants