You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by karuturi <gi...@git.apache.org> on 2015/08/27 14:29:44 UTC
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
GitHub user karuturi opened a pull request:
https://github.com/apache/cloudstack/pull/755
Cloudstack:8647 LDAP Trust AD and Autoimport
Today, CloudStack can automatically import LDAP users based on the configuration to a domain or an account. However, any new users in LDAP aren't automatically reflected. The admin has to manually import them again.
This feature enables admin to map LDAP group/OU to a CloudStack domain and any changes are reflected in ACS as well.
FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/WIP%3A+LDAP%3A+Trust+AD+and+Auto+Import
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/karuturi/cloudstack CLOUDSTACK-8647-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/755.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #755
----
commit 0680648036044e5d9166a0d973b2c9f714a41778
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-04T11:14:58Z
CLOUDSTACK-8647: added cmd and response class for the new api
commit e3ddde841ed4b4ee588927394a2121fefd2d684c
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-06T11:42:24Z
CLOUDSTACK-8647 added new api linkLdapToDomain
also added the required dao, table and vo
commit 7109689fde9895d1e702544e4011de9eecc2c1a6
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-10T12:01:34Z
CLOUDSTACK-8647 changed the authentication flow
added check to see if domain is linked to ldap. If yes and the user is
member of the group/OU, authenticate and import user.
commit 0dc9ccd189682f82abd9ce1ab816213094b037db
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-11T06:37:44Z
CLOUDSTACK-8647 added account_type to the linkDomainToLdap API
commit 59291864fc893935294fc9a8ac60c6c537a7caff
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-11T08:57:55Z
CLOUDSTACK-8647 added nested group enabled config in ldap
querying the nested groups only when nested groups are enabled
commit 2825c07b38795ff541d4e9dc648612ce84fd728f
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-11T09:50:22Z
CLOUDSTACK-8647 support for assigning and admin to linked ldap domain
if an admin username is given to the linkDomainToLdap, added support to
import this user
User will be imported only if the user is available in the group/ou in
ldap and an account with the name doesnt exist in cloudstack.
on successful import, accountid will be returned in response.
commit 6cb5d3a3bc4c02071fb1b1005f9fd40b7468d6e3
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-08-12T05:12:11Z
CLOUDSTACK-8647: fixed unittests
commit dd6d6d18a588e7884385c74b98c635b487ea4e93
Author: Rajani Karuturi <ra...@citrix.com>
Date: 2015-08-24T09:00:37Z
CLOUDSTACK-8647 unittests for LinkDomainToLdap api command
commit 6572abc7b3095d07d003498e06c85bef1fe9dfd9
Author: Rajani Karuturi <ra...@citrix.com>
Date: 2015-08-24T11:21:36Z
CLOUDSTACK-8647 added unittests for new methods in ldapmanager
commit 36340d97bdecbb437dcbfd5ef30b49020ef681a5
Author: Rajani Karuturi <ra...@citrix.com>
Date: 2015-08-27T09:11:37Z
CLOUDSTACK-8647: UI for trust AD feature
commit c2b36cb7059e91f6f5e9292cda4bb283017708cc
Author: Rajani Karuturi <ra...@citrix.com>
Date: 2015-08-27T10:48:47Z
CLOUDSTACK-8647: formatted LdapAuthenticatorSpec
commit 1c836a8999a28ef8d6161600a3a29586b30cb532
Author: Rajani Karuturi <ra...@citrix.com>
Date: 2015-08-27T11:54:40Z
CLOUDSTACK-8647: unittests for LdapAuthenticatorSpec
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38383999
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
it is ok for the first call to succeed and the second to fail. for example if the username already exists, it will fail. I will check if tx is required.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-137344109
@koushik-das I created two issue for the open items. I will track these later
https://issues.apache.org/jira/browse/CLOUDSTACK-8797
https://issues.apache.org/jira/browse/CLOUDSTACK-8796
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384226
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
+ //Account type constants in com.cloud.user.Account
+ Validate.isTrue(accountType>=0 && accountType<=5, "accountype should be a number from 0-5");
--- End diff --
mm.. yes. at this point I only checked for allowed account types. will make the change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38399524
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2177,6 +2173,7 @@ private UserAccount getUserAccount(String username, String password, Long domain
if (domain != null) {
domainName = domain.getName();
}
+ userAccount = _userAccountDao.getUserAccount(username, domainId);
--- End diff --
for an authenticated user, useraccount can never be null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38399931
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2196,6 +2193,11 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Unable to authenticate user with username " + username + " in domain " + domainId);
}
+ if (userAccount == null) {
+ s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
--- End diff --
This may also happen if ldap user exists but authn. failed due to incorrect password. The message wouldn't be correct. Is it intentional?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38512312
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
authentication wont be successful. authentication will fail as createuseraccount will throw exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384079
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
+ Account account = _accountService.getActiveAccountByName(admin, domainId);
+ if (account == null) {
+ UserAccount userAccount =
+ _accountService.createUserAccount(admin, "", ldapUser.getFirstname(), ldapUser.getLastname(), ldapUser.getEmail(), null, admin, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, domainId, admin, null, UUID.randomUUID().toString(),
+ UUID.randomUUID().toString(), User.Source.LDAP);
--- End diff --
password is not required in db for ldap as authentication happens at ldap.
if it fails, domain admin account will not be created. its documented in fs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38304057
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
Is it possible to map different CS domains to same LDAP group/ou name?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38512420
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
I will see if I can change that later. For now, it will as documented in FS which is, it returns created account id when the account can be created. Otherwise, the information on why it failed to create the account will be in logs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38732126
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2177,6 +2173,7 @@ private UserAccount getUserAccount(String username, String password, Long domain
if (domain != null) {
domainName = domain.getName();
}
+ userAccount = _userAccountDao.getUserAccount(username, domainId);
--- End diff --
If the authenticator returns true without a user in cloudstack in DB, it will still fail. It only happens at a later stage. There are no new assumptions here. It will continue to work as earlier. If its not that would be a bug. I do not see any issue when running this either from the test cases or manually. if you see any issue report it and it can be addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38401570
--- Diff: setup/db/db/schema-452to460.sql ---
@@ -399,3 +399,14 @@ CREATE TABLE `cloud`.`external_bigswitch_bcf_devices` (
CONSTRAINT `fk_external_bigswitch_bcf_devices__physical_network_id` FOREIGN KEY (`physical_network_id`) REFERENCES `physical_network`(`id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+CREATE TABLE `cloud`.`ldap_trust_map` (
+ `id` int unsigned NOT NULL AUTO_INCREMENT,
--- End diff --
If domainId is unique then what is the need for id as PK?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/755
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38302129
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
--- End diff --
BaseCmd already has AccountService. Why another one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38726691
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
Issue of multiple CS domains mapping to same ldap group is tracked as part of CLOUDSTACK-8797. For now changes look fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38511573
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
The api semantics doesn't seem right here. It is ok for createUserAccount to fail if the user exists but in that case the response should have the UUID of the existing account. Also if the user doesn't exist but still the call fails then the entire API should fail, a tx would help in this case. The semantics should be if optional admin user parameter is specified then the admin UUID should always be present in the response in case of success otherwise the API call should return failure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38400169
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
The second call to createUserAccount will fail and authentication will fail in the second attempt.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38301615
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
--- End diff --
Shouldn't this be mandatory?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38513100
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
will update FS with this info.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38397126
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -189,6 +194,21 @@ public LdapUser getUser(final String username) throws NoLdapUserMatchingQueryExc
}
@Override
+ public LdapUser getUser(final String username, final String type, final String name) throws NoLdapUserMatchingQueryException {
+ LdapContext context = null;
+ try {
+ context = _ldapContextFactory.createBindContext();
+ final String escapedUsername = LdapUtils.escapeLDAPSearchFilter(username);
+ return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider()).getUser(escapedUsername, type, name, context);
+ } catch (NamingException | IOException e) {
+ s_logger.debug("ldap Exception: ",e);
+ throw new NoLdapUserMatchingQueryException("No Ldap User found for username: "+username + "name: " + name + "of type" + type);
--- End diff --
nit-pick - format the string to have spaces
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384082
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
@@ -39,6 +39,9 @@
private static final ConfigKey<String> ldapProvider = new ConfigKey<String>(String.class, "ldap.provider", "Advanced", "openldap", "ldap provider ex:openldap, microsoftad",
true, ConfigKey.Scope.Global, null);
+ private static final ConfigKey<Boolean> ldapEnableNestedGroups = new ConfigKey<Boolean>(Boolean.class, "ldap.nested.groups.enable", "Advanced", "true",
--- End diff --
ok
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38392290
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
--- End diff --
As long as the SQL exception is not directly passed to the caller it is fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38396794
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
the domain which is given during login
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38396925
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
createUserAccount can fail and that doesnt mean api failure. When it is successful, the new id is returned in response. check fs for sample request response in different scenarios.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38303804
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
+ //Account type constants in com.cloud.user.Account
+ Validate.isTrue(accountType>=0 && accountType<=5, "accountype should be a number from 0-5");
--- End diff --
The parameter description in the Cmd says allowed values are 0, 1, 2. Shouldn't this be in sync with description?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38391829
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
What if user doesn't exist but still createUserAccount() fails?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38303216
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
@@ -39,6 +39,9 @@
private static final ConfigKey<String> ldapProvider = new ConfigKey<String>(String.class, "ldap.provider", "Advanced", "openldap", "ldap provider ex:openldap, microsoftad",
true, ConfigKey.Scope.Global, null);
+ private static final ConfigKey<Boolean> ldapEnableNestedGroups = new ConfigKey<Boolean>(Boolean.class, "ldap.nested.groups.enable", "Advanced", "true",
--- End diff --
Please update FS with this config information.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38511940
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
In this case as well, the authn is successful but user sees failure as account creation has failed. The behaviour is not intuitive. If it is difficult to address it in implementation then should be documented as limitation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384161
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
--- End diff --
a message is shown in the ui with "an entity already exists". We can refine it. But an additional query to do the check is not necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38396821
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
+ Account account = _accountService.getActiveAccountByName(admin, domainId);
+ if (account == null) {
+ UserAccount userAccount =
+ _accountService.createUserAccount(admin, "", ldapUser.getFirstname(), ldapUser.getLastname(), ldapUser.getEmail(), null, admin, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, domainId, admin, null, UUID.randomUUID().toString(),
+ UUID.randomUUID().toString(), User.Source.LDAP);
--- End diff --
no.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384040
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
--- End diff --
didnt see that. will reuse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38515428
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
By authn I mean the call to LdapManager.canAuthenticate(). This will be successful in both cases but CS will report authn failure in once case due to account creation failure which is not intuitive. Account creation failure will be passed as authn failure.
But this will be a corner case scenario.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384124
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
--- End diff --
it fails when an enum is created. check the next few lines and the test case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-136249877
Can someone review please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38303506
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
--- End diff --
What if type is empty or something other than "GROUP" or "OU"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38303769
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
What if the name doesn't exist in LDAP?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38410923
--- Diff: setup/db/db/schema-452to460.sql ---
@@ -399,3 +399,14 @@ CREATE TABLE `cloud`.`external_bigswitch_bcf_devices` (
CONSTRAINT `fk_external_bigswitch_bcf_devices__physical_network_id` FOREIGN KEY (`physical_network_id`) REFERENCES `physical_network`(`id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+CREATE TABLE `cloud`.`ldap_trust_map` (
+ `id` int unsigned NOT NULL AUTO_INCREMENT,
--- End diff --
I definitely do not want to rename domain-id to id and make it primary. its easier and convenient to have a surrogate id.
It helps me in joins and foreign keys which may be required. It will also help in normalizations or any data structure changes in futures.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38397250
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -189,6 +194,21 @@ public LdapUser getUser(final String username) throws NoLdapUserMatchingQueryExc
}
@Override
+ public LdapUser getUser(final String username, final String type, final String name) throws NoLdapUserMatchingQueryException {
+ LdapContext context = null;
+ try {
+ context = _ldapContextFactory.createBindContext();
+ final String escapedUsername = LdapUtils.escapeLDAPSearchFilter(username);
+ return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider()).getUser(escapedUsername, type, name, context);
+ } catch (NamingException | IOException e) {
+ s_logger.debug("ldap Exception: ",e);
+ throw new NoLdapUserMatchingQueryException("No Ldap User found for username: "+username + "name: " + name + "of type" + type);
--- End diff --
will add a space after type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38726468
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
Please open a tracking bug to see if this can be improved to make the outcome more intuitive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38727056
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2177,6 +2173,7 @@ private UserAccount getUserAccount(String username, String password, Long domain
if (domain != null) {
domainName = domain.getName();
}
+ userAccount = _userAccountDao.getUserAccount(username, domainId);
--- End diff --
For now this is not an issue as every authenticator checks for the presence of user account. But the code flow of getUserAccount() method doesn't clearly brings it up. Earlier since there was a check for user account at the beginning it was very explicit. Now there is an implicit assumption that authenticator should return failure if the user account is not present in CCP. Please file a tracking bug for this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38392508
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
If multiple CS domains map to same ldap group, then when the user is authn. against ldap and created in CS (lazy creation), under which domain does it go?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-137664209
LGTM based on the tracking bugs for addressing some of the issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38731619
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
There is no bug here. Its how the current authenticators work. the api output will be improved based on the outcome of CLOUDSTACK-8796
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38396458
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
Shouldn't 'user' be assigned the output of this call?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-137758483
I am merging this as there are two :+1: Let me know if there are any objections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38400365
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2196,6 +2193,11 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Unable to authenticate user with username " + username + " in domain " + domainId);
}
+ if (userAccount == null) {
+ s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
--- End diff --
yes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38511898
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
In this case as well, the authn is successful but user sees failure as account creation has failed. The behaviour is not intuitive. If it is difficult to address it in implementation then should be documented as limitation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-137645397
travis build for the PR is here. https://travis-ci.org/apache/cloudstack/builds/78567814
for some reason, its not showing it here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-136980464
users are added based on the username. So, in this case LDAP user wont be added(it will error out). CS user will exist and will continue to work with earlier password using native authentication.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38399252
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2145,14 +2145,10 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Attempting to log in user: " + username + " in domain " + domainId);
}
UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
- if (userAccount == null) {
- s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
- return null;
- }
boolean authenticated = false;
HashSet<ActionOnFailedAuthentication> actionsOnFailedAuthenticaion = new HashSet<ActionOnFailedAuthentication>();
- User.Source userSource = userAccount.getSource();
+ User.Source userSource = userAccount != null ? userAccount.getSource(): User.Source.UNKNOWN;
--- End diff --
Ldap imported user will not have the source as unknown. If does, it will continue to check in other authenticators and if they return success, user will be authenticated.
Thats an existing behaviour which is not changed by this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38303622
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
--- End diff --
What if domainId is already present? I understand there will be a DB constraint violation exception but it is better to throw InvalidParameterException instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38512226
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2145,14 +2145,10 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Attempting to log in user: " + username + " in domain " + domainId);
}
UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
- if (userAccount == null) {
- s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
- return null;
- }
boolean authenticated = false;
HashSet<ActionOnFailedAuthentication> actionsOnFailedAuthenticaion = new HashSet<ActionOnFailedAuthentication>();
- User.Source userSource = userAccount.getSource();
+ User.Source userSource = userAccount != null ? userAccount.getSource(): User.Source.UNKNOWN;
--- End diff --
Think about it from the perspective of authenticators. Can a new authenticator break this logic?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38301757
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
linkDomainToAdmin() and createUserAccount() involves DB updates. Shouldn't it be in a tx? What if the first call succeeds and the second call fails?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38383968
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
--- End diff --
yes it should be. will change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38302765
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
+ Account account = _accountService.getActiveAccountByName(admin, domainId);
+ if (account == null) {
+ UserAccount userAccount =
+ _accountService.createUserAccount(admin, "", ldapUser.getFirstname(), ldapUser.getLastname(), ldapUser.getEmail(), null, admin, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, domainId, admin, null, UUID.randomUUID().toString(),
+ UUID.randomUUID().toString(), User.Source.LDAP);
--- End diff --
1. Why is password empty? Shouldn't it be taken as input?
2. What happens if createUserAccount() fails with "username already exists in domain" exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38398982
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2145,14 +2145,10 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Attempting to log in user: " + username + " in domain " + domainId);
}
UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
- if (userAccount == null) {
- s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
- return null;
- }
boolean authenticated = false;
HashSet<ActionOnFailedAuthentication> actionsOnFailedAuthenticaion = new HashSet<ActionOnFailedAuthentication>();
- User.Source userSource = userAccount.getSource();
+ User.Source userSource = userAccount != null ? userAccount.getSource(): User.Source.UNKNOWN;
--- End diff --
Looks like ldap user may be authenticated by another authenticator in case user source is unknown. What happens if non-ldap authenticator returns success for ldap user?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38512709
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
This results in creation of more than one CS account (one under every domain) for a single AD account. I don't think this is desirable as it may lead to confusion. If there is a need to support this kind of a scenario then it should be properly documented and called out in the FS.
Personally I feel AD group/ou should be mapped to a single domain.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-136589494
updated the code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38513021
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2145,14 +2145,10 @@ private UserAccount getUserAccount(String username, String password, Long domain
s_logger.debug("Attempting to log in user: " + username + " in domain " + domainId);
}
UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
- if (userAccount == null) {
- s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId);
- return null;
- }
boolean authenticated = false;
HashSet<ActionOnFailedAuthentication> actionsOnFailedAuthenticaion = new HashSet<ActionOnFailedAuthentication>();
- User.Source userSource = userAccount.getSource();
+ User.Source userSource = userAccount != null ? userAccount.getSource(): User.Source.UNKNOWN;
--- End diff --
I checked that after the discussion yesterday.
Every authenticator checks if a user exists in cloudstack. That responsibility is with the authenticator. In the earlier case, if the user doesnt exist, it throws a NPE on line 2155 which is a recent change. Prior to that change, any cloud provider can write an authenticator which always returns true even when cloudstack account doesnt exist. But, will break in lot of other places with NPEs as the rest of the app assumes logged in user has an account in cloudstack db. I think that needs to be fixed so that anyone can plugin their own user management system without using native cloudstack users.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384030
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
--- End diff --
its mentioned in fs. It will be disabled only when the user attempts to login. but, once the user is disabled, he wont be able to login. resources will continue to work
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38399204
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -2177,6 +2173,7 @@ private UserAccount getUserAccount(String username, String password, Long domain
if (domain != null) {
domainName = domain.getName();
}
+ userAccount = _userAccountDao.getUserAccount(username, domainId);
--- End diff --
There should be a null check for userAccount
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38392031
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
+ Account account = _accountService.getActiveAccountByName(admin, domainId);
+ if (account == null) {
+ UserAccount userAccount =
+ _accountService.createUserAccount(admin, "", ldapUser.getFirstname(), ldapUser.getLastname(), ldapUser.getEmail(), null, admin, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, domainId, admin, null, UUID.randomUUID().toString(),
+ UUID.randomUUID().toString(), User.Source.LDAP);
--- End diff --
Ldap authn. is fine but can this user be used for native CS authn.?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38397144
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
user isnt required after this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38302004
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
+ if(admin!=null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(admin, type, name);
+ if(!ldapUser.isDisabled()) {
--- End diff --
If the LDAP user is disabled after creation of CS user account, how will the account be removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/755#issuecomment-136946071
@karuturi LGTM, though one question - once an LDAP group is linked with a cloudstack domain, and then say if I manually create a user in cloudstack's domain first and then say add that same user (or different user but with the same username) what will happen, how does this resolve conflict or log this error?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38384216
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
@@ -243,4 +263,22 @@ public boolean isLdapEnabled() {
closeContext(context);
}
}
+
+ @Override
+ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, String name, short accountType) {
+ Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
+ Validate.notNull(domainId, "domainId cannot be null.");
+ Validate.notEmpty(name, "GROUP or OU name cannot be empty");
--- End diff --
if the name doesnt exist, authentication will fail.
Yes it is possible to map different cs domains to same ldap group
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38397611
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java ---
@@ -52,21 +58,71 @@ public LdapAuthenticator(final LdapManager ldapManager, final UserAccountDao use
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
}
- final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ boolean result = false;
+ ActionOnFailedAuthentication action = null;
- if (user == null) {
- s_logger.debug("Unable to find user with " + username + " in domain " + domainId);
- return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
- } else if (_ldapManager.isLdapEnabled()) {
- boolean result = _ldapManager.canAuthenticate(username, password);
- ActionOnFailedAuthentication action = null;
- if (result == false) {
+ if (_ldapManager.isLdapEnabled()) {
+ final UserAccount user = _userAccountDao.getUserAccount(username, domainId);
+ LdapTrustMapVO ldapTrustMapVO = _ldapManager.getDomainLinkedToLdap(domainId);
+ if(ldapTrustMapVO != null) {
+ try {
+ LdapUser ldapUser = _ldapManager.getUser(username, ldapTrustMapVO.getType().toString(), ldapTrustMapVO.getName());
+ if(!ldapUser.isDisabled()) {
+ result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password);
+ if(result) {
+ if(user == null) {
+ // import user to cloudstack
+ createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType());
--- End diff --
What happens if more than one thread calls authenticate for same user for the first time (i.e. both see user is not created)? Will one of the call to createCSUserAccount() fail? Ideally first call should create the account and 2nd should reuse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack:8647 LDAP Trust AD and Autoimp...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/755#discussion_r38726623
--- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.user.UserAccount;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.LinkDomainToLdapResponse;
+import org.apache.cloudstack.ldap.LdapManager;
+import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.cloudstack.ldap.NoLdapUserMatchingQueryException;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.Account;
+
+import java.util.UUID;
+
+@APICommand(name = "linkDomainToLdap", description = "link an existing cloudstack domain to group or OU in ldap", responseObject = LinkDomainToLdapResponse.class, since = "4.6.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class LinkDomainToLdapCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
+ private static final String s_name = "linkdomaintoldapresponse";
+
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
+ private String type;
+
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
+ private String name;
+
+ @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
+ private String admin;
+
+ @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
+ "admin, and 2 for domain admin")
+ private short accountType;
+
+ @Inject
+ private LdapManager _ldapManager;
+
+ @Inject
+ public AccountService _accountService;
+
+ @Override
+ public void execute() throws ServerApiException {
+ try {
+ LinkDomainToLdapResponse response = _ldapManager.linkDomainToLdap(domainId, type, name, accountType);
--- End diff --
Since the semantic aspect of the API will be revisited as part of CLOUDSTACK-8796, for now the changes are ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---