User backend API overhaul

Heyo,

it looks like we should attempt an overhaul of our user back-end API. There is some really old code, some traces of new APIs and a lot of inconsistency and confusion.

Hence I would say that we should find consent on how user backends shall look like and then settle on an API so we can phase out any thing that should go away.

Some things that instantly pop to my mind

Can I already use OCP\User\Backend to implement a user backend sparked this idea.

Once we have a solid API we should also document everything and make the docs the single source of truth :wink:

cc @blizzz @jones4410 @nickvergessen @PancakeConnaisseur @rullzer

1 Like

also cc @icewind @juliushaertl @MorrisJobke as maintainers of other backend apps

Thank you for creating this. I agree with everything you wrote, although I can’t find ICustomLoginin the Nextcloud code.

More thoughts from me:

  • User backend capability detection (e.g. “can this user backend change passwords?”) system should not rely on compile time type checking, i.e. interfaces as is the case with OCP\User\Backend\I....Backend.

    This is because user backend’s capabilities vary from Nextcloud instance to instance and they all use the same user backend code. Therefore user backends must be able to communicate to Nextcloud what capabilities they have during runtime, depending on what admins have configured.

    See also: Can I already use OCP\User\Backend to implement a user backend

  • There should be more capabilities for user backends to provide. e.g.

    • getUserEMail / setUserEmail
    • getUserQuota / setUserQuota
1 Like

I guess that was an example. Right now it is an “implementsAction(INTEGER_THAT_REPRESENTS_THE_ACTION)”. This causes other problems like static code analysis failing as this is custom code that basically redos what interfaces are for. So every action gets it’s own interface and then you implement the ones that are available in your backend. Then also the server can properly check the implementations for those interfaces.

I like the idea of getting this into shape. Also the IBackend vs IUserInterface is a bit confusing and should be avoided with the new approach.

Ah, that makes sense when it was meant as an example.

As i have written, static code checks do not account for runtime differences in capabilities. I think, it will not be enough to merely rely on interfaces, unfortunately as clean as that would have been.

I thought so too but as @PancakeConnaisseur found out it’s totally legitimate to decide the capability at runtime. We do the same for LDAP (:eyes: @blizzz) and therefore LDAP can’t work with the structure of the interfaces that we currently have. The supported actions also depend on config and the LDAP server in use.

One possible solution or enhancement to this is that we either let the actions throw a specific exception when the action is not allowed or we extend the actions with another method isSupported or isAvailable and there the app can say nope for whatever reason. The latter is a breaking change, though and we have to make sure the methods don’t overlap if more than one interface is implemented by a class.

Here is my first draft proposal:

tl;dr Instead of bit based action determination use an array of class const strings. Let a user backend return a list of supported actions (can be changed during runtime). Use a user backend proxy to have a single place in the code where supported actions are checked (and exceptions thrown). Let code that deals with users only have access to the user backend proxy.

Code Draft

Action Definitions

class UserBackendAction {
	public const CREATE_USER = 'create user';
	public const DELETE_USER = 'delete user';
	public const GET_EMAIL_ADDRESS = 'get e-mail address';
	// TODO: add other actions
}

This is very similar to OCP\UserInterface\Backend but the capabilities are just const attributes not const attributes with number values and then an additional list with names.

New UserBackend Interface

namespace OCP;
interface IUserBackend {
	public function getBackendName(): string;
	public function getSupportedActions() : iterable;

	public function supportsAction($action) : bool;
	public function getEMailAddress($username) : string;
	//TODO: add other functions
}

Now contains getSupportedActions() and would also need all methods that are potentially available. We can write int he documentation that they can be left empty. It is a bit ugly and a design weak point. Maybe we can come up with something better.

Example Backend Implementation

class ExampleUserBackend implements \OCP\IUserBackend {

	public function getBackendName() {
		return "example Backend";
	}

	private $supportedActions = array(
		UserBackendAction::CREATE_USER,
		UserBackendAction::DELETE_USER,
		); // default values for illustration or could be empty


	public function __construct() {
		$enabled_actions = readConfig(); // readConfig is the app dev's implementation
		foreach ($enabled_actions as $action) {
			array_push($supportedActions, $action);
		}
	}

	public function getSupportedActions(): array {
		return $this->supportedActions;
	}
}

This should not be accessible from UserManager but only the Proxy (see below). This is to enforce the action checks and throwing of exceptions.

User Backend Proxy

class BackendProxy implements \OCP\IUserBackend {

	private $actualBackend;

	public function __construct(\OCP\IUserBackend $backend) {
		$this->actualBackend = $backend;
	}

	public function getBackendName() {
		return $this->actualBackend->getBackendName();
	}

	public function supportsAction($action): bool {
		return in_array($action,  $this->actualBackend->supportedActions());
	}

	public function getEmailAddress($username) {
		if ($this->supportsAction(UserBackendAction::GET_EMAIL_ADDRESS)) {
			return $this->actualBackend->getEmailAdress($username);
		}
		else {
			throw new UnsupportedUserBackendActionException("This user backend does not support GET_EMAIL_ADDRESS");
		}
	}
       
        //TODO: add methods for all other actions 

}

Usage in User.php

try {
	$this->backendProxy->setEMailAddress("user@example.com");
}
catch (UnsupportedUserBackendActionException $e) {
	// handle exception	
}

Considerations

  • this enables change of supported actions during runtime
  • having the proxy complicates things, but I think it is worth it because
    • you don’t have to check for supported actions each time in the code when you do user operations, e.g in User.php. Alternatively, the supportsAction() can be used to avoid try/catch if that turns out to be easier.
    • implementors of user backends only need to provide a list of supported actions. they don’t have to understand all this logic -> it is easier to write user backends.
  • There is the “problem” of keeping the list of supported actions in UserBackendAction and the list of methods in BackendProxy and the documentation in sync.

I am looking forward to your comments.

EDIT: fixed IUserBackend. Forgot to add methods at first.

Thanks a lot @PancakeConnaisseur, I see you put a lot of thought into this.

Let me shed some light on this and analyze it from my perspective.

I think you’re basically reinventing the old system. There are a few minor changes but it’s the same untyped system where you have an API to know what actions are supported by the backend but you have zero type safety. Type safety means knowing what methods are supposed to look like, which arguments they take and of what type those are as well as what to expect as a return value and which exceptions are ok to throw and therefore should be caught on the calling side.

If we take a step back there is two goals we want to achive

  • Flexibility both statically in how backends are developed as well as how they can express the support for a certain action as well as
  • Type soundness so that we know what the methods looks like, have proper versioning and deprecation policies and so on. The usual clean design stuff.

With that in mind I would still argue that the interfaces are required for the type soundness. But we have to reconsider the way we check a user back-end for whether it supports a certain action. I’m repeating myself a bit with what I commented before, but I think we only have to change the assumption that an implemented interface means that the backend will process the action. Implementing the action on the class of a backend should rather mean I can potentially handle this but it shouldn’t mean that MUST handle it, like we saw in your example. Hence we need the additional method (or a comparable mechanism) on each interface where the Nextcloud server can check if the implemented action is currently possible to execute. Only then the code should run.

The old code was

$canCreate = $be->implementsActions(\OC\User\Backend::CREATE_USER);
$be->createUser(..); // 0% type safety due to duck typing, we can only guess the API

then the new (but as we now know limited) approach was

$canCreate = $be instanceof ICreateUserBackend;
$be->createUser(...); // 100% type safe, but the backend now MUST support this

but what we actually need is

$canCreate = ($be instanceof ICreateUserBackend) && $be->canCreateUser();
$be->createUser(...); // 100% type safetey AND we can dynmically ask the BE if it wants to carry out the operation

Does that make sense?

The only downside is that it’s a breaking change for the existing interfaces. But we kind of have this anyway because the alternative of methods on each of the action interfaces is one method on the backend itself for the check and then you’ll have all checks in one method instead of separate paths for each of the often unrelated action logic parts. Or we just go with exceptions and accept a marginal performance disadvantage.

Yeah, I was away from the computer and thought about this more and then realized that we can still incooperate the single-method interfaces. They can at least define the upper set of actions that a user backend implements. It is better than forcing each user backend to have stubs for each actions even though they don’t implement them all.

Not sure if you mean return types. I just left them out in the draft, because it’s a given. Of course all methods and interfaces should have return types and type hints for the arguments.

I don’t think I understand this last paragraph.

  • Could you give examples for what you mean?
  • By “existing interfaces”, you mean the “new” OCP\User\Backend\I....Backend?
  • AFAIK "Ask forgiveness not permission”, i.e. using exceptions is acutally faster, because most of the time you don’t have anything to catch, so I would prefer try/catch to if ($backend->canCreateUser() but maybe this is different for web applications and PHP.
    • try/catch also create more lines of code, because they introduce line breaks, so thats another minor disadvantage

I will add another post to reflect the single method-interfaces.

Second Draft Proposal

tl;dr Instead of bit based action determination use an array of class const strings. Let a user backend return a list of supported actions (can be changed during runtime). Use a user backend proxy to have a single place in the code where supported actions are checked (and exceptions thrown). Let code that deals with users only have access to the user backend proxy.
changes from Draft 1: concrete UserBackends only implements a subset of Interfaces. There is an single-method interface for each action. UserProxy implements all of them.

Code

Action Definitions (same as draft 1)

class UserBackendAction {
	public const CREATE_USER = 'create user';
	public const DELETE_USER = 'delete user';
	public const GET_EMAIL_ADDRESS = 'get e-mail address';
	// TODO: add other actions
}

This is very similar to OCP\UserInterface\Backend but the capabilities are just const attributes not const attributes with number values and then an additional list with names.

New UserBackend Interface (no action signatures at all)

namespace OCP;
interface IUserBackend {
	public function getBackendName(): string;
	public function getSupportedActions() : iterable;
}

Contains getSupportedActions() but no actual action interfaces. They are implemented indivudually.

Single-method example Interface IUserBackendGetEMailAddress

interface IUserBackendGetEMailAddress {
	public function getEMailAddress(string $username) : string;
}

Single-method example Interface IUserBackendCreateUser

interface IUserBackendCreateUser {
	public function createUser(string $username, string $password): void;
	// or return string if you want to return the uid to signal that it was successful
}

Single-method Interfaces (the rest)

...

Example Backend Implementation (only the action interfaces it supports at most)

class ExampleUserBackend implements IUserBackend, IUserBackendGetEMailAddress, IUserBackendCreateUser {

	public function getBackendName(): string {
		return "example Backend";
	}

	private $supportedActions = array(
		UserBackendAction::GET_EMAIL_ADDRESS,
		UserBackendAction::CREATE_USER,
		); // default values for illustration or could be empty
		   // and only set in constructor


	public function __construct() {
		$enabled_actions = readConfig(); // readConfig is the app dev's implementation
		foreach ($enabled_actions as $action) {
			array_push($supportedActions, $action);
		}
	}

	public function getSupportedActions(): array {
		return $this->supportedActions;
	}

	public function getEmailAdress(string $username): string {
		// actually retrieve the email address
	}
	
   	public function createUser(string $username, string $password): void {
		// actually create user
	}
}

This should not be accessible from UserManager but only the Proxy (see below). This is to enforce the action checks and throwing of exceptions.

User Backend Proxy

class BackendProxy implements \OCP\IUserBackend, /**and ALL possbile Interfaces**/ {

	private $actualBackend;

	public function __construct(\OCP\IUserBackend $backend) {
		$this->actualBackend = $backend;
	}

	public function getBackendName() {
		return $this->actualBackend->getBackendName();
	}

	public function supportsAction($action): bool {
		return in_array($action,  $this->actualBackend->supportedActions());
	}

	//for each user backend actions that nextcloud supports
	// as in `UserBackendAction`, here for UserBackendAction::GET_EMAIL_ADDRESS
	public function getEmailAddress($username) {
		if ($this->supportsAction(UserBackendAction::GET_EMAIL_ADDRESS)) {
			return $this->actualBackend->getEmailAdress($username);
		}
		else {
			throw new UnsupportedUserBackendActionException("This user backend does not support GET_EMAIL_ADDRESS");
		}
	}
       
        //TODO: add methods for all other actions 

}

The “ugly” part is now here, where UserBackendProxy must implement all action interfaces, because this class has to handle all of them potentially.

Usage in User.php

try {
	$this->backendProxy->setEMailAddress("user@example.com");
}
catch (UnsupportedUserBackendActionException $e) {
	// handle exception	
}

Considerations/Summary

  • concrete user backend implmementations can set an upper limit to the action they support by only implementing a subset of action interfaces
  • the Proxy must implement all action interfaces, because it must be able to handle all action method calls
  • I understand that it’s not the most straight forward solution, but I still think the ability to have runtime handling of available actions is worth it and “there is no free lunch”. Having compile time type safety and contracts with interfaces is great, but it just doesn’t fit a web application well where everything is decided on the go.
  • I also looked into eval() which could create custom classes during runtime but it is supposed to be slow and maybe introduce some security issues (although by app developers) which have access to everything anyway.

I am “developing” this in my head. Maybe I should do a POC to be able to see possible improvements or show problems that I did not think about.

Yes, ICreateUserBackend and its friends.

Fair point. In that case the API changes are also the minimal.

But also in your updated proposal you have a mechanism to ask if an action is supported via supportsAction. I would certainly either have no method to check if something is supported and just call the method when the interface is implemented and catch the exception or ask if it’s supported and then assume it will be carried out.

The downside of exceptions instead of methods that ask if the operation can be run is that you can’t determine the ability to run without executing it. Right now this is possibly a theoretical concern but it lowers the flexibility of the API.

The part about the proxy I don’t get at all. But I assume this has nothing to do with the API anyway but just how it’s handled internally. Let’s ignore this part for the API discussion.


So I would again say it’s best to have interfaces like

interface ICreateUserBackend extends IUserBackend {

 	public function canCreateUser(): bool;

	public function createUser(string $uid, string $password): bool;
}

… because this gives us most flexibility.

  • A user back-end can decide which actions it generally implements.
  • We have the small interfaces to ensure type safety and versioning.
  • At runtime you can still say you can’t run a certain action right now by returning false from canXXX.
  • The logic about which actions can be run is not mixed in one implementsAction method or similar but separated into the canXXX predicates.
  • We can determine if an action will be carried out without having to invoke the action.
  • We don’t need exceptions and the overhead of asking canXXX is minimal because in most cases the implementation will return a static true.

What do you think? :slight_smile:

@ChristophWurst, I probably missed some clear technical element…
Yet couldn’t be worthwhile to mention other app developers, like @Rello or @marcelklehr?

Probably they will not be interested (Sorry to disturb!!!), but maybe they could provide some elements to the discussion.

Again, sorry if I just disturbed everybody…

Hi,
I don´t know how it would affect any of my apps. I am not working with user creation or similar.
…Or I did not get the discussion here :thinking:

This is mainly interesting for devs and maintainers of apps that provide a user back-end :slight_smile:

I’ve been following this quietly all along. Even though I don’t have any code in this area of the API surface it looks like a good proposal.

On throw vs canXXX I’m unsure. Theoretically, there’s cases where canXXX would have to try the operation to know the answer, but there’s also cases where you just want to know whether it’s possible. I guess, in general, having canXXX is good, as backends that do have to attempt the operation can just return true and then throw later on.

– my two cents. In any case, I think it’s awesome to discuss this kind of changes with the community :slight_smile: :heart:

Absolutely. For those cases I’d allow throwing an exception.

I started a PR with the changes. Not much so I don’t run into a path that we later say is the wrong one but maybe enough to give a better vision of how I picture it.

The proxy is not part of the API but I think it makes sense to think about this part as well, because this might have consequences for the API. The intent behind it, is to make development of user backends easier, because user backend developers only need to supply the capabilities of their backend and not throw all exceptions. But after thinking about this, we can document in the interfaces that exceptions must be thrown when this action is not supported during runtime and having one less component (proxy) is worthwhile as well. So, I retract the proxy idea for now.

I don’t think the action interfaces should inherit from IUserBackend. Is this a mistake?

I suggest to call it canCurrentlyCreateUser() to make it clear that this is a runtime decision.

Yes, I agree the user backend dev can still modify this response during runtime. I did not take this into account.

I don’t think the action interfaces should inherit from IUserBackend. Is this a mistake?

it is a is a relationship. Every ICreateUserBackend is also a IUserBackend: https://en.wikipedia.org/wiki/Is-a

I suggest to call it canCurrentlyCreateUser() to make it clear that this is a runtime decision.

IMO that is a bit too much. It’s obvious that the call has to ask this every time, like per each request. And the the result must not be cached.