[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r361098706
##
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:
tnx @rhtyd. @nvazquez I am not changing this. Having functionality specific
to an enum value be implemented by that enum value is a more neat solution than
having a switch in calling code. we'll have a beer about it, next conf.
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360813276
##
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360432047 ## 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: i downed the version to 2.0.0.AM25 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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360407183
##
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:
done
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360407044
##
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:
_queryService.searchForUsers(..) will at least return an empty list of
userresponses, never null.
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360404508 ## 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: done 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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360403041
##
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360401916
##
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360401970
##
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360398649
##
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:
not sure if this is the place to check for validity of user names. Why do
you think we could encounter a blank here?
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360396116
##
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:
yes, i didn't have headspace to think on testing this, yet.
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360387379
##
File path:
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/LdapConstants.java
##
@@ -0,0 +1,21 @@
+// 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;
+
+public interface LdapConstants {
Review comment:
anticipation. I'm not against moving it but it is a very specific ldap
related term, so leaving it for now.
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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360386594 ## 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: Pulling the global ones from the top level pom and the local ones that are test specific only in the local properties section. 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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360384545 ## 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: i'll test, but i think not :( 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] DaanHoogland commented on a change in pull request #3694: Ldap fixes
DaanHoogland commented on a change in pull request #3694: Ldap fixes
URL: https://github.com/apache/cloudstack/pull/3694#discussion_r360311276
##
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:
extended as explanation
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
