support for per-user encryption

[summersab]

Implementing PR #498 from immerda. It's a feature I'd like to have, and he hasn't been active on GitHub since December. So, I'd like to submit the PR.

[summersab]

I'm unsure what I need to do to fix the CI error. Any help?

[MichaIng]

Regarding nextcloud/server#27929 (comment) @summersab you mean the CI drone check? Not sure either, it says the "master" and "stable21" integration tests were cancelled, while the "stable22" integration tests was done and succeeded. Not sure how those work. I just confirmed to run another check (composer) which went well, but I don't think that is related to what drone does. Can you rebase onto current master, so I can see whether drone fails exact the same way?

[juliusknorr]

Failures seemed to be an unrelated timeout, I've retriggered the CI.

[juliusknorr]

Waiting for server PR to be finalized, with a new interface

[immerda]

I tried this patch and there are currently some issues:

The UserBackend is missing a implements IProvideUserSecretBackend. For that reason the secret is never requested by https://github.com/nextcloud/server/pull/27929/files#diff-b30a8c4cef5da5cede4d9038c16ede43e1746f85270e4249598fd305b0fa77deR182

The getCurrentUserSecret is missing the :string annotation from the interface.

getUserSecret returns null, which contradicts the type of the interface.

[MichaIng]

Whether it's string or ?string needs to follow the interface vice versa and there was no final decision about yet: https://github.com/nextcloud/server/pull/27929/files#r695632608

Generally null passwords were made explicitly possible here: nextcloud/server#29200
And/as of: nextcloud/server#29187

But I lack the overview to reliably follow how everything is linked. As passwords intentionally were made nullable, looks like using ?string is the logical consequence. The other idea would be to conditionally implement the interface only if a password is actually provided. The same way on the current server-side interface code getCurrentUserSecret is only called if the interface is implemented by the backend, so that null means backend with no user secret support/enabled and a string means the backend supports it and then is also expected to provide it, hence a more explicit solution.

[immerda]

@MichaIng thanks for bringing me up to speed on the discussion!

@summersab I had some time at hand, so here is my working version of this patch, that additionally fixes #547 https://gist.github.com/immerda/527cd5ef0c73cb0e5e4ed6e34c824324

[summersab]

@summersab I had some time at hand, so here is my working version of this patch, that additionally fixes #547 https://gist.github.com/immerda/527cd5ef0c73cb0e5e4ed6e34c824324

@immerda Your patch is causing some errors after I apply it:

Doctrine\DBAL\Exception\NotNullConstraintViolationException
1364
An exception occurred while executing a query: SQLSTATE[HY000]: General error: 1364 Field 'name' doesn't have a default value
/var/www/nextcloud/3rdparty/doctrine/dbal/src/Driver/API/MySQL/ExceptionConverter.php
111

Trace
#0 /var/www/nextcloud/3rdparty/doctrine/dbal/src/Connection.php(1728): Doctrine\DBAL\Driver\API\MySQL\ExceptionConverter->convert()
#1 /var/www/nextcloud/3rdparty/doctrine/dbal/src/Connection.php(1667): Doctrine\DBAL\Connection->handleDriverException()
#2 /var/www/nextcloud/3rdparty/doctrine/dbal/src/Connection.php(1146): Doctrine\DBAL\Connection->convertExceptionDuringQuery()
#3 /var/www/nextcloud/lib/private/DB/Connection.php(262): Doctrine\DBAL\Connection->executeStatement()
#4 /var/www/nextcloud/3rdparty/doctrine/dbal/src/Query/QueryBuilder.php(213): OC\DB\Connection->executeStatement()
#5 /var/www/nextcloud/lib/private/DB/QueryBuilder/QueryBuilder.php(287): Doctrine\DBAL\Query\QueryBuilder->execute()
#6 /var/www/nextcloud/apps/user_saml/lib/UserBackend.php(198): OC\DB\QueryBuilder\QueryBuilder->execute()
#7 /var/www/nextcloud/apps/user_saml/lib/UserBackend.php(750): OCA\User_SAML\UserBackend->updateUserSecretHash()
#8 /var/www/nextcloud/apps/user_saml/lib/Controller/SAMLController.php(147): OCA\User_SAML\UserBackend->updateAttributes()
#9 /var/www/nextcloud/apps/user_saml/lib/Controller/SAMLController.php(335): OCA\User_SAML\Controller\SAMLController->autoprovisionIfPossible()
#10 /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php(217): OCA\User_SAML\Controller\SAMLController->assertionConsumerService()
#11 /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php(126): OC\AppFramework\Http\Dispatcher->executeController()
#12 /var/www/nextcloud/lib/private/AppFramework/App.php(156): OC\AppFramework\Http\Dispatcher->dispatch()
#13 /var/www/nextcloud/lib/private/Route/Router.php(301): OC\AppFramework\App::main()
#14 /var/www/nextcloud/lib/base.php(1000): OC\Route\Router->match()
#15 /var/www/nextcloud/index.php(36): OC::handleRequest()
#16 {main}

I'm pretty sure the rest of my code is clean and unmodified. Any clue?

[immerda]

@summersab

Your patch is causing some errors after I apply it:

hah, good that somebody else tests it :) I used this unused table user_saml_auth_token to store the hashes, since it was already there... But that table seems to also have a field called name, for some reason and depending on the database backend, it complains if we don't set it to something.

so let's set the name to sso_secret_hash, it could be useful in the future if there are multiple kind of tokens...

I have update the patch. basically added lines 38, 67 and 73

once it is working you should see hashes being inserted into user_saml_auth_token

[summersab]

Seems to be working, @immerda! I'll poke at it a little more over the weekend just to be sure.

[immerda]

@summersab sorry, found another bug that is only triggered when the token exires. updated my gist. line 65 needs to be $qb->createNamedParameter($hash) instead of just $hash.

[summersab]

You're a good man*, @immerda - thanks!

*Or woman - don't want to assume.

[MichaIng]

nextcloud/server#27929 has been merged, so work here can go on. When the new interface is used, a password must be set, the string is not nullable at server side.