Skip to content

Commit cf73440

Browse files
icewind1991icewind1991
authored
Merge pull request #40183 from nextcloud/sftp-fixes
SFTP improvements
2 parents ee90109 + d42d809 commit cf73440

File tree

3 files changed

+226
-9
lines changed

3 files changed

+226
-9
lines changed

‎.github/workflows/sftp.yml‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
name: SFTP unit tests
2+
on:
3+
push:
4+
branches:
5+
- master
6+
- stable*
7+
paths:
8+
- 'apps/files_external/**'
9+
pull_request:
10+
paths:
11+
- 'apps/files_external/**'
12+
13+
env:
14+
APP_NAME: files_external
15+
16+
jobs:
17+
sftp-tests:
18+
runs-on: ubuntu-latest
19+
20+
if: ${{ github.repository_owner != 'nextcloud-gmbh' }}
21+
22+
strategy:
23+
# do not stop on another job's failure
24+
fail-fast: false
25+
matrix:
26+
php-versions: ['8.0']
27+
sftpd: ['openssh']
28+
29+
name: php${{ matrix.php-versions }}-${{ matrix.sftpd }}
30+
31+
steps:
32+
- name: Checkout server
33+
uses: actions/checkout@v3
34+
with:
35+
submodules: true
36+
37+
- name: Set up sftpd
38+
run: |
39+
sudo mkdir /tmp/sftp
40+
sudo chown -R 0777 /tmp/sftp
41+
if [[ "${{ matrix.sftpd }}" == 'openssh' ]]; then docker run -p 2222:22 --name sftp -d -v /tmp/sftp:/home/test atmoz/sftp "test:test:::data"; fi
42+
- name: Set up php ${{ matrix.php-versions }}
43+
uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d #v2.25.2
44+
with:
45+
php-version: ${{ matrix.php-versions }}
46+
tools: phpunit:9
47+
extensions: mbstring, fileinfo, intl, sqlite, pdo_sqlite, zip, gd
48+
env:
49+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
50+
51+
- name: Set up Nextcloud
52+
run: |
53+
mkdir data
54+
./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password
55+
./occ app:enable --force ${{ env.APP_NAME }}
56+
php -S localhost:8080 &
57+
- name: PHPUnit
58+
run: |
59+
echo "<?php return ['run' => true, 'host' => 'localhost:2222','user' => 'test','password' => 'test', 'root' => 'data'];" > apps/${{ env.APP_NAME }}/tests/config.sftp.php
60+
phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/SftpTest.php
61+
- name: sftpd logs
62+
if: always()
63+
run: |
64+
ls -l /tmp/sftp
65+
docker logs sftp
66+
67+
sftp-summary:
68+
runs-on: ubuntu-latest
69+
needs: sftp-tests
70+
71+
if: always()
72+
73+
steps:
74+
- name: Summary status
75+
run: if ${{ needs.sftp-tests.result != 'success' }}; then exit 1; fi

‎apps/files_external/lib/Lib/Storage/SFTP.php‎

Lines changed: 117 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,21 @@
3636
*/
3737
namespace OCA\Files_External\Lib\Storage;
3838

39+
use Icewind\Streams\CountWrapper;
3940
use Icewind\Streams\IteratorDirectory;
4041
use Icewind\Streams\RetryWrapper;
42+
use OC\Files\Filesystem;
43+
use OC\Files\Storage\Common;
44+
use OCP\Constants;
45+
use OCP\Files\FileInfo;
46+
use OCP\Files\IMimeTypeDetector;
4147
use phpseclib\Net\SFTP\Stream;
4248

4349
/**
4450
* Uses phpseclib's Net\SFTP class and the Net\SFTP\Stream stream wrapper to
4551
* provide access to SFTP servers.
4652
*/
47-
class SFTP extends \OC\Files\Storage\Common {
53+
class SFTP extends Common {
4854
private $host;
4955
private $user;
5056
private $root;
@@ -56,6 +62,9 @@ class SFTP extends \OC\Files\Storage\Common {
5662
* @var \phpseclib\Net\SFTP
5763
*/
5864
protected $client;
65+
private IMimeTypeDetector $mimeTypeDetector;
66+
67+
const COPY_CHUNK_SIZE = 8 * 1024 * 1024;
5968

6069
/**
6170
* @param string $host protocol://server:port
@@ -111,6 +120,7 @@ public function __construct($params) {
111120

112121
$this->root = '/' . ltrim($this->root, '/');
113122
$this->root = rtrim($this->root, '/') . '/';
123+
$this->mimeTypeDetector = \OC::$server->get(IMimeTypeDetector::class);
114124
}
115125

116126
/**
@@ -370,20 +380,24 @@ public function unlink($path) {
370380
public function fopen($path, $mode) {
371381
try {
372382
$absPath = $this->absPath($path);
383+
$connection = $this->getConnection();
373384
switch ($mode) {
374385
case 'r':
375386
case 'rb':
376-
if (!$this->file_exists($path)) {
387+
$stat = $this->stat($path);
388+
if (!$stat) {
377389
return false;
378390
}
379391
SFTPReadStream::register();
380-
$context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]);
392+
$context = stream_context_create(['sftp' => ['session' => $connection, 'size' => $stat['size']]]);
381393
$handle = fopen('sftpread://' . trim($absPath, '/'), 'r', false, $context);
382394
return RetryWrapper::wrap($handle);
383395
case 'w':
384396
case 'wb':
385397
SFTPWriteStream::register();
386-
$context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]);
398+
// the SFTPWriteStream doesn't go through the "normal" methods so it doesn't clear the stat cache.
399+
$connection->_remove_from_stat_cache($absPath);
400+
$context = stream_context_create(['sftp' => ['session' => $connection]]);
387401
return fopen('sftpwrite://' . trim($absPath, '/'), 'w', false, $context);
388402
case 'a':
389403
case 'ab':
@@ -395,7 +409,7 @@ public function fopen($path, $mode) {
395409
case 'x+':
396410
case 'c':
397411
case 'c+':
398-
$context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]);
412+
$context = stream_context_create(['sftp' => ['session' => $connection]]);
399413
$handle = fopen($this->constructUrl($path), $mode, false, $context);
400414
return RetryWrapper::wrap($handle);
401415
}
@@ -450,14 +464,14 @@ public function rename($source, $target) {
450464
}
451465

452466
/**
453-
* {@inheritdoc}
467+
* @return array{mtime: int, size: int, ctime: int}|false
454468
*/
455469
public function stat($path) {
456470
try {
457471
$stat = $this->getConnection()->stat($this->absPath($path));
458472

459-
$mtime = $stat ? $stat['mtime'] : -1;
460-
$size = $stat ? $stat['size'] : 0;
473+
$mtime = $stat ? (int)$stat['mtime'] : -1;
474+
$size = $stat ? (int)$stat['size'] : 0;
461475

462476
return ['mtime' => $mtime, 'size' => $size, 'ctime' => -1];
463477
} catch (\Exception $e) {
@@ -476,4 +490,99 @@ public function constructUrl($path) {
476490
$url = 'sftp://' . urlencode($this->user) . '@' . $this->host . ':' . $this->port . $this->root . $path;
477491
return $url;
478492
}
493+
494+
public function file_put_contents($path, $data) {
495+
/** @psalm-suppress InternalMethod */
496+
$result = $this->getConnection()->put($this->absPath($path), $data);
497+
if ($result) {
498+
return strlen($data);
499+
} else {
500+
return false;
501+
}
502+
}
503+
504+
public function writeStream(string $path, $stream, int $size = null): int {
505+
if ($size === null) {
506+
$stream = CountWrapper::wrap($stream, function (int $writtenSize) use (&$size) {
507+
$size = $writtenSize;
508+
});
509+
if (!$stream) {
510+
throw new \Exception("Failed to wrap stream");
511+
}
512+
}
513+
/** @psalm-suppress InternalMethod */
514+
$result = $this->getConnection()->put($this->absPath($path), $stream);
515+
fclose($stream);
516+
if ($result) {
517+
return $size;
518+
} else {
519+
throw new \Exception("Failed to write steam to sftp storage");
520+
}
521+
}
522+
523+
public function copy($source, $target) {
524+
if ($this->is_dir($source) || $this->is_dir($target)) {
525+
return parent::copy($source, $target);
526+
} else {
527+
$absSource = $this->absPath($source);
528+
$absTarget = $this->absPath($target);
529+
530+
$connection = $this->getConnection();
531+
$size = $connection->size($absSource);
532+
if ($size === false) {
533+
return false;
534+
}
535+
for ($i = 0; $i < $size; $i += self::COPY_CHUNK_SIZE) {
536+
/** @psalm-suppress InvalidArgument */
537+
$chunk = $connection->get($absSource, false, $i, self::COPY_CHUNK_SIZE);
538+
if ($chunk === false) {
539+
return false;
540+
}
541+
/** @psalm-suppress InternalMethod */
542+
if (!$connection->put($absTarget, $chunk, \phpseclib\Net\SFTP::SOURCE_STRING, $i)) {
543+
return false;
544+
}
545+
}
546+
return true;
547+
}
548+
}
549+
550+
public function getPermissions($path) {
551+
$stat = $this->getConnection()->stat($this->absPath($path));
552+
if (!$stat) {
553+
return 0;
554+
}
555+
if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) {
556+
return Constants::PERMISSION_ALL;
557+
} else {
558+
return Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE;
559+
}
560+
}
561+
562+
public function getMetaData($path) {
563+
$stat = $this->getConnection()->stat($this->absPath($path));
564+
if (!$stat) {
565+
return null;
566+
}
567+
568+
if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) {
569+
$stat['permissions'] = Constants::PERMISSION_ALL;
570+
} else {
571+
$stat['permissions'] = Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE;
572+
}
573+
574+
if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) {
575+
$stat['size'] = -1;
576+
$stat['mimetype'] = FileInfo::MIMETYPE_FOLDER;
577+
} else {
578+
$stat['mimetype'] = $this->mimeTypeDetector->detectPath($path);
579+
}
580+
581+
$stat['etag'] = $this->getETag($path);
582+
$stat['storage_mtime'] = $stat['mtime'];
583+
$stat['name'] = basename($path);
584+
585+
$keys = ['size', 'mtime', 'mimetype', 'etag', 'storage_mtime', 'permissions', 'name'];
586+
return array_intersect_key($stat, array_flip($keys));
587+
}
479588
}

‎apps/files_external/lib/Lib/Storage/SFTPReadStream.php‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ class SFTPReadStream implements File {
4949
private $eof = false;
5050

5151
private $buffer = '';
52+
private bool $pendingRead = false;
53+
private int $size = 0;
5254

5355
public static function register($protocol = 'sftpread') {
5456
if (in_array($protocol, stream_get_wrappers(), true)) {
@@ -75,6 +77,9 @@ protected function loadContext($name) {
7577
} else {
7678
throw new \BadMethodCallException('Invalid context, session not set');
7779
}
80+
if (isset($context['size'])) {
81+
$this->size = $context['size'];
82+
}
7883
return $context;
7984
}
8085

@@ -118,7 +123,25 @@ public function stream_open($path, $mode, $options, &$opened_path) {
118123
}
119124

120125
public function stream_seek($offset, $whence = SEEK_SET) {
121-
return false;
126+
switch ($whence) {
127+
case SEEK_SET:
128+
$this->seekTo($offset);
129+
break;
130+
case SEEK_CUR:
131+
$this->seekTo($this->readPosition + $offset);
132+
break;
133+
case SEEK_END:
134+
$this->seekTo($this->size + $offset);
135+
break;
136+
}
137+
return true;
138+
}
139+
140+
private function seekTo(int $offset): void {
141+
$this->internalPosition = $offset;
142+
$this->readPosition = $offset;
143+
$this->buffer = '';
144+
$this->request_chunk(256 * 1024);
122145
}
123146

124147
public function stream_tell() {
@@ -142,11 +165,17 @@ public function stream_read($count) {
142165
}
143166

144167
private function request_chunk($size) {
168+
if ($this->pendingRead) {
169+
$this->sftp->_get_sftp_packet();
170+
}
171+
145172
$packet = pack('Na*N3', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size);
173+
$this->pendingRead = true;
146174
return $this->sftp->_send_sftp_packet(NET_SFTP_READ, $packet);
147175
}
148176

149177
private function read_chunk() {
178+
$this->pendingRead = false;
150179
$response = $this->sftp->_get_sftp_packet();
151180

152181
switch ($this->sftp->packet_type) {
@@ -195,6 +224,10 @@ public function stream_eof() {
195224
}
196225

197226
public function stream_close() {
227+
// we still have a read request incoming that needs to be handled before we can close
228+
if ($this->pendingRead) {
229+
$this->sftp->_get_sftp_packet();
230+
}
198231
if (!$this->sftp->_close_handle($this->handle)) {
199232
return false;
200233
}

0 commit comments

Comments
 (0)