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.

Sign up for GitHub

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

add email & group query support #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add email & group query support #26

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4a66e4
add email query support (closes #20)
kosli Apr 8, 2020
7c523a3
Merge remote-tracking branch 'upstream/master'
kosli Feb 24, 2021
ce4d316
Merge branch 'PanCakeConnaisseur:master' into master
kosli Oct 4, 2021
597ff6c
Increase compatible version to Nextcloud 22
kosli Oct 4, 2021
618c27f
Merge branch 'master' of https://github.com/PanCakeConnaisseur/user_b…
kosli Jul 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/Config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class Config {
const CONFIG_KEY_COUNT_USERS = 'count_users';
const CONFIG_KEY_GET_HOME = 'get_home';
const CONFIG_KEY_CREATE_USER = 'create_user';
const CONFIG_KEY_GET_EMAIL_ADDRESS = 'get_email_address';
const CONFIG_KEY_GET_GROUPS = 'get_groups';

private $logger;
private $appConfiguration;
Expand Down Expand Up @@ -218,6 +220,14 @@ public function getQueryCreateUser() {
return $this->getQueryStringOrFalse(self::CONFIG_KEY_CREATE_USER);
}

public function getQueryGetEmailAddress() {
return $this->getQueryStringOrFalse(self::CONFIG_KEY_GET_EMAIL_ADDRESS);
}

public function getQueryGetGroups() {
return $this->getQueryStringOrFalse(self::CONFIG_KEY_GET_GROUPS);
}

/**
* Tries to read a config value and throws an exception if it is not set.
* This is used for config keys that are mandatory.
Expand Down
60 changes: 59 additions & 1 deletion lib/UserBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,26 @@

use OCP\ILogger;
use OC\User\Backend;
use OCP\IUserManager;
use OCP\IGroupManager;

class UserBackend implements \OCP\IUserBackend, \OCP\UserInterface {

private $logger;
private $logContext = ['app' => 'user_backend_sql_raw'];
private $config;
private $db;
private $userManager;
private $groupManager;

public function __construct(ILogger $logger, Config $config, Db $db) {
public function __construct(ILogger $logger, Config $config, Db $db, IUserManager $userManager, IGroupManager $groupManager) {
$this->logger = $logger;
$this->config = $config;
// Don't get db handle (dbo object) here yet, so that it is only created
// when db queries are actually run.
$this->db = $db;
$this->userManager = $userManager;
$this->groupManager = $groupManager;
}

public function getBackendName() {
Expand Down Expand Up @@ -135,6 +141,8 @@ public function getDisplayName($providedUsername) {
$statement = $this->db->getDbHandle()->prepare($this->config->getQueryGetDisplayName());
$statement->execute(['username' => $providedUsername]);
$retrievedDisplayName = $statement->fetchColumn();

$this->updateAttributes($providedUsername);
Copy link
Owner

Choose a reason for hiding this comment

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

Any particular reason why attributes are updated in getDisplayName()?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @PanCakeConnaisseur for picking this up.
It is already a while ago, so I am not 100% sure why I have put it at that specific place, but I assume I didn't find a better place to trigger the update of the attributes. Where would you suggest to put it?

Copy link
Owner

Choose a reason for hiding this comment

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

It makes more sense to update the attributes whenever it is requested. The way you implemented it, each time a display name is read (which might be often), you update the attributes. This is firstly probably very inefficient and secondly might lead to unexpected behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

I think the problem was that by only relying on the "whenever it is requested", it got requested too seldom and the only way to make it update it more often was to call it from the getDisplayName(). Whereas the updateAttributes() function only updates the attributes if there is really a change, so to prevent the database being updated all the time.

Copy link
Owner

@PanCakeConnaisseur PanCakeConnaisseur Oct 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I went through the code (Nextcloud's) that is calling the user backend and the problem is that setting an email by a user backend is not supported in Nextcloud. There is also no clean way to inject the e-mail at a different point, AFAICS. I thought about adding something to createUser() to set the e-mail initially but at that stage the user object has not been created yet. I would need to do it later, without getting called explicitly, i.e. store state. This is means there is no clean solution.

You circumvented this limitation by using getDisplayName() to just overwrite the e-mail field if necessary. I don't agree with that solution because it mixes up reading and writing data and is unexpected behavior. I think a trigger that listens for the "created user" event would be a better solution to set the e-mail initially and then allow the user to change the e-mail. Always overwriting the e-mail like you suggested, feels like a hack to me. I understand that it does the job, but this would be better located in an an extra app. IMHO, the user backend should not have any additional logic that corrects or circumvents the behavior of Nextcloud.

I am currently trying to get in contact with core Nexcloud devs to suggest an overhaul of the user interfaces that would allow a clean solution. Until then, I don't think I will implement an e-mail from/to SQL solution.

return $retrievedDisplayName;
}

Expand Down Expand Up @@ -259,6 +267,56 @@ public function createUser($providedUsername, $providedPassword) {
}
}

public function updateAttributes($providedUsername) {
$retrievedEmailAddress = null;
if(!empty($this->config->getQueryGetEmailAddress())) {
$statement = $this->db->getDbHandle()->prepare($this->config->getQueryGetEmailAddress());
$statement->execute(['username' => $providedUsername]);
$newEmailAddress = $statement->fetchColumn();
}
$user = $this->userManager->get($providedUsername);

$newGroups = null;
if(!empty($this->config->getQueryGetGroups())) {
$statement = $this->db->getDbHandle()->prepare($this->config->getQueryGetGroups());
$statement->execute(['username' => $providedUsername]);
$newGroups = $statement->fetchAll(\PDO::FETCH_COLUMN, 0);

// Make sure that the user is always in the "everyone" group
if(!in_array('everyone', $newGroups)) {
$newGroups[] = 'everyone';
}
}

if ($user !== null) {
$currentEmailAddress = (string)$user->getEMailAddress();
if ($newEmailAddress !== null
&& $currentEmailAddress !== $newEmailAddress) {
$user->setEMailAddress($newEmailAddress);
}

if ($newGroups !== null) {
$groupManager = $this->groupManager;
$oldGroups = $groupManager->getUserGroupIds($user);

$groupsToAdd = array_unique(array_diff($newGroups, $oldGroups));
$groupsToRemove = array_diff($oldGroups, $newGroups);

foreach ($groupsToAdd as $group) {
if (!($groupManager->groupExists($group))) {
$groupManager->createGroup($group);
}
$groupManager->get($group)->addUser($user);
}

foreach ($groupsToRemove as $group) {
$groupManager->get($group)->removeUser($user);
}
}
}
}


/**
* Escape % and _ with \.
*
Expand Down