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.

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 a new group template just as it is already there for adding users #219

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Oct 19, 2020

continuation and rebase of #214

@blizzz
Copy link
Member Author

blizzz commented Nov 4, 2020

@rotdrop may I ask you to sign-off your commits?

@rotdrop
Copy link

rotdrop commented Nov 4, 2020

@rotdrop may I ask you to sign-off your commits?

Oops. Yes. Does it still work as this is not my original pull request? I have a look ...

@blizzz
Copy link
Member Author

blizzz commented Nov 10, 2020

will work with the membership. Then you can also create branches in repos of the nextcloud organization :)

@rotdrop
Copy link

rotdrop commented Nov 10, 2020 via email

@blizzz
Copy link
Member Author

blizzz commented Jan 29, 2021

@rotdrop may I ask you to sign-off your commits?

@rotdrop would you mind?

@rotdrop rotdrop force-pushed the enh/noid/new-group-template branch from 406046b to 290411d Compare February 16, 2021 23:32
@rotdrop
Copy link

rotdrop commented Feb 16, 2021 via email

@rotdrop rotdrop force-pushed the enh/noid/new-group-template branch from 290411d to d324a20 Compare February 24, 2021 08:58
@susnux susnux added enhancement New feature or request 3. to review labels Sep 23, 2022
@susnux susnux linked an issue Sep 23, 2022 that may be closed by this pull request
@susnux susnux force-pushed the enh/noid/new-group-template branch from d324a20 to d168db1 Compare September 23, 2022 08:16
rotdrop and others added 2 commits October 1, 2022 17:40
Rationale: this allows to support "custom" group LDAP classes, like the
groupOfMembers class of the rfc2307bis draft. The core user_ldap app
could also be changed to make this more handy, but already allows custom
group filters.

Signed-off-by: Claus-Justus Heine <himself@claus-justus-heine.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the enh/noid/new-group-template branch from d168db1 to d5a5555 Compare October 1, 2022 16:51
@blizzz blizzz requested a review from come-nc October 1, 2022 18:07
@@ -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

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?

'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

$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])) {

@@ -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?

@@ -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(),

<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.

<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>
<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?

Comment on lines +81 to +82
group: Object,
groupDefault: Object,
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

});
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add group template to settings
4 participants