-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Split FilePath constructor to guarantee constructor with AmbientAuth #190
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
- Feature Name: Separate `FilePath` constructor from `AmbientAuth` | ||
- Start Date: 2021-07-02 | ||
- RFC PR: | ||
- Pony Issue: | ||
|
||
# Summary | ||
|
||
It should be without error to constructor a `FilePath` when `AmbientAuth` is provided. The current single constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail. | ||
|
||
# Motivation | ||
|
||
Beyond making using `FilePath` more convenient, this change will allow use cases where a path is guaranteed such as constructing a path to the root directory. Currently, all `FilePath` must be constructed within a try-else block. Importantly, `FilePath` does not guarantee its operations will succeed, however a `FilePath` is merely a combination of some directory path and capabilities. No changes to __using__ a `FilePath` are intended by this change. | ||
|
||
# Detailed design | ||
|
||
The suggested change is to split the implementation in two around the current match statement with `create` using `AmbientAuth` and a new constructor called `from` using an existing `FilePath`. | ||
|
||
Current constructor (documentation excluded): | ||
|
||
```pony | ||
new val create( | ||
base: (FilePath | AmbientAuth), | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
? | ||
=> | ||
caps.union(caps') | ||
|
||
path = match base | ||
| let b: FilePath => | ||
if not b.caps(FileLookup) then | ||
error | ||
end | ||
|
||
let tmp_path = Path.join(b.path, path') | ||
caps.intersect(b.caps) | ||
|
||
if not tmp_path.at(b.path, 0) then | ||
error | ||
end | ||
tmp_path | ||
| let b: AmbientAuth => | ||
Path.abs(path') | ||
end | ||
``` | ||
|
||
Will become: | ||
|
||
```pony | ||
new val create( | ||
base: AmbientAuth, | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
=> | ||
caps.union(caps') | ||
|
||
path = Path.abs(path') | ||
``` | ||
|
||
and | ||
|
||
```pony | ||
new val from( | ||
base: FilePath, | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
? | ||
=> | ||
caps.union(caps') | ||
if not base.caps(FileLookup) then | ||
error | ||
end | ||
|
||
let tmp_path = Path.join(base.path, path') | ||
caps.intersect(base.caps) | ||
|
||
if not tmp_path.at(base.path, 0) then | ||
error | ||
end | ||
path = tmp_path | ||
``` | ||
|
||
# How We Teach This | ||
|
||
Nothing about our teaching should need to change, however this change will allow early tutorials to avoid introducing try-else blocks and partial functions for longer if they so choose. For example, with a separate `FilePath` constructor an early tutorial could construct a `FilePath` for a file in the current directory containing a simple message, constructor the necessary `File`, and call `File.read()` or `File.lines`, none of which would be partial functions after this change -- such a program is just beyond the complexity of the ever ubiquitous "Hello, World" initial lesson. | ||
|
||
Advanced users of Pony, should have little difficulty switching based on the simple statistics from stdlib below. | ||
|
||
- Uses of `FilePath`: 166 | ||
- Uses of `FilePath(...)` from `AmbientAuth`: 37 | ||
- Uses of `_ as FilePath` typing: 19 | ||
- Uses of `FilePath(...)` not from `AmbientAuth`: 29 | ||
- Uses referring to `FilePath` for its type (e.g., within unions): 53 | ||
- Uses excluding the above (mostly documentation): 28 | ||
|
||
# How We Test This | ||
|
||
At least one additional unit test is recommended as nearly all existing tests use `AmbientAuth`. It should suffice for this added unit test to first build a path via `AmbientAuth` then build a second path from the first. | ||
|
||
Existing unit tests will need to change as the common phrase `let filepath = FilePath(h.env.root as AmbientAuth, path)?` will not fail. As well, `var tmp_dir: (FilePath | None) = ...` will likely not be needed as this is a defense against `FilePath` failing within a try statement. | ||
|
||
# Drawbacks | ||
|
||
- Breaks existing code | ||
- Added maintenance cost | ||
- May lead to passing `AmbientAuth` around excessively since building from an existing path (`FilePath.from...)`) will become less appealing | ||
|
||
I am not sure we can change how this RFC is implemented to minimize any of the above. The first two are expected with additional breaking code as important as a constructor. The latter is a training issue so is best handled by better educational materials around Pony for _why_ it would be a "bad idea" to pass `AmbientAuth` around excessively. | ||
|
||
# Alternatives | ||
|
||
Keeping the implementation as it currently is written. There is nothing wrong with the existing implementation, this RFC is a suggestion for making the file package slightly easier to use. | ||
|
||
# Unresolved questions | ||
|
||
Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be mitigated if we added a new
*Auth
capability that only works for creating files, similar to what we do in thenet
package (UPDSocketAuth
,TCPListenerAuth
, etc.)Let's say I create a library that only needs access to the filesystem, but not networking. It's true that the user could supply a crafted
FilePath
instance, but maybe they don't care about specifying the path, or capabilities of that path: they only care about authorizing the library to modify the filesystem.This new Auth class can be implemented in terms of FilePath:
FilePath.create(AmbientAuth, "/", recover val FileCaps .> all() end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand what you mean. We have a capability only for creating files
FileCreate
. UsingFilePath.create(AmbientAuth, "/", recover val FileCaps .> all() end)
would mean the ability to create anyFilePath
with any capabilities via the suggestedFilePath.from(...)
constructor so I do not see what that is fixing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might have explained myself badly.
What I mean (and what you also put in the RFC), is that by taking
AmbientAuth
,FilePath.create
incentivizes library authors that need to access the filesystem to ask the user for a very coarse-grained capability.Imagine you're a library author: your library is only interested in using the "files" package, but you don't necessarily want to ask the user to pass your library a
FilePath
class (maybe you don't want the user to think about paths). Your other option is to ask the user forAmbientAuth
, and you will use this auth inside your library to createFilePath
classes as you need them.However,
AmbientAuth
allows you do to much more than creatingFilePath
s: since it is the root Auth, you can use it to create other auths likeTCPListenerAuth
and open network connections behind the user's back.My suggestion here is to create a new *Auth primitive, for example
FileAuth
, and use that as a parameter forFilePath.create
, instead of usingAmbientAuth
. That way, we use an auth that is only used for filesystem access, much like we do in thenet
package already.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot more sense now. Thank you for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of this, but exactly how we want it to be implemented is a topic of discussion. Integrating it with this RFC I see the same split occurring so we have:
and
where
FileAuth
can be created viaAmbientAuth
viaThis
FileAuth
would then be used to produce any lower auths.My primary concern with this is that it seems dangerously close to duplicating the purpose of
FileCaps
. I want to push this forward as I prefer the pattern of a package having its own dedicated root auth -- when the alternative is passingAmbientAuth
around without much regard for how that would then allow use within any yet-written package. By creating our own root auth we prevent this before it can become a problem and anyone withAmbientAuth
is only a singleFileAuth(AmbientAuth)
away from the solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a FileAuth. I think its reasonable and matches up with how the net package works.
It's a pretty easy thing for a user to fix. I'd be in favor of expanding this to include that change.
I'm also ok with it staying with AmbientAuth for now.
I don't think that FileAuth is too close to FileCaps. FileAuth is "you can do file stuff", file caps is specific file operations you can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the same page as Sean.
I particularly don't think there is significant duplication, because in the past I have advocated for the
net
package to get something akin toFileCaps
(e.g. I'll allow you to open a single listener within this port range, but you can do nothing else with networking apart from that).So like Sean, I don't see any troubling overlap between "you can do any file stuff" and "you can do this specific subset of file stuff".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I will expand the RFC this evening to include a new
FileAuth
package-level root.