General: Require account activation only where a user can activate their own account (#13541)

Author: krusche

docker/nginx/artemis-nginx.conf

@@ -35,6 +35,10 @@ map $rate_limit_exempt $rate_limit_key {
 
 # Rate limit for the login REST call, at most one request every two seconds
 limit_req_zone $rate_limit_key zone=loginlimit:10m rate=30r/m;
+# Rate limit for the login-options lookup that drives the identifier-first login form. The client calls it once per
+# login attempt, right before POST authenticate, so it gets the same budget as the login zone - but in a zone of its
+# own, so that consuming it does not halve the number of logins a shared campus address can perform.
+limit_req_zone $rate_limit_key zone=loginoptionslimit:10m rate=30r/m;
 # Rate limit for account recovery and registration (password reset init/finish, register).
 # Stricter than the login zone: these endpoints send mail and mutate credentials, and no
 # legitimate client calls them repeatedly.

docker/nginx/artemis-server.conf

@@ -59,6 +59,19 @@ location /api/core/public/authenticate {
    limit_req zone=loginlimit burst=3 delay=2;
 }
 
+# Login options lookup for the identifier-first login form. NOTE: this must match the real endpoint served by
+# PublicAccountResource (@RequestMapping("api/core/public/") + @GetMapping("login-options")). Bounded here as well as by
+# the application-level @LimitRequestsPerMinute bucket, so that a flood is dropped at the edge instead of reaching a
+# database lookup per request. The zone keys on $rate_limit_key, which maps to $binary_remote_addr (the real TCP peer,
+# which the application-level limiter cannot see directly) for every client except an exempted one, which maps to the
+# empty key that nginx does not account - see the geo/map blocks in artemis-nginx.conf.
+location /api/core/public/login-options {
+    proxy_pass http://artemis/api/core/public/login-options;
+    # Same shape as the login block above: the first 2 requests pass immediately, the third waits for a slot, the rest
+    # are answered 429. A real client sends exactly one of these per login attempt.
+    limit_req zone=loginoptionslimit burst=3 delay=2;
+}
+
 # Account recovery and registration. These send mail and mutate credentials, and no legitimate client
 # calls them repeatedly, so they get the stricter zone. These limits key on $binary_remote_addr (the
 # real TCP peer) and so complement the application-level @LimitRequestsPerMinute buckets.

documentation/docs/admin/production-setup/security.mdx

@@ -241,12 +241,16 @@ artemis:
     enabled: true
     account-management-requests-per-minute: 5
     authentication-requests-per-minute: 30
+    login-options-requests-per-minute: 30
 ```
 
 ### Where It Is Enforced
 
 - Registration
+- Account activation
 - Password reset (request reset link + actual password change)
+- Login options lookup (the identifier step of the login form, in its own bucket so that consuming it does not
+  reduce the number of logins a shared address can perform)
 - Username/password login
 - WebAuthn authentication
 - Git over SSH and HTTP operations

documentation/docs/admin/user-registration.mdx

@@ -30,6 +30,7 @@ const sidebars: SidebarsConfig = {
         'user-registration',
         'jenkins-localvc',
         'saml2-login-registration',
+        'oidc-login-registration',
         'troubleshooting',
         'database-tips',
         'known-issues',

documentation/sidebar-admin.ts

@@ -104,6 +104,37 @@ public class User extends AbstractAuditingEntity implements Participant {
     @Column(length = 100)
     private String email;
 
+    /**
+     * Whether this account may authenticate. Every authentication path enforces it: the internal, SAML2, OIDC and passkey
+     * providers, and both git paths (HTTPS via {@code LocalVCServletService} and SSH via {@code GitPublickeyAuthenticatorService}).
+     * <p>
+     * <b>An account is only ever created unactivated when its own owner is expected to activate it</b>, which requires the
+     * account to be <b>internal</b> ({@link #isInternal()}). That is what
+     * {@link de.tum.cit.aet.artemis.account.service.user.UserCreationService#createUser} checks. An externally managed
+     * account authenticates against the external identity provider, so Artemis has no activation step to offer it: the
+     * {@link #activationKey} is redeemable only through {@code GET /activate}, which never sends an external account there.
+     * Creating an external account unactivated therefore produces an account that <em>nothing</em> can ever activate. This
+     * really happened: the student import created LDAP users unactivated, and they lost repository access as soon as git
+     * authentication began enforcing this flag.
+     * <p>
+     * Being internal is necessary but not by itself sufficient for the key to be redeemable: {@code GET /activate} and the
+     * mail carrying the key are both gated behind {@code artemis.user-management.registration.enabled}, so on an instance
+     * with self-registration disabled even an internal account has no way to redeem one. Creation is deliberately
+     * <em>not</em> narrowed to match, because the LTI launch also creates an internal account through the factory and reads
+     * this flag as its own record of whether it still owes the account holder the generated password.
+     * <p>
+     * Only three kinds of writes set this to {@code false}, and only the first is the activation workflow:
+     * <ol>
+     * <li><b>awaiting activation</b> - {@code UserCreationService.createUser} for an internal account, and
+     * {@code UserService.registerUser}, whose accounts are always internal. Paired with a non-null
+     * {@link #activationKey}.</li>
+     * <li><b>deliberate deactivation</b> - {@code UserCreationService.deactivateUser} and the admin edit form. Applies to
+     * any account regardless of type, and never sets an activation key.</li>
+     * <li><b>soft deletion</b> - {@code UserService.anonymizeUser}, alongside {@link #deleted}.</li>
+     * </ol>
+     * The presence of an {@link #activationKey} consequently distinguishes (1) from (2), which is what made it possible to
+     * repair the affected rows without touching accounts an admin had deactivated on purpose.
+     */
     @NonNull
     @Column(nullable = false)
     private boolean activated = false;
@@ -136,6 +167,15 @@ public class User extends AbstractAuditingEntity implements Participant {
     @Column(name = "image_url", length = 256)
     private String imageUrl;
 
+    /**
+     * One-time key a user redeems through {@code GET /activate} to activate their own account. Only ever set on an
+     * <b>internal</b> account, and only together with {@code activated = false} - see {@link #activated} for why an
+     * externally managed account must never be given one, and for how the key's presence tells an account awaiting
+     * activation apart from one an admin deactivated.
+     * <p>
+     * Cleared by every write that activates the account, so the two fields stay consistent: {@code activateUser} for the
+     * activation workflow and the administrative action, and {@code setRandomPasswordAndReturn} for the LTI launch.
+     */
     @Size(max = 20)
     @Column(name = "activation_key", length = 20)
     @JsonIgnore

src/main/java/de/tum/cit/aet/artemis/account/domain/User.java

@@ -19,6 +19,7 @@
 
 import de.tum.cit.aet.artemis.account.config.LdapEnabled;
 import de.tum.cit.aet.artemis.account.domain.User;
+import de.tum.cit.aet.artemis.account.exception.UserNotActivatedException;
 import de.tum.cit.aet.artemis.account.repository.UserRepository;
 import de.tum.cit.aet.artemis.account.service.ldap.LdapUserDto;
 import de.tum.cit.aet.artemis.account.service.ldap.LdapUserService;
@@ -105,8 +106,15 @@ private User authenticateUser(Authentication authentication) throws BadCredentia
 
         // update the user details from ldapUserDto (because they might have changed, e.g. when the user changes the name)
         if (optionalUser.isPresent()) {
-            // TODO: make sure the user is not deactivated in the meantime
-            return saveUserIfNeeded(optionalUser.get(), ldapUserDto);
+            // Checked after the LDAP credentials have been verified, so an unauthenticated caller cannot use the outcome to
+            // learn anything about an account. This provider was the only one that did not consult the flag, which is why an
+            // account left unactivated by the student import could sign in here while being refused everywhere else.
+            User existingUser = optionalUser.get();
+            if (!existingUser.getActivated() || existingUser.isDeleted()) {
+                log.warn("Login attempt for user {} whose account is deactivated or deleted", existingUser.getLogin());
+                throw new UserNotActivatedException("User " + existingUser.getLogin() + " was not activated");
+            }
+            return saveUserIfNeeded(existingUser, ldapUserDto);
         }
         else {
             // this handles the case that the user does not exist in the Artemis database yet (i.e. first time user login)
@@ -127,10 +135,8 @@ private User createUser(LdapUserDto ldapUserDto) {
 
         newUser.setAuthorities(authorityService.buildAuthorities(newUser));
 
-        if (!newUser.getActivated()) {
-            newUser.setActivated(true);
-            newUser.setActivationKey(null);
-        }
+        // No activation handling here: userCreationService.createUser already creates an external user activated, because such a user has
+        // no way to redeem an activation key. This used to re-activate the user to undo what the factory did unconditionally.
         log.info("New LDAP user {} created in Artemis", ldapUserDto.getLogin());
         return userCreationService.saveUser(newUser);
     }

src/main/java/de/tum/cit/aet/artemis/account/security/LdapAuthenticationProvider.java

@@ -13,13 +13,16 @@
 import de.tum.cit.aet.artemis.account.dto.LoginOptionsDTO;
 import de.tum.cit.aet.artemis.account.dto.LoginOptionsDTO.LoginMethod;
 import de.tum.cit.aet.artemis.account.repository.UserRepository;
-import de.tum.cit.aet.artemis.account.service.ldap.LdapUserDto;
-import de.tum.cit.aet.artemis.account.service.ldap.LdapUserService;
 import de.tum.cit.aet.artemis.core.security.SecurityUtils;
 
 /**
- * Service responsible for determining the appropriate login options (such as password, OIDC, or SAML2)
- * for a user based on their identifier (login or email).
+ * Determines which login option (password, OIDC, or SAML2) the login form should offer for a given identifier, so that the
+ * identifier-first form can either show a password field or send the user to the configured identity provider.
+ * <p>
+ * The decision is made from local account state alone. This backs an unauthenticated endpoint, so its answer must be
+ * derivable from what the caller already supplies: it deliberately does not consult the configured directory, which would
+ * both make the response depend on whether the identifier exists there and let an unauthenticated caller drive queries
+ * against it.
  */
 @Profile(PROFILE_CORE)
 @Service
@@ -28,8 +31,6 @@ public class LoginOptionsService {
 
     private final UserRepository userRepository;
 
-    private final Optional<LdapUserService> ldapUserService;
-
     @Value("${artemis.user-management.oidc.enabled:false}")
     private boolean oidcEnabled;
 
@@ -42,13 +43,19 @@ public class LoginOptionsService {
     @Value("${info.saml2.buttonLabel:TUM Login}")
     private String samlDisplayName;
 
-    public LoginOptionsService(UserRepository userRepository, Optional<LdapUserService> ldapUserService) {
+    public LoginOptionsService(UserRepository userRepository) {
         this.userRepository = userRepository;
-        this.ldapUserService = ldapUserService;
     }
 
     /**
      * Determines which login method the user should use based on their identifier (login or email).
+     * <p>
+     * An internal account is the only kind that authenticates against a password stored in Artemis, so it is the only case
+     * answered with the password form. Everything else - an externally managed account, and an identifier this instance has
+     * never seen - is sent to the external provider, which is also where a first-time user gets provisioned. Those two are
+     * answered identically on purpose, so the response does not distinguish a known identifier from an unknown one.
+     * <p>
+     * Falls back to the password form when no external provider is configured, and for a blank identifier.
      *
      * @param emailOrLogin the username or email address entered by the user
      * @return the LoginOptionsDTO containing the determined login method and the display name of the provider
@@ -61,30 +68,17 @@ public LoginOptionsDTO getLoginOptions(String emailOrLogin) {
         boolean isEmail = SecurityUtils.isEmail(sanitizedInput);
         // only project the internal flag instead of loading the whole user entity: empty means the user is not in the database
         Optional<Boolean> internalFlag = isEmail ? userRepository.isInternalUserByEmailIgnoreCase(sanitizedInput) : userRepository.isInternalUserByLogin(sanitizedInput);
-        if (internalFlag.isPresent()) {
-            if (internalFlag.get()) {
-                return new LoginOptionsDTO(LoginMethod.PASSWORD, null);
-            }
-            else {
-                return getExternalUser();
-            }
-        }
-        if (ldapUserService.isPresent()) {
-            Optional<LdapUserDto> ldapUser = isEmail ? ldapUserService.get().findByAnyEmail(sanitizedInput) : ldapUserService.get().findByLogin(sanitizedInput);
-            // if user has a university account
-            if (ldapUser.isPresent()) {
-                return getExternalUser();
-            }
-            // if not: this is an internal user
-            else {
-                return new LoginOptionsDTO(LoginMethod.PASSWORD, null);
-            }
-        }
-        // If user is new and ldap is disabled - provide the SSO authentication option
-        if (oidcEnabled || samlEnabled) {
-            return getExternalUser();
+        // An internal account is the only kind that authenticates against a password stored in Artemis, so it is the only case that
+        // needs the password form. Everything else - an externally managed account, and an identifier this instance has never seen -
+        // is sent to the external provider, which is also where a first-time user gets provisioned.
+        //
+        // The two are answered identically on purpose. This endpoint is unauthenticated, so its answer must be derivable from what the
+        // caller already knows; it deliberately does not consult the configured directory, which would both make the response depend on
+        // whether the identifier exists there and let an unauthenticated caller drive queries against it.
+        if (internalFlag.isPresent() && internalFlag.get()) {
+            return new LoginOptionsDTO(LoginMethod.PASSWORD, null);
         }
-        return new LoginOptionsDTO(LoginMethod.PASSWORD, null);
+        return getExternalUser();
     }
 
     /**