Skip to content

Add a new group template just as it is already there for adding users #219

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
73 changes: 56 additions & 17 deletions lib/LDAPGroupManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,25 @@
use Exception;
use OC\Group\Backend;
use OCA\LdapWriteSupport\AppInfo\Application;
use OCA\LdapWriteSupport\Service\Configuration;
use OCA\User_LDAP\Group_Proxy;
use OCA\User_LDAP\ILDAPGroupPlugin;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
use OCP\LDAP\ILDAPProvider;
use Psr\Log\LoggerInterface;

class LDAPGroupManager implements ILDAPGroupPlugin {
/** @var Configuration */
protected $configuration;

/** @var ILDAPProvider */
private $ldapProvider;

/** @var IUserSession */
private $userSession;

/** @var IGroupManager */
private $groupManager;

Expand All @@ -47,11 +55,13 @@ class LDAPGroupManager implements ILDAPGroupPlugin {
/** @var LoggerInterface */
private $logger;

public function __construct(IGroupManager $groupManager, LDAPConnect $ldapConnect, LoggerInterface $logger, ILDAPProvider $LDAPProvider) {
public function __construct(IGroupManager $groupManager, IUserSession $userSession, LDAPConnect $ldapConnect, LoggerInterface $logger, ILDAPProvider $ldapProvider, Configuration $configuration) {
$this->groupManager = $groupManager;
$this->userSession = $userSession;
$this->ldapConnect = $ldapConnect;
$this->ldapProvider = $ldapProvider;
$this->configuration = $configuration;
$this->logger = $logger;
$this->ldapProvider = $LDAPProvider;

if($this->ldapConnect->groupsEnabled()) {
$this->makeLdapBackendFirst();
Expand Down Expand Up @@ -82,15 +92,27 @@ public function respondToActions() {
* @return string|null
*/
public function createGroup($gid) {
$adminUser = $this->userSession->getUser();
$requireActorFromLDAP = $this->configuration->isLdapActorRequired();
if ($requireActorFromLDAP && !$adminUser instanceof IUser) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ($requireActorFromLDAP && !$adminUser instanceof IUser) {
if ($requireActorFromLDAP && !($adminUser instanceof IUser)) {

I was unsure of the precedence between operators here so I expect others might

throw new Exception('Acting user is not from LDAP');
}
try {
$connection = $this->ldapProvider->getLDAPConnection($adminUser->getUID());
// TODO: what about multiple bases?
$base = $this->ldapProvider->getLDAPBaseGroups($adminUser->getUID());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$base = $this->ldapProvider->getLDAPBaseGroups($adminUser->getUID());
$base = $this->ldapProvider->getLDAPBaseGroups($adminUser->getUID())[0];

To match line 112, no?

} catch (Exception $e) {
if ($requireActorFromLDAP) {
if ($this->configuration->isPreventFallback()) {
throw new \Exception('Acting admin is not from LDAP', 0, $e);
}
return false;
}
$connection = $this->ldapConnect->getLDAPConnection();
$base = $this->ldapConnect->getLDAPBaseGroups()[0];
}

/**
* FIXME could not create group using LDAPProvider, because its methods rely
* on passing an already inserted [ug]id, which we do not have at this point.
*/

$newGroupEntry = $this->buildNewEntry($gid);
$connection = $this->ldapConnect->getLDAPConnection();
$newGroupDN = "cn=$gid," . $this->ldapConnect->getLDAPBaseGroups()[0];
list($newGroupDN, $newGroupEntry) = $this->buildNewEntry($gid, $base);
$newGroupDN = $this->ldapProvider->sanitizeDN([$newGroupDN])[0];

if ($ret = ldap_add($connection, $newGroupDN, $newGroupEntry)) {
Expand Down Expand Up @@ -151,7 +173,6 @@ public function addToGroup($uid, $gid) {
break;
case 'gidNumber':
throw new Exception('Cannot add to group when gidNumber is used as relation');
break;
}

if (!$ret = ldap_mod_add($connection, $groupDN, $entry)) {
Expand Down Expand Up @@ -220,12 +241,30 @@ public function isLDAPGroup($gid): bool {
}
}

private function buildNewEntry($gid): array {
return [
'objectClass' => ['groupOfNames', 'top'],
'cn' => $gid,
'member' => ['']
];
private function buildNewEntry($gid, $base): array {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please type parameters

$ldif = $this->configuration->getGroupTemplate();

$ldif = str_replace('{GID}', $gid, $ldif);
$ldif = str_replace('{BASE}', $base, $ldif);

$entry = [];
$lines = explode(PHP_EOL, $ldif);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don’t we have parsing LDIF code elsewhere?

foreach ($lines as $line) {
$split = explode(':', $line, 2);
$key = trim($split[0]);
$value = trim($split[1]);
if (!isset($entry[$key])) {
$entry[$key] = $value;
} else if (is_array($entry[$key])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else if (is_array($entry[$key])) {
} elseif (is_array($entry[$key])) {

$entry[$key][] = $value;
} else {
$entry[$key] = [$entry[$key], $value];
}
}
$dn = $entry['dn'];
unset($entry['dn']);

return [$dn, $entry];
}

public function makeLdapBackendFirst(): void {
Expand Down
6 changes: 6 additions & 0 deletions lib/LDAPUserManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public function respondToActions() {
* @throws ServerNotAvailableException
*/
public function setDisplayName($uid, $displayName) {
trigger_error(__METHOD__);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
trigger_error(__METHOD__);

Forgotten debug?

$this->logger->error(__METHOD__, ['app' => 'ldap_write_support']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->logger->error(__METHOD__, ['app' => 'ldap_write_support']);

Same?

$userDN = $this->getUserDN($uid);

$connection = $this->ldapProvider->getLDAPConnection($uid);
Expand All @@ -130,6 +132,10 @@ public function setDisplayName($uid, $displayName) {
if (ldap_mod_replace($connection, $userDN, [$displayNameField => $displayName])) {
return $displayName;
}
trigger_error(print_r(['conn' => $connection,
'user' => $userDN,
'dpyField' => $displayNameField,
'dpy' => $displayName], true));
Comment on lines +135 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
trigger_error(print_r(['conn' => $connection,
'user' => $userDN,
'dpyField' => $displayNameField,
'dpy' => $displayName], true));

🥉

throw new HintException('Failed to set display name');
} catch (ConstraintViolationException $e) {
throw new HintException(
Expand Down
16 changes: 16 additions & 0 deletions lib/Service/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ public function getUserTemplate() {
);
}

public function getGroupTemplate() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getGroupTemplate() {
public function getGroupTemplate(): string {

return $this->config->getAppValue(
Application::APP_ID,
'template.group',
$this->getGroupTemplateDefault()
);
}

public function getUserTemplateDefault() {
return
'dn: uid={UID},{BASE}' . PHP_EOL .
Expand All @@ -66,6 +74,14 @@ public function getUserTemplateDefault() {
'sn: {UID}';
}

public function getGroupTemplateDefault() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getGroupTemplateDefault() {
public function getGroupTemplateDefault(): string {

return
'dn: cn={GID},{BASE}' . PHP_EOL .
'objectClass: groupOfNames' . PHP_EOL .
'cn: {GID}' . PHP_EOL .
'member:';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the empty member line?

}

public function isRequireEmail(): bool {
// this core settings flag is not exposed anywhere else
return $this->config->getAppValue('core', 'newUser.requireEmail', 'no') === 'yes';
Expand Down
4 changes: 3 additions & 1 deletion lib/Settings/Admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ public function getForm() {
'templates',
[
'user' => $this->config->getUserTemplate(),
'userDefault' => $this->config->getUserTemplateDefault(),
'userDefault' => $this->config->getGroupTemplateDefault(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'userDefault' => $this->config->getGroupTemplateDefault(),
'userDefault' => $this->config->getUserTemplateDefault(),

'group' => $this->config->getGroupTemplate(),
'groupDefault' => $this->config->getGroupTemplateDefault(),
]
);
$this->initialStateService->provideInitialState(
Expand Down
21 changes: 21 additions & 0 deletions src/components/AdminSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@
<li><span class="mono">{BASE}</span> – {{ t('ldap_write_support', 'the LDAP node of the acting (sub)admin or the configured user base') }}</li>
</ul>
<textarea v-model="userTemplate" class="mono" @change="setUserTemplate" />
<h3>{{ t('ldap_write_support', 'Group template') }}</h3>
<p>{{ t('ldap_write_support', 'LDIF template for creating groups. Following placeholders may be used') }}</p>
<ul class="disc">
<li><span class="mono">{GID}</span> – {{ t('ldap_write_support', 'the group id provided by the (sub)admin') }}</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

int or string? Should be made clear.

<li><span class="mono">{BASE}</span> – {{ t('ldap_write_support', 'the LDAP node of the acting (sub)admin or the configured group base') }}</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The base of the acting subadmin, right?

</ul>
<textarea class="mono" v-on:change="setGroupTemplate" v-model="templates.group">{{templates.group}}</textarea>
</div>
</template>

Expand All @@ -71,6 +78,8 @@ export default {
templates: {
required: true,
type: Object,
group: Object,
groupDefault: Object,
Comment on lines +81 to +82
Copy link
Collaborator

Choose a reason for hiding this comment

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

identation seems off

},
switches: {
required: true,
Expand All @@ -95,6 +104,18 @@ export default {
return
}
OCP.AppConfig.setValue('ldap_write_support', 'template.user', this.userTemplate)
},
setGroupTemplate() {
if(this.templates.group === "") {
let self = this;
OCP.AppConfig.deleteKey('ldap_write_support', 'template.group', {
success: function() {
self.templates.group = self.templates.groupDefault;
}
});
return;
}
OCP.AppConfig.setValue('ldap_write_support', 'template.group', this.templates.group);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be this.groupTemplate to be consistent, or change the user template to this.templates.user.

},

toggleSwitch(prefKey, state, appId = 'ldap_write_support') {
Expand Down