Pearl1594 commented on code in PR #11436:
URL: https://github.com/apache/cloudstack/pull/11436#discussion_r2606956111
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java:
##########
@@ -106,14 +105,14 @@ public LdapImportUsersCmd(final LdapManager ldapManager,
final DomainService dom
private void createCloudstackUserAccount(LdapUser user, String
accountName, Domain domain) {
Account account = _accountService.getActiveAccountByName(accountName,
domain.getId());
if (account == null) {
- logger.debug("No account exists with name: " + accountName + "
creating the account and an user with name: " + user.getUsername() + " in the
account");
+ logger.debug("No account exists with name: {} creating the account
and an user with name: {} in the account", accountName, user.getUsername());
Review Comment:
```suggestion
logger.debug("No account exists with name: {}. Creating the
account and a user with name: {} in the account", accountName,
user.getUsername());
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java:
##########
@@ -202,11 +201,11 @@ private Domain getDomainForName(String name) {
private Domain getDomain(LdapUser user) {
Domain domain;
if (_domain != null) {
- //this means either domain id or groupname is passed and this will
be same for all the users in this call. hence returning it.
+ //this means either domain id or group name is passed and this
will be same for all the users in this call. hence returning it.
Review Comment:
```suggestion
// This means that either a domain id or group name is passed
and this will be same for all the users in this call, hence returning it.
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java:
##########
@@ -211,48 +206,34 @@ UserFilter getUserFilter() {
return UserFilter.fromString(getUserFilterString());
}
- boolean isACloudstackUser(final LdapUser ldapUser) {
+ boolean isACloudStackUser(final LdapUser ldapUser) {
+ String username = ldapUser.getUsername();
+ return isACloudStackUser(username);
+ }
+
+ boolean isACloudStackUser(final LdapUserResponse ldapUser) {
+ logger.trace("checking response : {}", ldapUser.toString());
+ String username = ldapUser.getUsername();
+ return isACloudStackUser(username);
+ }
+
+ private boolean isACloudStackUser(String username) {
boolean rc = false;
final List<UserResponse> cloudstackUsers = getCloudstackUsers();
- if (cloudstackUsers != null) {
+ if (CollectionUtils.isNotEmpty(cloudstackUsers)) {
for (final UserResponse cloudstackUser : cloudstackUsers) {
- if
(ldapUser.getUsername().equals(cloudstackUser.getUsername())) {
- if(logger.isTraceEnabled()) {
- logger.trace(String.format("found user %s in
cloudstack", ldapUser.getUsername()));
- }
-
+ if (username.equals(cloudstackUser.getUsername())) {
+ logger.trace("found user {} in cloudstack user {}",
username, cloudstackUser.getUsername());
Review Comment:
```suggestion
logger.trace("Found user {} in CloudStack",
cloudstackUser.getUsername());
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java:
##########
@@ -433,10 +414,10 @@ public List<LdapUserResponse>
filterPotentialImport(List<LdapUserResponse> input
logger.trace("should be filtering potential imports!!!");
}
// functional possibility do not add only users not yet in cloudstack
but include users that would be moved if they are so in ldap?
- // this means if they are part of a account linked to an ldap group/ou
+ // this means if they are part of an account linked to a ldap group/ou
Review Comment:
```suggestion
// this means if they are part of an account linked to an ldap
group/ou
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java:
##########
@@ -39,7 +39,7 @@ public List<LdapUser> getUsersInGroup(String groupName,
LdapContext context, Lon
throw new IllegalArgumentException("ldap group name cannot be
blank");
}
- String basedn = _ldapConfiguration.getBaseDn(domainId);
+ String basedn = LdapConfiguration.getBaseDn(domainId);
Review Comment:
should we rather have
```suggestion
String basedn = ldapConfiguration.getBaseDn(domainId);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LinkAccountToLdapCmd.java:
##########
@@ -81,7 +81,7 @@ public void execute() throws ServerApiException {
try {
ldapUser = _ldapManager.getUser(admin, type, ldapDomain,
domainId);
} catch (NoLdapUserMatchingQueryException e) {
- logger.debug("no ldap user matching username " + admin + "
in the given group/ou", e);
+ logger.debug("no ldap user matching username {} in the
given group/ou", admin, e);
Review Comment:
```suggestion
logger.debug("no LDAP user matching username {} in the
given group/OU", admin, e);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/dao/LdapConfigurationDao.java:
##########
@@ -29,16 +29,14 @@
public interface LdapConfigurationDao extends GenericDao<LdapConfigurationVO,
Long> {
/**
* @deprecated there might well be more then one ldap implementation on a
host and or a double binding of several domains
- * @param hostname
- * @return
+ * @param hostname the hostname or ip address of the ldap server
Review Comment:
```suggestion
* @param hostname the hostname or ip address of the LDAP server
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/dao/LdapConfigurationDao.java:
##########
@@ -29,16 +29,14 @@
public interface LdapConfigurationDao extends GenericDao<LdapConfigurationVO,
Long> {
/**
* @deprecated there might well be more then one ldap implementation on a
host and or a double binding of several domains
- * @param hostname
- * @return
+ * @param hostname the hostname or ip address of the ldap server
+ * @return the ldap configuration for the given hostname
Review Comment:
```suggestion
* @return the LDAP configuration for the given hostname
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java:
##########
@@ -202,11 +201,11 @@ private Domain getDomainForName(String name) {
private Domain getDomain(LdapUser user) {
Domain domain;
if (_domain != null) {
- //this means either domain id or groupname is passed and this will
be same for all the users in this call. hence returning it.
+ //this means either domain id or group name is passed and this
will be same for all the users in this call. hence returning it.
domain = _domain;
} else {
if (domainId != null) {
- // a domain Id is passed. use it for this user and all the
users in the same api call (by setting _domain)
+ // a domain ID is passed. use it for this user and all the
users in the same api call (by setting _domain)
Review Comment:
```suggestion
// a domain ID is passed. Use it for this user and all the
users in the same API call (by setting _domain)
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -180,7 +180,7 @@ private LdapConfigurationResponse
addConfigurationInternal(final String hostname
context =
_ldapContextFactory.createBindContext(providerUrl,domainId);
configuration = new LdapConfigurationVO(hostname, port,
domainId);
_ldapConfigurationDao.persist(configuration);
- logger.info("Added new ldap server with url: " + providerUrl +
(domainId == null ? "": " for domain " + domainId));
+ logger.info("Added new ldap server with url: {}{}",
providerUrl, domainId == null ? "" : " for domain " + domainId);
Review Comment:
```suggestion
logger.info("Added a new LDAP server with URL: {}{}",
providerUrl, domainId == null ? "" : " for domain " + domainId);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -432,14 +430,18 @@ private LinkDomainToLdapResponse linkDomainToLdap(Long
domainId, String type, St
LinkType linkType = LdapManager.LinkType.valueOf(type.toUpperCase());
LdapTrustMapVO vo = _ldapTrustMapDao.persist(new
LdapTrustMapVO(domainId, linkType, name, accountType, 0));
DomainVO domain = domainDao.findById(vo.getDomainId());
+ String domainUuid = getDomainUuid(domain, vo);
+ return new LinkDomainToLdapResponse(domainUuid,
vo.getType().toString(), vo.getName(), vo.getAccountType().ordinal());
+ }
+
+ private String getDomainUuid(DomainVO domain, LdapTrustMapVO vo) {
String domainUuid = "<unknown>";
if (domain == null) {
- logger.error("no domain in database for id " + vo.getDomainId());
+ logger.error("no domain in database for id {}", vo.getDomainId());
Review Comment:
```suggestion
logger.error("No domain in database for id {}",
vo.getDomainId());
```
However, do we need to log this with the database ID?
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -211,13 +211,11 @@ public boolean canAuthenticate(final String principal,
final String password, fi
// TODO return the right account for this user
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));
- }
+ logger.trace("User({}) authenticated for domain({})", principal,
domainId);
return true;
} catch (NamingException | IOException e) {/* AuthenticationException
is caught as NamingException */
- logger.debug("Exception while doing an LDAP bind for user "+"
"+principal, e);
- logger.info("Failed to authenticate user: " + principal + ".
incorrect password.");
+ logger.debug("Exception while doing an LDAP bind for user {}",
principal, e);
+ logger.info("Failed to authenticate user: {}. incorrect
password.", principal);
Review Comment:
```suggestion
logger.info("Failed to authenticate user: {}. Incorrect
password.", principal);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LinkAccountToLdapCmd.java:
##########
@@ -99,7 +99,7 @@ public void execute() throws ServerApiException {
logger.debug("an account with name {} already exists
in the domain {} with id {}", admin, _domainService.getDomain(domainId),
domainId);
}
} else {
- logger.debug("ldap user with username " + admin + " is
disabled in the given group/ou");
+ logger.debug("ldap user with username {} is disabled in
the given group/ou", admin);
Review Comment:
```suggestion
logger.debug("LDAP user with username {} is disabled in
the given group/OU", admin);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:
##########
@@ -90,7 +91,7 @@ public Pair<Boolean, ActionOnFailedAuthentication>
authenticate(final String use
}
} else {
if (logger.isTraceEnabled()) {
- logger.trace(String.format("'this' domain (%d) is not
linked to ldap follow normal authentication", domainId));
+ logger.trace("'this' domain ({}) is not linked to ldap
follow normal authentication", domainId);
Review Comment:
```suggestion
logger.trace("'this' domain ({}) is not linked to
LDAP follow normal authentication", domainId);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java:
##########
@@ -106,14 +105,14 @@ public LdapImportUsersCmd(final LdapManager ldapManager,
final DomainService dom
private void createCloudstackUserAccount(LdapUser user, String
accountName, Domain domain) {
Account account = _accountService.getActiveAccountByName(accountName,
domain.getId());
if (account == null) {
- logger.debug("No account exists with name: " + accountName + "
creating the account and an user with name: " + user.getUsername() + " in the
account");
+ logger.debug("No account exists with name: {} creating the account
and an user with name: {} in the account", accountName, user.getUsername());
_accountService.createUserAccount(user.getUsername(),
generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(),
timezone, accountName, getAccountType(), getRoleId(),
domain.getId(), domain.getNetworkDomain(), details,
UUID.randomUUID().toString(), UUID.randomUUID().toString(), User.Source.LDAP);
} else {
// check if the user exists. if yes, call update
UserAccount csuser =
_accountService.getActiveUserAccount(user.getUsername(), domain.getId());
if (csuser == null) {
- logger.debug("No user exists with name: " + user.getUsername()
+ " creating a user in the account: " + accountName);
+ logger.debug("No user exists with name: {} creating a user in
the account: {}", user.getUsername(), accountName);
Review Comment:
```suggestion
logger.debug("No user exists with name: {}. Creating a user
in the account: {}", user.getUsername(), accountName);
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
