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

Support SFTP paths to root or home directory #5

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

jgogstad
Copy link
Collaborator

@jgogstad jgogstad commented Feb 16, 2020

This PR introduces a new test container that configures the SFTP server without the ChrootDirectory directive. This causes users to land in /home/<username>/ instead of / (ChrootDirectory %h maps the user's home directory to /).

In order for tests to pass on both server configuration, we must support relative paths. This PR introduces that support.

Discussion, not strictly related to PR contents

I think the Path abstraction needs to be augmented or adjusted in order to fully support SFTP. It currently leaks assumptions about the underlying storage, i.e. there's always two components to paths (root and key) and there's no separation between absolute and relative paths. This doesn't hold up for SFTP stores where the following paths are all valid and pointing to different files (ref spec#section-6):

  1. Absolute path "/foo.txt"
  2. Relative to CWD path "./foo.txt"
  3. Relative to user home path "foo.txt"

Since we don't surface any APIs for changing CWD, we can probably get away with assuming CWD == user home.

Any views on a better API is welcome. I think we should encode the fact that blobstore paths are different from filestore paths (e.g. SFTP) and introduce something like

sealed trait Path
object Path {
  case class BlobstorePath(scheme:String, bucketName: String, path: String) extends Path
  case class FilestorePath(path: String) extends Path
}

trait Store[F[_]] {
  type PathType
}

Copy link
Contributor

@rolandomanrique rolandomanrique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path abstraction is certainly hard to generalize and still support all stores native features (e.g. s3 store must have size on put or could support per path encryption).

We did make that compromise that if you need different settings per file Path you would need different Store instances (at LendUp I had to do this for files with different encryption algorithms).

Our use case for SFTP (and also box.com store) was that we had to restrict users to a specific dir just like a “bucket” so this is why we never added support for abs paths.

All these assumptions can certainly be revised and improved.

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #5 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #5   +/-   ##
=======================================
  Coverage   67.57%   67.57%           
=======================================
  Files           8        8           
  Lines         293      293           
  Branches       11        8    -3     
=======================================
  Hits          198      198           
  Misses         95       95           
Flag Coverage Δ
#scala_2_12_10 67.57% <100.00%> (ø)
#scala_2_13_1 67.58% <100.00%> (+0.22%) ⬆️

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 7a3ba09...4c6d45b. Read the comment docs.

Comment on lines -80 to -96
for{
q <- fs2.Stream.eval(Queue.bounded[F, Option[ChannelSftp#LsEntry]](64))
channel <- fs2.Stream.resource(channelResource)
entry <- q.dequeue.unNoneTerminate.filter(e => e.getFilename != "." && e.getFilename != "..").concurrently(
fs2.Stream.eval(
blocker.blockOn(F.flatMap(F.attempt(F.delay(channel.ls(path, entrySelector(e => F.runAsync(q.enqueue1(Some(e)))(_ => IO.unit).unsafeRunSync())))))(_ => q.enqueue1(None)))
for {
q <- fs2.Stream.eval(Queue.bounded[F, Option[ChannelSftp#LsEntry]](64))
channel <- fs2.Stream.resource(channelResource)
entry <- q.dequeue.unNoneTerminate.filter(e => e.getFilename != "." && e.getFilename != "..").concurrently(
fs2.Stream.eval(
blocker.blockOn(F.flatMap(F.attempt(F.delay(channel.ls(path, entrySelector(e => F.runAsync(q.enqueue1(Some(e)))(_ => IO.unit).unsafeRunSync())))))(_ => q.enqueue1(None)))
)
)
} yield {
val newPath = if (path.filename == entry.getFilename) path else path / entry.getFilename
newPath.copy(
size = Option(entry.getAttrs.getSize),
isDir = entry.getAttrs.isDir,
lastModified = Option(entry.getAttrs.getMTime).map(i => new Date(i.toLong * 1000))
)
)
} yield {
val newPath = if (path.filename == entry.getFilename) path else path / entry.getFilename
newPath.copy(
size = Option(entry.getAttrs.getSize),
isDir = entry.getAttrs.isDir,
lastModified = Option(entry.getAttrs.getMTime).map(i => new Date(i.toLong * 1000))
)
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just formatting here

@jgogstad jgogstad merged commit 75bf28f into master Mar 4, 2020
@jgogstad jgogstad deleted the improve-sftp-path-tostring branch March 4, 2020 12:20
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.

None yet

3 participants