owncloud / owncloud/ocis

Support for AD and other ldap providers for the graph microservice

Open
#4,467 6 comments 0 reactions 0 assignees View on GitHub
Category:Enhancement
Dominant language
Go
Stars
2.1k
Forks
274
Avg merge
2d 1h
Merged PRs (30d)
103

Description

## Is your feature request related to a problem? Please describe.
The graph microservice uses ldap to connect to an internal libregraph-idm service. It can be useful if graph could connect to other ldap providers such as AD if needed.

## Describe the solution you'd like
Multiple implementations can be provided taking advantage of provider's details in order to improve performance when accessing the ldap server

## Describe alternatives you've considered

## Additional context
We can create a package in the ocis-pkg folder that can be reused by multiple microservices if needed. For now, the main "client" will be the graph microservice.

The package will be divided as follows:
* ocis-pkg/ldap/connectors/ldapconnector.go -> interface, common errors, factory method
* ocis-pkg/ldap/connectors/libregraph.go -> implementation for libregraph-idm (mostly moved from the current code in the graph microservice)
* ocis-pkg/ldap/connectors/ad.go -> implementation for AD

Additional implementations can be added later if needed. Note that the libregraph implementation might be the less restrictive and might be used as default. As said, other implementations are expected to provide improved performance.

In order to provide a common access to all available providers, the following interface will be used:
```
type LdapConnector interface {
IsReadOnly() bool

CreateUser(ctx context.Context, user *libregraph.User) (*libregraph.User, error)
DeleteUser(ctx context.Context, DN string) error
UpdateUser(ctx context.Context, DN string, user *libregraph.User) (*libregraph.User, error)
CreateGroup(ctx context.Context, group *libregraph.Group) (*libregraph.Group, error)
DeleteGroup(ctx context.Context, DN string) error
AddMembersToGroup(ctx context.Context, groupDN string, memberDNs []string) error
RemoveMemberFromGroup(ctx context.Context, groupDN string, memberDN string) error

GetUsers(ctx context.Context, filter string) ([]*libregraph.User, error)
GetGroups(ctx context.Context, filter string) ([]*libregraph.Group, error)
GetGroupMembers(ctx context.Context, DN string) ([]*libregraph.User, error)

GetUserAttrMap() UserAttrMap
GetGroupAttrMap() GroupAttrMap
}
```
Note the following restrictions:
* Not all the implementations are expected to give write access. For implementations giving read-only access, these are the additional restrictions:
* The `IsReadOnly` MUST return true.
* Any write method MUST return a "not implemented" error
* Most of the operations require a DN. This should work fine for a lot of cases, and guarantees that we operate over the ldap register we want
* Searching by name or id might return multiple results. In that case, it's difficult for the connector to know what to do. The easiest option would be to operate over all the entries, but that's unlikely what the user wants, and it might cause issues.
* The expected workflow for clients such as the graph microservice is to search the user by name or id and then use it. As far as I know, graph is already following this approach, so it shouldn't be a big deal. In case multiple results are found in the search, it's up to the client (graph in this case) to decide what to do (likely return an error).
* Caching is expected to be done by graph. Graph can get the information and cache it based on the name or id, so the next time it doesn't need to request the same info to the ldap server.
* We might want to include a way for the implementation to update the cache based on the notifications coming from the ldap server. This isn't planned at the moment because we need to check how many providers support this and if it's really worthy.
* User and group mappings will be managed by each implementation.
* Each implementation will provide default mappings for both user and groups.
* The implementations will allow modifications of those mappings when the instance is created, but not later
* Validation of the user-entered mappings can be done by the implementations when they're created. If there is any kind of error with the mappings (attribute not known, or the mapping isn't allowed for a specific attribute), the instance shouldn't be created.
* The interface doesn't provide a method to get the user or group by DN.
* As said, the expected workflow is that the graph (and any other client) searches the user info by name or by id (same for groups). Having the info, graph can cache that information by whatever attribute it wants.
* Graph and any other microservices are unlikely to ask for DNs.
* Search filters will be provided as strings.
* The implementations will assume they're valid filters, enclosed in "(" and ")". They shouldn't be changed.
* Graph is expected to escape the values if needed

Taking that into account, it's expected that the AD implementation will hide a couple of things from the graph service:
* Groups with more than 1500 members (by default) will require to use range requests in order to get all the members. This will need additional handling that other implementations might not need to deal with.
* Nested groups will need special handling. Knowing that the target is AD, we can use specific features that AD has.

Both of those points will be difficult to deal with if we need to take into account additional restrictions that other providers than AD could impose.

-----

While the goal could be to add support for AD, the design should allow more providers to be added as needed. It's expected that the current connection used for the libregraph-idm is generic enough to be used with all the providers, but at expense of having a poor performance on some operations.

From a configuration point of view, graph (and any microservice that needs it) can use a default generic connector if none is provided. Mappings for each connector should be documented, so if they don't match with the provider, the admin should adjust the mapping.
Configuring the right connector for the provider should be easier (less mapping should be adjusted if the default ones are good enough) and also provide a performance boost.

Note that it's unlikely that we can ensure, for example, that the AD connector is only used for AD. If the admin configures a wrong connector things can go really wrong.

-----

To be checked:
* Support for notifications coming from the ldap server (which providers support this feature, possible real usage in ocis, integration complexity, etc)
* Safety net to prevent configuring a wrong connector

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.