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

Expose session based authentication through mount point type #23261

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

juliushaertl
Copy link
Member

With this apps can check whether an external storage mountpoint uses credentials stored in the session and can act upon that. An example scenario would be Collabora/ONLYOFFICE or the direct editing endpoint on mobile/desktop clients that cannot be used with session based auth since the session is not available in the unauthenticated requests.

Reference implementation for Collabora nextcloud/richdocuments#1178

public function getMountType() {
return 'external';
return ($this->storageConfig->getAuthMechanism() instanceof SessionCredentials) ? 'external-session' : 'external';
Copy link
Member Author

Choose a reason for hiding this comment

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

There seems to be some code in place where for the mount root 'external-root' is set for this, but I cannot figure out currently where. Maybe @icewind1991 knows, since this seems to be one thing that doesn't work anymore with this change.

@icewind1991
Copy link
Member

I'm not a fan of re-using the mount point field for this, since it can cause issues for code relying on the values of that field.

Adding a separate field would be my preference

@juliushaertl juliushaertl force-pushed the enh/external-storage-session-type branch from 8fb946d to 9af668b Compare October 9, 2020 09:41
@juliushaertl
Copy link
Member Author

I'm not a fan of re-using the mount point field for this, since it can cause issues for code relying on the values of that field.
Adding a separate field would be my preference

Yes, I cleaned that up now.

@juliushaertl juliushaertl force-pushed the enh/external-storage-session-type branch 2 times, most recently from ba9269c to fe940c0 Compare October 9, 2020 11:05
@juliushaertl juliushaertl force-pushed the enh/external-storage-session-type branch from fe940c0 to 1b72185 Compare November 3, 2020 09:28
@juliushaertl
Copy link
Member Author

🏓 for review again

@rullzer rullzer mentioned this pull request Dec 14, 2020
59 tasks
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense

@ChristophWurst
Copy link
Member

needs rebase

@rullzer rullzer mentioned this pull request Dec 18, 2020
39 tasks
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl force-pushed the enh/external-storage-session-type branch from 1b72185 to 35e5cc8 Compare December 21, 2020 21:56
@juliushaertl
Copy link
Member Author

Rebased

@faily-bot
Copy link

faily-bot bot commented Dec 21, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 633: failure

mysql8.0-php7.4

Show full log
There was 1 error:

1) OCA\DAV\Tests\unit\CardDAV\CardDavBackendTest::testMultipleUIDDenied
TypeError: Argument 2 passed to OCA\DAV\Events\AddressBookCreatedEvent::__construct() must be of the type array, null given, called in /drone/src/apps/dav/lib/CardDAV/CardDavBackend.php on line 458

/drone/src/apps/dav/lib/Events/AddressBookCreatedEvent.php:51
/drone/src/apps/dav/lib/CardDAV/CardDavBackend.php:458
/drone/src/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php:400

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

integration-ldap-openldap-uid-features

  • failure block could not be found - most likely this run got canceled
Show full log
+ bash tests/drone-run-integration-tests.sh || exit 0
=========================
= List of changed files =
=========================
apps/files_external/lib/Config/ConfigAdapter.php
apps/files_external/lib/Config/ExternalMountPoint.php
apps/files_external/lib/Lib/PersonalMount.php
apps/files_external/tests/PersonalMountTest.php
=========================
PHP files are modified
+ ./occ maintenance:install --admin-pass=admin --data-dir=/dev/shm/nc_int
Nextcloud was successfully installed
+ ./occ config:system:set redis host --value=cache
System config value redis => host set to string cache
+ ./occ config:system:set redis port --value=6379 --type=integer
System config value redis => port set to integer 6379
+ ./occ config:system:set redis timeout --value=0 --type=integer
System config value redis => timeout set to integer 0
+ ./occ config:system:set --type string --value "\OC\Memcache\Redis" memcache.local
System config value memcache.local set to string \OC\Memcache\Redis
+ ./occ config:system:set --type string --value "\OC\Memcache\Redis" memcache.distributed
PHP Warning:  Redis::connect(): php_network_getaddresses: getaddrinfo failed: Name or service not known in /drone/src/lib/private/RedisFactory.php on line 92
An unhandled exception has been thrown:
RedisException: Redis server went away in /drone/src/lib/private/Memcache/Redis.php:55
Stack trace:
#0 /drone/src/lib/private/Memcache/Redis.php(55): Redis->get('c6bc51848682557...')
#1 /drone/src/lib/private/App/InfoParser.php(58): OC\Memcache\Redis->get('/drone/src/apps...')
#2 /drone/src/lib/private/App/AppManager.php(511): OC\App\InfoParser->parse('/drone/src/apps...')
#3 /drone/src/lib/private/legacy/OC_App.php(589): OC\App\AppManager->getAppInfo('files', false, NULL)
#4 /drone/src/lib/private/AppFramework/App.php(70): OC_App::getAppInfo('files')
#5 /drone/src/lib/private/legacy/OC_App.php(270): OC\AppFramework\App::buildAppNamespace('files')
#6 /drone/src/lib/private/AppFramework/Bootstrap/Coordinator.php(108): OC_App::registerAutoloading('files', '/drone/src/apps...')
#7 /drone/src/lib/private/AppFramework/Bootstrap/Coordinator.php(82): OC\AppFramework\Bootstrap\Coordinator->registerApps(Array)
#8 /drone/src/lib/base.php(644): OC\AppFramework\Bootstrap\Coordinator->runInitialRegistration()
#9 /drone/src/lib/base.php(1091): OC::init()
#10 /drone/src/console.php(48): require_once('/drone/src/lib/...')
#11 /drone/src/occ(11): require_once('/drone/src/cons...')
#12 {main}```
</details>


This was referenced Dec 28, 2020
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@MorrisJobke MorrisJobke merged commit 275f15c into master Jan 7, 2021
@MorrisJobke MorrisJobke deleted the enh/external-storage-session-type branch January 7, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants