Allow SSO authentication to provide a user secret

[MichaIng]

Implementing PR #24837 from immerda

As of my failed attempt to fix #27909 😄.

[summersab]

Thanks, @MichaIng! Sorry for the hassle.

Do you think you could give me a hand with figuring out why this PR is failing? It's the companion code for the user_saml app:
nextcloud/user_saml#537

[MichaIng]

CI is happy now.

[LukasReschke]

Would making this nullable make sense? Especially as $password = null; is the default.

Suggested change

	public function getCurrentUserSecret(): string;
	public function getCurrentUserSecret(): ?string;

[MichaIng]

I think the idea was to explicitly not allow a null value here, so that checks can be omitted when the interface is implemented. Backends then need to always provide a string.

Ref: #27929 (comment)

But I see this is inconsistent with the description "Optionally returns a stable per-user secret.", which just has not been changed when I copy&pasted it from IApacheBackend (original idea, where it needed to be optional, of course) to its own interface. Shall I replace with:

* Returns a stable per-user secret e.g. used to secure file encryption keys.

[juliusknorr]

I think I suggested to introduce the non-nullable return value earlier. To me it seems more reasonable as the user secret would be used for the user key encryption where I'd argue that if a backend provides user secrets there should be always one set for the user.

[summersab]

Two comments/issues:

1. I've tried the most recent version of this PR with the IProvideUserSecretBackend in conjunction with the associated user_saml plugin PR:
   support for per-user encryption user_saml#537
   .
   Since users logging in with this method would be using the user_saml backend, lib/private/legacy/OC_User.php:177 never gets executed. Therefore, the password is never set. Am I missing something?
2. With the code from the original PR, there seems to be a problem. After a few minutes, heartbeat requests start returning 401 responses. Then, any getstoragestats request triggers a page refresh after a 401 response because the session is no longer valid. I have yet to determine what is causing the session to fail, but it is a problem.

Any thoughts? I have a local Keycloak -> NC dev setup working, and I'm happy to test.

[MichaIng]

Without a password, getCurrentUserSecret() will return null. So, this suggested change should either be reversed (probably the recommended solution) or a try/catch should be added

Is it possible that user_saml "provides" the IProvideUserSecretBackend only when a user secret is actually set? In theory this would then reduce some overhead, when the backend is used without it and would make allowing/checking for null obsolete, right? But I clearly lack knowledge about theses things, so probably it's nonsense 😄.

[immerda]

I have a proposal to get us unstuck here. How about we make IProvideUserSecretBackend::getCurrentUserSecret() nullable (or conditionally implementing IProvideUserSecretBackend is fine too -- anything that gets us unstuck...), but then do the following:

$password = null;
if ($backend instanceof \OCP\Authentication\IProvideUserSecretBackend) {
  $password = $backend->getCurrentUserSecret();
}
if ($password !== null) {
  $userSession->createSessionToken($request, $uid, $uid, $password);
} else {
  $userSession->createSessionToken($request, $uid, $uid);
}

This would allow us to disentangle the two problems, preserve the identical behavior for the non-userSecret case and let createSessionToken decide what to do when there is no password. This solution would also be more robust, as it removes that dependency on the decision if token passwords are nullable or not and allows us to define an interface that is independent of that.

[MichaIng]

Isn't it exactly the same outcome whether passing $password = null or no argument at all? The createSessionToken has it's fourth argument null in both cases, isn't it?

The main question is indeed whether we want to be more explicit and force usage of a password when the backend is implemented or whether we want to allow implementing the backend without the need to provide a password. Generally I'm fans of being more explicit, but I understand that it may make the backend implementation more complicated.

I think it is agreed that we can or even should merge this PR first, before doing so at the backend, especially since the interfaces are defined here, so the above is really the last open question. I think we need someone who is more in charge to simply do a decision and we'll be able to deal with the result. But we need to get unstuck 😄.

[immerda]

Hi, what is the status of this PR? It seems to me that we have managed to have fairly non-intrusive changes here, that will not change or break any existing interface, but allows us to continue with the saml side of things.

Are there any remaining objections to address before inclusion, @blizzz since you pushed the milestone?

[MichaIng]

Rebased and resolved conflict. We need someone to decide whether the API function should be allowed to return null or not (see discussion in code review).

[immerda]

Two things happened:

1. There is a discussion in this PR if our new interface should be allowed to return null. But everybody in this PR is in favor of returning non-null values. As the original author I agree and it will be possible to rewrite the sso backend for this new api, by simply implementing the behavior in a subclass of the UserBackend. This should be easy, as there is only one central location where it is instantiated.
2. There was a discussion in wider NC if the password that we pass to createSessionToken can be null. This is indeed a discussion that afaik has not finished. But we resolved the issue here by not passing any more nulls to that function than the code already did. so our PR is decoupled from that discussion.

In summary, the people behind this PR agree on the api, the original author agrees, the api works for the intended usecase and this PR is not blocked by any external decision.

So I think we took that decision that you are talking about. If not, please let me know who can take this decision.

[MichaIng]

So we just need another approval.

The failing S3 checks are unrelated, fail randomly, I'll try to resolve with a rebase before merging.

[ChristophWurst]

Looks good

[MichaIng]

Drone failure is unrelated, but I cannot merge due to it. Please someone do, so work on nextcloud/user_saml#537 can finally go on 🙂.