Skip to content

Commit

Permalink
feat: sanity checks on user join and before sending any notification …
Browse files Browse the repository at this point in the history
…email (#586)
  • Loading branch information
ymarcon authored Jan 11, 2025
1 parent 66f79ea commit 4e219fe
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 18 deletions.
4 changes: 4 additions & 0 deletions agate-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@
<artifactId>postgresql</artifactId>
</dependency>

<dependency>
<groupId>org.owasp.esapi</groupId>
<artifactId>esapi</artifactId>
</dependency>
<dependency>
<groupId>dev.samstevens.totp</groupId>
<artifactId>totp</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import org.hibernate.validator.constraints.NotBlank;
import org.obiba.mongodb.domain.AbstractAuditableDocument;
import org.obiba.runtime.Version;
import org.springframework.data.annotation.Transient;
Expand All @@ -28,8 +27,6 @@
@Document
public class Configuration extends AbstractAuditableDocument {

private static final long serialVersionUID = -9020464712632680519L;

public static final String DEFAULT_NAME = "Agate";

public static final Locale DEFAULT_LOCALE = Locale.ENGLISH;
Expand All @@ -40,7 +37,6 @@ public class Configuration extends AbstractAuditableDocument {

public static final int DEFAULT_INACTIVE_TIMEOUT = 365 * 24; // 1 year (in hours)

@NotBlank
private String name = DEFAULT_NAME;

private String domain;
Expand Down
2 changes: 0 additions & 2 deletions agate-core/src/main/java/org/obiba/agate/domain/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import com.google.common.base.Strings;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import org.hibernate.validator.constraints.Email;
import org.joda.time.DateTime;
import org.obiba.agate.security.Roles;
import org.obiba.mongodb.domain.AbstractAuditableDocument;
Expand All @@ -41,7 +40,6 @@ public class User extends AbstractAuditableDocument {

private String lastName;

@Email
@Indexed(unique = true)
private String email;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.obiba.agate.web.controller.domain;
package org.obiba.agate.domain;

import org.obiba.agate.domain.User;
import org.owasp.esapi.ESAPI;

import java.util.Map;
Expand Down Expand Up @@ -51,7 +50,7 @@ public String getDisplayName() {
}

public String getPreferredLanguage() {
return user.getPreferredLanguage();
return ESAPI.encoder().encodeForHTML(user.getPreferredLanguage());
}

public boolean getOtpEnabled() {
Expand Down
16 changes: 14 additions & 2 deletions agate-core/src/main/java/org/obiba/agate/service/UserService.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.obiba.agate.repository.UserCredentialsRepository;
import org.obiba.agate.repository.UserRepository;
import org.obiba.agate.service.support.MessageResolverMethod;
import org.obiba.agate.validator.NameValidator;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.BeanUtils;
Expand Down Expand Up @@ -78,6 +79,7 @@ public class UserService {
+ "(?=.*[@#$%^&+=!])" // a special character that must occur at least once
+ "(?=\\S+$).{" + PWD_MINIMUM_LENGTH + "," + PWD_MAXIMUM_LENGTH + "}$");


@Value("${login.otpTimeout:300}")
private int otpTimeout;

Expand Down Expand Up @@ -266,6 +268,10 @@ public void updateUserPassword(@Nonnull User user, @Nonnull String password) {
}

public User createUser(@Nonnull User user, @Nullable String password) {
checkName(user.getName());
checkName(user.getFirstName());
checkName(user.getLastName());

if (Strings.isNullOrEmpty(password)) {
if (user.getRealm() == null) user.setRealm(AgateRealm.AGATE_USER_REALM.getName());
else {
Expand Down Expand Up @@ -455,7 +461,7 @@ public void sendPendingEmail(UserJoinedEvent userJoinedEvent) {
String organization = configurationService.getConfiguration().getName();

Map<String, Object> context = Maps.newHashMap();
context.put("user", user);
context.put("user", new UserProfile(user));

administrators.forEach(u -> sendEmail(u, "[" + organization + "] " + env.getProperty("registration.pendingForReviewSubject"),
"pendingForReviewEmail", context));
Expand All @@ -479,7 +485,7 @@ private void sendEmail(User user, String subject, String templateName, Map<Strin
Locale locale = LocaleUtils.toLocale(user.getPreferredLanguage());
Map<String, Object> ctx = context == null ? Maps.newHashMap() : Maps.newHashMap(context);
if (!ctx.containsKey("user"))
ctx.put("user", user);
ctx.put("user", new UserProfile(user));
ctx.put("organization", configurationService.getConfiguration().getName());
// get user's realm and find if there is a specific agate base url for this realm
Optional<RealmConfig> realmConfig = getRealmConfigs(user).stream().findFirst();
Expand Down Expand Up @@ -711,4 +717,10 @@ private List<RealmConfig> getRealmConfigs(User user) {
return realmConfigRepository.findAll().stream().filter(realmConfig -> user.getRealm().equals(realmConfig.getName())).collect(Collectors.toList());
}

private void checkName(String name) {
if (!NameValidator.isValid(name)) {
throw new BadRequestException("Name contains invalid characters");
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.obiba.agate.validator;

import jakarta.mail.internet.InternetAddress;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static java.util.regex.Pattern.CASE_INSENSITIVE;

public final class EmailValidator {

public static boolean isValid(String email) {
if (email == null || email.trim().isEmpty()) {
return false;
}

// Check length constraints
if (email.length() > 254) {
return false; // Email too long
}

String[] parts = email.split("@");
if (parts[0].length() > 64) {
return false; // Local part too long
}

// Check for common issues
if (email.contains("..") || email.startsWith(".") || email.endsWith(".")) {
return false; // Invalid dot placement
}

try {
InternetAddress emailAddr = new InternetAddress(email);
emailAddr.validate();
return true;
} catch (Exception e) {
return false;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.obiba.agate.validator;

import java.util.regex.Pattern;

public final class NameValidator {

// International pattern supporting accents and other characters
private static final Pattern INTERNATIONAL_NAME_PATTERN = Pattern.compile(
"^[\\p{L}][\\p{L}\\s\\-']{1,49}$");

public static boolean isValid(String name) {
if (name == null || name.isEmpty()) {
return true;
}

return INTERNATIONAL_NAME_PATTERN.matcher(name).matches();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@
import com.google.common.collect.Sets;
import org.apache.shiro.authc.AuthenticationInfo;
import org.apache.shiro.authc.UsernamePasswordToken;
import org.bson.types.ObjectId;
import org.hibernate.validator.internal.constraintvalidators.hv.EmailValidator;
import org.joda.time.DateTime;
import org.obiba.agate.domain.*;
import org.obiba.agate.security.AgateUserRealm;
import org.obiba.agate.security.Roles;
import org.obiba.agate.service.*;
import org.obiba.agate.validator.EmailValidator;
import org.obiba.agate.web.rest.config.JerseyConfiguration;
import org.obiba.agate.web.rest.security.InvalidApplicationKeyException;
import org.obiba.shiro.authc.HttpAuthorizationToken;
Expand Down Expand Up @@ -185,7 +184,7 @@ public Response create(@Context HttpServletRequest request, MultivaluedMap<Strin
if (email == null || Strings.isNullOrEmpty(email.trim())) throw new BadRequestException("Email cannot be empty");
email = email.trim();

if (!new EmailValidator().isValid(email, null)) throw new BadRequestException("Not a valid email address");
if (!EmailValidator.isValid(email)) throw new BadRequestException("Not a valid email address");

if (!config.getJoinWhitelist().isEmpty()) {
if (config.getJoinWhitelist().stream().noneMatch(email::endsWith)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import org.apache.shiro.authz.annotation.RequiresRoles;
import org.hibernate.validator.internal.constraintvalidators.hv.EmailValidator;
import org.obiba.agate.domain.AgateRealm;
import org.obiba.agate.domain.User;
import org.obiba.agate.domain.UserStatus;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import org.obiba.agate.service.ConfigurationService;
import org.obiba.agate.service.RealmConfigService;
import org.obiba.agate.service.UserService;
import org.obiba.agate.web.controller.domain.UserProfile;
import org.obiba.agate.domain.UserProfile;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Component;
Expand All @@ -20,8 +20,6 @@
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

import java.util.stream.Collectors;

@Component
public class SessionInterceptor implements HandlerInterceptor {

Expand Down

0 comments on commit 4e219fe

Please sign in to comment.