[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-23 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360865125
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -201,33 +199,10 @@ public void execute() throws ServerApiException {
 if(s_logger.isTraceEnabled()) {
 s_logger.trace(String.format("applying filter: %s or %s.", 
this.getListTypeString(), this.getUserFilter()));
 }
-Method filterMethod = getFilterMethod();
-List responseList = ldapResponses;
-if (filterMethod != null) {
-if(s_logger.isTraceEnabled()) {
-s_logger.trace("applying filter: " + filterMethod.getName());
-}
-try {
-responseList = 
(List)filterMethod.invoke(this, ldapResponses);
-} catch (IllegalAccessException | InvocationTargetException e) {
-throw new CloudRuntimeException("we're damned, the earth is 
screwed. we will now return to our maker",e);
-}
-}
+List responseList = 
getUserFilter().filter(this,ldapResponses);
 
 Review comment:
   Looks better but I would prefer having an if-then-else block here instead of 
the abstract method implemented on the enum


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-20 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360414511
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -104,20 +241,251 @@ public long getEntityOwnerId() {
 return Account.ACCOUNT_ID_SYSTEM;
 }
 
-private String getListType() {
+String getListTypeString() {
 return listType == null ? "all" : listType;
 }
 
-private boolean isACloudstackUser(final LdapUser ldapUser) {
-final ListResponse response = 
_queryService.searchForUsers(new ListUsersCmd());
-final List cloudstackUsers = response.getResponses();
+String getUserFilterString() {
+return userFilter == null ? getListTypeString() == null ? "NoFilter" : 
getListTypeString().equals("all") ? "NoFilter" : "AnyDomain" : userFilter;
+}
+
+UserFilter getUserFilter() {
+return UserFilter.fromString(getUserFilterString());
+}
+
+boolean isACloudstackUser(final LdapUser ldapUser) {
+boolean rc = false;
+final List cloudstackUsers = getCloudstackUsers();
+if (cloudstackUsers != null) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
+if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
 
 Review comment:
   Just defensive check prior comparison, but ok to leave it as it is if the 
received list does not contain empty usernames (which shouldnt)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-20 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360413353
 
 

 ##
 File path: plugins/user-authenticators/ldap/pom.xml
 ##
 @@ -27,6 +27,11 @@
 4.14.0.0-SNAPSHOT
 ../../pom.xml
 
+
+
+2.0.0.AM26-SNAPSHOT
 
 Review comment:
   Should be good to rely on a release version to avoid any changes that may 
break functionality on this PR, but ok anyways


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358937338
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -104,20 +241,251 @@ public long getEntityOwnerId() {
 return Account.ACCOUNT_ID_SYSTEM;
 }
 
-private String getListType() {
+String getListTypeString() {
 return listType == null ? "all" : listType;
 }
 
-private boolean isACloudstackUser(final LdapUser ldapUser) {
-final ListResponse response = 
_queryService.searchForUsers(new ListUsersCmd());
-final List cloudstackUsers = response.getResponses();
+String getUserFilterString() {
+return userFilter == null ? getListTypeString() == null ? "NoFilter" : 
getListTypeString().equals("all") ? "NoFilter" : "AnyDomain" : userFilter;
+}
+
+UserFilter getUserFilter() {
+return UserFilter.fromString(getUserFilterString());
+}
+
+boolean isACloudstackUser(final LdapUser ldapUser) {
+boolean rc = false;
+final List cloudstackUsers = getCloudstackUsers();
+if (cloudstackUsers != null) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
+if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
+
+rc = true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
+}
+}
+}
+return rc;
+}
+
+boolean isACloudstackUser(final LdapUserResponse ldapUser) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("checking response : " + ldapUser.toString());
+}
+final List cloudstackUsers = getCloudstackUsers();
 if (cloudstackUsers != null && cloudstackUsers.size() != 0) {
-for (final UserResponse cloudstackUser : response.getResponses()) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
 if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
 return true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
 }
 }
 }
 return false;
 }
+/**
+ * typecheck for userfilter values
+ */
+enum UserFilter {
+NO_FILTER("NoFilter"),
+LOCAL_DOMAIN("LocalDomain"),
+ANY_DOMAIN("AnyDomain"),
+POTENTIAL_IMPORT("PotentialImport");
+
+private final String value;
+
+UserFilter(String val) {
+this.value = val;
+}
+
+static UserFilter fromString(String val) {
+if(NO_FILTER.toString().equalsIgnoreCase(val)) {
+return NO_FILTER;
+} else if (LOCAL_DOMAIN.toString().equalsIgnoreCase(val)) {
+return LOCAL_DOMAIN;
+} else if(ANY_DOMAIN.toString().equalsIgnoreCase(val)) {
+return ANY_DOMAIN;
+} else if(POTENTIAL_IMPORT.toString().equalsIgnoreCase(val)) {
+return POTENTIAL_IMPORT;
+} else {
+throw new IllegalArgumentException(String.format("%s is not a 
legal 'UserFilter' value", val));
+}
+}
+
+@Override public String toString() {
+return value;
+}
+}
+
+/**
+ * no filtering but improve with annotation of source for existing ACS 
users
+ * @param input ldap response list of users
+ * @return unfiltered list of the input list of ldap users
+ */
+public List filterNoFilter(List input) 
{
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("returning unfiltered list of ldap users");
+}
+annotateUserListWithSources(input);
+return input;
+}
+
+/**
+ * filter the list of ldap users. no users visible to the caller should be 
in the returned list
+ * @param input ldap response list of users
+ * @return a list of ldap users not already in ACS
+ */
+public List filterAnyDomain(List 
input) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("filtering existing users");
+

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358926572
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -66,34 +122,115 @@ public LdapListUsersCmd(final LdapManager ldapManager, 
final QueryService queryS
 _queryService = queryService;
 }
 
+/**
+ * (as a check for isACloudstackUser is done) only non cloudstack users 
should be shown
+ * @param users a list of {@code LdapUser}s
+ * @return a (filtered?) list of user response objects
+ */
 private List createLdapUserResponse(final List 
users) {
 final List ldapResponses = new 
ArrayList();
 for (final LdapUser user : users) {
-if (getListType().equals("all") || !isACloudstackUser(user)) {
-final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
-ldapResponse.setObjectName("LdapUser");
-ldapResponses.add(ldapResponse);
-}
+final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
+ldapResponse.setObjectName("LdapUser");
+ldapResponses.add(ldapResponse);
 }
 return ldapResponses;
 }
 
+private List cloudstackUsers = null;
+
 @Override
 public void execute() throws ServerApiException {
-List ldapResponses = null;
+cloudstackUsers = null;
+List ldapResponses = new 
ArrayList();
 final ListResponse response = new 
ListResponse();
 try {
-final List users = _ldapManager.getUsers(null);
+final List users = _ldapManager.getUsers(domainId);
 ldapResponses = createLdapUserResponse(users);
+//now filter and annotate
+ldapResponses = applyUserFilter(ldapResponses);
 } catch (final NoLdapUserMatchingQueryException ex) {
-ldapResponses = new ArrayList();
+// ok, we'll make do with the empty list ldapResponses = new 
ArrayList();
 } finally {
 response.setResponses(ldapResponses);
 response.setResponseName(getCommandName());
 setResponseObject(response);
 }
 }
 
+private List getCloudstackUsers() {
+// get a list of relevant cloudstack users, meaning
+// if we are filtering for local domain, only get users for the 
current domain
+// if we are filtering for any domain, get recursive all users for the 
root domain
+// if we are filtering for potential imports,
+//we are only looking for users in the linked domains/accounts,
+//which is only relevant if we ask ldap users for this domain.
+//So we are asking for all users in the current domain as well
+// in case of no filter we should find all users in the current domain 
for annotation.
+if (cloudstackUsers == null) {
+ListResponse cloudstackUsersresponse;
+switch (getUserFilter()) {
+case ANY_DOMAIN:
 
 Review comment:
   Minor one - can these cases be indented with a tab?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358932516
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -104,20 +241,251 @@ public long getEntityOwnerId() {
 return Account.ACCOUNT_ID_SYSTEM;
 }
 
-private String getListType() {
+String getListTypeString() {
 return listType == null ? "all" : listType;
 }
 
-private boolean isACloudstackUser(final LdapUser ldapUser) {
-final ListResponse response = 
_queryService.searchForUsers(new ListUsersCmd());
-final List cloudstackUsers = response.getResponses();
+String getUserFilterString() {
+return userFilter == null ? getListTypeString() == null ? "NoFilter" : 
getListTypeString().equals("all") ? "NoFilter" : "AnyDomain" : userFilter;
+}
+
+UserFilter getUserFilter() {
+return UserFilter.fromString(getUserFilterString());
+}
+
+boolean isACloudstackUser(final LdapUser ldapUser) {
+boolean rc = false;
+final List cloudstackUsers = getCloudstackUsers();
+if (cloudstackUsers != null) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
+if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
 
 Review comment:
   Can you also add StringUtils.isNotBlank() check?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358930053
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -66,34 +122,115 @@ public LdapListUsersCmd(final LdapManager ldapManager, 
final QueryService queryS
 _queryService = queryService;
 }
 
+/**
+ * (as a check for isACloudstackUser is done) only non cloudstack users 
should be shown
+ * @param users a list of {@code LdapUser}s
+ * @return a (filtered?) list of user response objects
+ */
 private List createLdapUserResponse(final List 
users) {
 final List ldapResponses = new 
ArrayList();
 for (final LdapUser user : users) {
-if (getListType().equals("all") || !isACloudstackUser(user)) {
-final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
-ldapResponse.setObjectName("LdapUser");
-ldapResponses.add(ldapResponse);
-}
+final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
+ldapResponse.setObjectName("LdapUser");
+ldapResponses.add(ldapResponse);
 }
 return ldapResponses;
 }
 
+private List cloudstackUsers = null;
+
 @Override
 public void execute() throws ServerApiException {
-List ldapResponses = null;
+cloudstackUsers = null;
+List ldapResponses = new 
ArrayList();
 final ListResponse response = new 
ListResponse();
 try {
-final List users = _ldapManager.getUsers(null);
+final List users = _ldapManager.getUsers(domainId);
 ldapResponses = createLdapUserResponse(users);
+//now filter and annotate
+ldapResponses = applyUserFilter(ldapResponses);
 } catch (final NoLdapUserMatchingQueryException ex) {
-ldapResponses = new ArrayList();
+// ok, we'll make do with the empty list ldapResponses = new 
ArrayList();
 } finally {
 response.setResponses(ldapResponses);
 response.setResponseName(getCommandName());
 setResponseObject(response);
 }
 }
 
+private List getCloudstackUsers() {
+// get a list of relevant cloudstack users, meaning
+// if we are filtering for local domain, only get users for the 
current domain
+// if we are filtering for any domain, get recursive all users for the 
root domain
+// if we are filtering for potential imports,
+//we are only looking for users in the linked domains/accounts,
+//which is only relevant if we ask ldap users for this domain.
+//So we are asking for all users in the current domain as well
+// in case of no filter we should find all users in the current domain 
for annotation.
+if (cloudstackUsers == null) {
+ListResponse cloudstackUsersresponse;
+switch (getUserFilter()) {
+case ANY_DOMAIN:
+// get the user domain so if the calling user is a root admin 

+cloudstackUsersresponse = 
_queryService.searchForUsers(CallContext.current().getCallingAccount().getDomainId(),
 true);
+break;
+case NO_FILTER:
+cloudstackUsersresponse = 
_queryService.searchForUsers(this.domainId,true);
+break;
+case POTENTIAL_IMPORT:
+case LOCAL_DOMAIN:
+cloudstackUsersresponse = 
_queryService.searchForUsers(this.domainId,false);
+break;
+default:
+throw new CloudRuntimeException("error in program login; we 
are not filtering but still querying users to filter???");
+}
+cloudstackUsers = cloudstackUsersresponse.getResponses();
+if(s_logger.isTraceEnabled()) {
+StringBuilder users = new StringBuilder();
+for (UserResponse user : cloudstackUsers) {
+if (users.length()> 0) {
+users.append(", ");
+}
+users.append(user.getUsername());
+}
+
+s_logger.trace(String.format("checking against %d 
cloudstackusers: %s.", this.cloudstackUsers.size(), users.toString()));
+}
+}
+return cloudstackUsers;
+}
+
+private List applyUserFilter(List 
ldapResponses) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("applying filter: %s or %s.", 
this.getListTypeString(), this.getUserFilter()));
+}
+Method filterMethod = getFilterMethod();
+List responseList = ldapResponses;
+if (filterMethod != null) {
+if(s_logger.isTraceEnabled()) {
+

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358907873
 
 

 ##
 File path: plugins/user-authenticators/ldap/pom.xml
 ##
 @@ -81,38 +86,126 @@
 **/*Spec.groovy
 **/*Test.java
 
+
+META-INF/*.SF
+META-INF/*.DSA
+META-INF/*.RSA
+
 
 
 
 com.btmatthews.maven.plugins
 ldap-maven-plugin
-1.1.0
+1.1.3
 
 11389
 ldap
 false
 dc=cloudstack,dc=org
 10389
-test/resources/cloudstack.org.ldif
+src/test/resources/cloudstack.org.ldif
 
 
 
-test
+src/test/java
 
 
 
+
+com.btmatthews.ldapunit
+ldapunit
+1.1.3
 
 Review comment:
   Following the same pattern for dependencies, can version be moved to the 
properties section? This one and the ones below


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358917787
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -38,24 +48,70 @@
 
 import com.cloud.user.Account;
 
-@APICommand(name = "listLdapUsers", responseObject = LdapUserResponse.class, 
description = "Lists all LDAP Users", since = "4.2.0",
-requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+/**
+ * @startuml
+ * start
+ * :list ldap users request;
+ * :get ldap binding;
+ * if (domain == null) then (true)
+ *   :get global trust domain;
+ * else (false)
+ *   :get trustdomain for domain;
+ * endif
+ * :get ldap users\n using trust domain;
+ * if (filter == 'NoFilter') then (pass as is)
+ * elseif (filter == 'AnyDomain') then (anydomain)
+ *   :filterList = all\n\t\tcloudstack\n\t\tusers;
+ * elseif (filter == 'LocalDomain')
+ *   :filterList = local users\n\t\tfor domain;
+ * elseif (filter == 'PotentialImport') then (address 
account\nsynchronisation\nconfigurations)
+ *   :query\n the account\n bindings;
+ *   :check and markup\n ldap users\n for bound OUs\n with usersource;
+ * else ( unknown value for filter )
+ *   :throw invalid parameter;
+ *   stop
+ * endif
+ *   :remove users in filterList\nfrom ldap users list;
+ * :return remaining;
+ * stop
+ * @enduml
+ */
 
 Review comment:
   +1
   can you add a short comment on what is needed to "see" the UML?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358918976
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -38,24 +48,70 @@
 
 import com.cloud.user.Account;
 
-@APICommand(name = "listLdapUsers", responseObject = LdapUserResponse.class, 
description = "Lists all LDAP Users", since = "4.2.0",
-requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+/**
+ * @startuml
+ * start
+ * :list ldap users request;
+ * :get ldap binding;
+ * if (domain == null) then (true)
+ *   :get global trust domain;
+ * else (false)
+ *   :get trustdomain for domain;
+ * endif
+ * :get ldap users\n using trust domain;
+ * if (filter == 'NoFilter') then (pass as is)
+ * elseif (filter == 'AnyDomain') then (anydomain)
+ *   :filterList = all\n\t\tcloudstack\n\t\tusers;
+ * elseif (filter == 'LocalDomain')
+ *   :filterList = local users\n\t\tfor domain;
+ * elseif (filter == 'PotentialImport') then (address 
account\nsynchronisation\nconfigurations)
+ *   :query\n the account\n bindings;
+ *   :check and markup\n ldap users\n for bound OUs\n with usersource;
+ * else ( unknown value for filter )
+ *   :throw invalid parameter;
+ *   stop
+ * endif
+ *   :remove users in filterList\nfrom ldap users list;
+ * :return remaining;
+ * stop
+ * @enduml
+ */
+@APICommand(name = "listLdapUsers", responseObject = LdapUserResponse.class, 
description = "Lists LDAP Users according to the specifications from the user 
request.", since = "4.2.0",
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, 
authorized = {RoleType.Admin,RoleType.DomainAdmin})
 public class LdapListUsersCmd extends BaseListCmd {
 
 public static final Logger s_logger = 
Logger.getLogger(LdapListUsersCmd.class.getName());
 private static final String s_name = "ldapuserresponse";
 @Inject
 private LdapManager _ldapManager;
 
+// TODO queryservice is already injected oin basecmd remove it here
 @Inject
 private QueryService _queryService;
 
 Review comment:
   Can we remove it as the comment says?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358933052
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -104,20 +241,251 @@ public long getEntityOwnerId() {
 return Account.ACCOUNT_ID_SYSTEM;
 }
 
-private String getListType() {
+String getListTypeString() {
 return listType == null ? "all" : listType;
 }
 
-private boolean isACloudstackUser(final LdapUser ldapUser) {
-final ListResponse response = 
_queryService.searchForUsers(new ListUsersCmd());
-final List cloudstackUsers = response.getResponses();
+String getUserFilterString() {
+return userFilter == null ? getListTypeString() == null ? "NoFilter" : 
getListTypeString().equals("all") ? "NoFilter" : "AnyDomain" : userFilter;
+}
+
+UserFilter getUserFilter() {
+return UserFilter.fromString(getUserFilterString());
+}
+
+boolean isACloudstackUser(final LdapUser ldapUser) {
+boolean rc = false;
+final List cloudstackUsers = getCloudstackUsers();
+if (cloudstackUsers != null) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
+if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
+
+rc = true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
+}
+}
+}
+return rc;
+}
+
+boolean isACloudstackUser(final LdapUserResponse ldapUser) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("checking response : " + ldapUser.toString());
+}
+final List cloudstackUsers = getCloudstackUsers();
 if (cloudstackUsers != null && cloudstackUsers.size() != 0) {
-for (final UserResponse cloudstackUser : response.getResponses()) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
 if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
 return true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
 }
 }
 }
 return false;
 }
+/**
+ * typecheck for userfilter values
+ */
+enum UserFilter {
+NO_FILTER("NoFilter"),
+LOCAL_DOMAIN("LocalDomain"),
+ANY_DOMAIN("AnyDomain"),
+POTENTIAL_IMPORT("PotentialImport");
+
+private final String value;
+
+UserFilter(String val) {
+this.value = val;
+}
+
+static UserFilter fromString(String val) {
+if(NO_FILTER.toString().equalsIgnoreCase(val)) {
+return NO_FILTER;
+} else if (LOCAL_DOMAIN.toString().equalsIgnoreCase(val)) {
+return LOCAL_DOMAIN;
+} else if(ANY_DOMAIN.toString().equalsIgnoreCase(val)) {
+return ANY_DOMAIN;
+} else if(POTENTIAL_IMPORT.toString().equalsIgnoreCase(val)) {
+return POTENTIAL_IMPORT;
+} else {
+throw new IllegalArgumentException(String.format("%s is not a 
legal 'UserFilter' value", val));
+}
+}
+
+@Override public String toString() {
+return value;
+}
+}
+
+/**
+ * no filtering but improve with annotation of source for existing ACS 
users
+ * @param input ldap response list of users
+ * @return unfiltered list of the input list of ldap users
+ */
+public List filterNoFilter(List input) 
{
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("returning unfiltered list of ldap users");
+}
+annotateUserListWithSources(input);
+return input;
+}
+
+/**
+ * filter the list of ldap users. no users visible to the caller should be 
in the returned list
+ * @param input ldap response list of users
+ * @return a list of ldap users not already in ACS
+ */
+public List filterAnyDomain(List 
input) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("filtering existing users");
+

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358927359
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -66,34 +122,115 @@ public LdapListUsersCmd(final LdapManager ldapManager, 
final QueryService queryS
 _queryService = queryService;
 }
 
+/**
+ * (as a check for isACloudstackUser is done) only non cloudstack users 
should be shown
+ * @param users a list of {@code LdapUser}s
+ * @return a (filtered?) list of user response objects
+ */
 private List createLdapUserResponse(final List 
users) {
 final List ldapResponses = new 
ArrayList();
 for (final LdapUser user : users) {
-if (getListType().equals("all") || !isACloudstackUser(user)) {
-final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
-ldapResponse.setObjectName("LdapUser");
-ldapResponses.add(ldapResponse);
-}
+final LdapUserResponse ldapResponse = 
_ldapManager.createLdapUserResponse(user);
+ldapResponse.setObjectName("LdapUser");
+ldapResponses.add(ldapResponse);
 }
 return ldapResponses;
 }
 
+private List cloudstackUsers = null;
+
 @Override
 public void execute() throws ServerApiException {
-List ldapResponses = null;
+cloudstackUsers = null;
+List ldapResponses = new 
ArrayList();
 final ListResponse response = new 
ListResponse();
 try {
-final List users = _ldapManager.getUsers(null);
+final List users = _ldapManager.getUsers(domainId);
 ldapResponses = createLdapUserResponse(users);
+//now filter and annotate
+ldapResponses = applyUserFilter(ldapResponses);
 } catch (final NoLdapUserMatchingQueryException ex) {
-ldapResponses = new ArrayList();
+// ok, we'll make do with the empty list ldapResponses = new 
ArrayList();
 } finally {
 response.setResponses(ldapResponses);
 response.setResponseName(getCommandName());
 setResponseObject(response);
 }
 }
 
+private List getCloudstackUsers() {
+// get a list of relevant cloudstack users, meaning
+// if we are filtering for local domain, only get users for the 
current domain
+// if we are filtering for any domain, get recursive all users for the 
root domain
+// if we are filtering for potential imports,
+//we are only looking for users in the linked domains/accounts,
+//which is only relevant if we ask ldap users for this domain.
+//So we are asking for all users in the current domain as well
+// in case of no filter we should find all users in the current domain 
for annotation.
+if (cloudstackUsers == null) {
+ListResponse cloudstackUsersresponse;
+switch (getUserFilter()) {
+case ANY_DOMAIN:
+// get the user domain so if the calling user is a root admin 

+cloudstackUsersresponse = 
_queryService.searchForUsers(CallContext.current().getCallingAccount().getDomainId(),
 true);
+break;
+case NO_FILTER:
+cloudstackUsersresponse = 
_queryService.searchForUsers(this.domainId,true);
+break;
+case POTENTIAL_IMPORT:
+case LOCAL_DOMAIN:
+cloudstackUsersresponse = 
_queryService.searchForUsers(this.domainId,false);
+break;
+default:
+throw new CloudRuntimeException("error in program login; we 
are not filtering but still querying users to filter???");
+}
+cloudstackUsers = cloudstackUsersresponse.getResponses();
 
 Review comment:
   Null check?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358940815
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java
 ##
 @@ -142,12 +142,15 @@ private LdapConfigurationResponse 
addConfigurationInternal(final String hostname
 public boolean canAuthenticate(final String principal, final String 
password, final Long domainId) {
 try {
 // TODO return the right account for this user
-final LdapContext context = 
_ldapContextFactory.createUserContext(principal, password,domainId);
+final LdapContext context = 
_ldapContextFactory.createUserContext(principal, password, domainId);
 closeContext(context);
+if(LOGGER.isTraceEnabled()) {
+LOGGER.trace(String.format("User(%s) authenticated for 
domain(%s)", principal, domainId));
+}
 return true;
-} catch (NamingException | IOException e) {
-s_logger.debug("Exception while doing an LDAP bind for user "+" 
"+principal, e);
-s_logger.info("Failed to authenticate user: " + principal + ". 
incorrect password.");
+} catch (NamingException /* | AuthenticationException */ | IOException 
e) {
 
 Review comment:
   Can we remove this comment?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358933564
 
 

 ##
 File path: 
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java
 ##
 @@ -104,20 +241,251 @@ public long getEntityOwnerId() {
 return Account.ACCOUNT_ID_SYSTEM;
 }
 
-private String getListType() {
+String getListTypeString() {
 return listType == null ? "all" : listType;
 }
 
-private boolean isACloudstackUser(final LdapUser ldapUser) {
-final ListResponse response = 
_queryService.searchForUsers(new ListUsersCmd());
-final List cloudstackUsers = response.getResponses();
+String getUserFilterString() {
+return userFilter == null ? getListTypeString() == null ? "NoFilter" : 
getListTypeString().equals("all") ? "NoFilter" : "AnyDomain" : userFilter;
+}
+
+UserFilter getUserFilter() {
+return UserFilter.fromString(getUserFilterString());
+}
+
+boolean isACloudstackUser(final LdapUser ldapUser) {
+boolean rc = false;
+final List cloudstackUsers = getCloudstackUsers();
+if (cloudstackUsers != null) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
+if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
+
+rc = true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
+}
+}
+}
+return rc;
+}
+
+boolean isACloudstackUser(final LdapUserResponse ldapUser) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("checking response : " + ldapUser.toString());
+}
+final List cloudstackUsers = getCloudstackUsers();
 if (cloudstackUsers != null && cloudstackUsers.size() != 0) {
-for (final UserResponse cloudstackUser : response.getResponses()) {
+for (final UserResponse cloudstackUser : cloudstackUsers) {
 if 
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("found user %s in 
cloudstack", ldapUser.getUsername()));
+}
 return true;
+} else {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace(String.format("ldap user %s does not 
match cloudstack user", ldapUser.getUsername(), cloudstackUser.getUsername()));
+}
 }
 }
 }
 return false;
 }
+/**
+ * typecheck for userfilter values
+ */
+enum UserFilter {
+NO_FILTER("NoFilter"),
+LOCAL_DOMAIN("LocalDomain"),
+ANY_DOMAIN("AnyDomain"),
+POTENTIAL_IMPORT("PotentialImport");
+
+private final String value;
+
+UserFilter(String val) {
+this.value = val;
+}
+
+static UserFilter fromString(String val) {
+if(NO_FILTER.toString().equalsIgnoreCase(val)) {
+return NO_FILTER;
+} else if (LOCAL_DOMAIN.toString().equalsIgnoreCase(val)) {
+return LOCAL_DOMAIN;
+} else if(ANY_DOMAIN.toString().equalsIgnoreCase(val)) {
+return ANY_DOMAIN;
+} else if(POTENTIAL_IMPORT.toString().equalsIgnoreCase(val)) {
+return POTENTIAL_IMPORT;
+} else {
+throw new IllegalArgumentException(String.format("%s is not a 
legal 'UserFilter' value", val));
+}
+}
+
+@Override public String toString() {
+return value;
+}
+}
+
+/**
+ * no filtering but improve with annotation of source for existing ACS 
users
+ * @param input ldap response list of users
+ * @return unfiltered list of the input list of ldap users
+ */
+public List filterNoFilter(List input) 
{
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("returning unfiltered list of ldap users");
+}
+annotateUserListWithSources(input);
+return input;
+}
+
+/**
+ * filter the list of ldap users. no users visible to the caller should be 
in the returned list
+ * @param input ldap response list of users
+ * @return a list of ldap users not already in ACS
+ */
+public List filterAnyDomain(List 
input) {
+if(s_logger.isTraceEnabled()) {
+s_logger.trace("filtering existing users");
+

[GitHub] [cloudstack] nvazquez commented on a change in pull request #3694: Ldap fixes

2019-12-17 Thread GitBox 
nvazquez commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r358907249
 
 

 ##
 File path: plugins/user-authenticators/ldap/pom.xml
 ##
 @@ -27,6 +27,11 @@
 4.14.0.0-SNAPSHOT
 ../../pom.xml
 
+
+
+2.0.0.AM26-SNAPSHOT
 
 Review comment:
   Can't we use a release version instead of snapshot?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services