Fix account claims not resolving from account properties and add

required claims

Author: ujibang

accounts/src/main/java/org/restheart/accounts/util/JwtHelper.java

@@ -9,7 +9,10 @@
 import org.restheart.plugins.PluginsRegistry;
 import org.restheart.security.AuthCookie;
 import org.restheart.security.authenticators.MongoRealmAuthenticator;
+import org.restheart.security.tokens.JwtConfigProvider;
 import org.restheart.security.tokens.JwtIssuer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
@@ -43,6 +46,8 @@
  */
 public class JwtHelper {
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(JwtHelper.class);
+
     private final String key;
     private final String issuer;
     private final int ttlMinutes;
@@ -102,9 +107,37 @@ public JwtHelper(String key, String issuer, int ttlMinutes, List<String> account
                 issuer,
                 null,
                 accountPropertiesClaims,
+                resolveRequiredClaims(registry),
                 resolvePasswordPropertyName(registry));
     }
 
+    /**
+     * The claims that must survive any per-request override, read from the deployment's existing
+     * {@code jwtConfigProvider} configuration rather than duplicated under {@code accountsConfig}:
+     * they are a property of the JWT this deployment issues, so every issuer must apply the same
+     * ones. Without this, a tenant supplying its own claim list on the {@code /auth/*} path could
+     * drop a claim later verified on every request and lock itself out.
+     */
+    private static List<String> resolveRequiredClaims(PluginsRegistry registry) {
+        if (registry == null) {
+            return null;
+        }
+
+        try {
+            for (var pr : registry.getProviders()) {
+                if ("jwtConfigProvider".equals(pr.getName()) && pr.isEnabled()
+                        && pr.getInstance() instanceof JwtConfigProvider jcp
+                        && jcp.get(pr) instanceof JwtConfigProvider.JwtConfig cfg) {
+                    return cfg.requiredAccountPropertiesClaims();
+                }
+            }
+        } catch (Exception e) {
+            LOGGER.debug("Could not resolve jwtConfigProvider required-account-properties-claims", e);
+        }
+
+        return null;
+    }
+
     /** Risolve il nome della proprietà password da {@code mongoRealmAuthenticator}, se disponibile. */
     private static String resolvePasswordPropertyName(PluginsRegistry registry) {
         if (registry == null) {

security/src/main/java/org/restheart/security/services/OAuthAuthorizationService.java

@@ -148,7 +148,8 @@ private JwtIssuer issuer() {
                 if (local == null) {
                     var algo = buildAlgorithm(jwtConfig);
                     local = new JwtIssuer(algo, jwtConfig.issuer(), jwtConfig.audience(),
-                            jwtConfig.accountPropertiesClaims(), resolvePasswordPropertyName());
+                            jwtConfig.accountPropertiesClaims(),
+                            jwtConfig.requiredAccountPropertiesClaims(), resolvePasswordPropertyName());
                     this.jwtIssuer = local;
                 }
             }

security/src/main/java/org/restheart/security/tokens/JwtConfigProvider.java

@@ -129,11 +129,12 @@ public void init() throws ConfigurationException {
         // Account properties copied into issued tokens. Shared here so that every issuer —
         // jwtTokenManager on /token, restheart-accounts at login — puts the same claims in the
         // token: they are the same JWT, issued at different moments.
-        var accountPropertiesClaims = accountPropertiesClaims(config);
+        var accountPropertiesClaims = claimList(config, "account-properties-claims");
+        var requiredAccountPropertiesClaims = claimList(config, "required-account-properties-claims");
 
         this.jwtConfig = new JwtConfig(key, algorithm, issuer,
                 audience.isEmpty() ? null : audience.toArray(String[]::new),
-                accountPropertiesClaims);
+                accountPropertiesClaims, requiredAccountPropertiesClaims);
 
         BootstrapLogger.info(LOGGER, "Algorithm: {}, Issuer: {}, Audience: {}",
                 algorithm, issuer, audience.isEmpty() ? "null" : String.join(", ", audience));
@@ -158,8 +159,8 @@ public JwtConfig get(PluginRecord<?> caller) {
      * @return the configured names, or {@code null} when not set here — issuers then fall back to
      *         their own deprecated per-plugin setting
      */
-    private List<String> accountPropertiesClaims(Map<String, Object> config) throws ConfigurationException {
-        var configured = argOrDefault(config, "account-properties-claims", null);
+    private List<String> claimList(Map<String, Object> config, String key) throws ConfigurationException {
+        var configured = argOrDefault(config, key, null);
 
         return switch (configured) {
             case null -> null;
@@ -169,7 +170,7 @@ private List<String> accountPropertiesClaims(Map<String, Object> config) throws
                     .map(e -> (String) e)
                     .toList();
             default -> throw new ConfigurationException(
-                    "Wrong account-properties-claims, must be a String or an Array of Strings");
+                    "Wrong " + key + ", must be a String or an Array of Strings");
         };
     }
 
@@ -192,9 +193,14 @@ private String generateSecureRandomKey() {
      *        same reason {@code key} and {@code issuer} are: a JWT issued by this deployment is one
      *        thing regardless of which component issues it. {@code null} means "not set here",
      *        in which case issuers fall back to their own deprecated setting.
+     * @param requiredAccountPropertiesClaims account properties always copied into issued tokens,
+     *        even when a per-request override supplies its own list. For claims the deployment
+     *        cannot work without — on a multi-tenant node, the claim naming the issuing node is
+     *        verified on every later request, so a tenant able to drop it would lock itself out.
      */
     public record JwtConfig(String key, String algorithm, String issuer, String[] audience,
-                            List<String> accountPropertiesClaims) {
+                            List<String> accountPropertiesClaims,
+                            List<String> requiredAccountPropertiesClaims) {
         public boolean hasAudience() {
             return audience != null && audience.length > 0;
         }

security/src/main/java/org/restheart/security/tokens/JwtIssuer.java

@@ -29,7 +29,6 @@
 import java.util.Set;
 
 import org.bson.BsonString;
-import org.restheart.configuration.Utils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -110,6 +109,7 @@ public static List<String> claimsOverride(org.restheart.exchange.Request<?> requ
     private final String issuer;
     private final String[] audience;
     private final List<String> defaultClaims;
+    private final Set<String> requiredClaims;
     private final Set<String> denylist;
 
     /**
@@ -124,10 +124,21 @@ public static List<String> claimsOverride(org.restheart.exchange.Request<?> requ
      */
     public JwtIssuer(Algorithm algo, String issuer, String[] audience,
                      List<String> defaultClaims, String passwordProperty) {
+        this(algo, issuer, audience, defaultClaims, null, passwordProperty);
+    }
+
+    /**
+     * @param requiredClaims account properties always copied into the token, whatever the effective
+     *                       claim list — see {@link #accountClaims(Map, List)}; {@code null} means
+     *                       none
+     */
+    public JwtIssuer(Algorithm algo, String issuer, String[] audience,
+                     List<String> defaultClaims, List<String> requiredClaims, String passwordProperty) {
         this.algo = algo;
         this.issuer = issuer;
         this.audience = audience;
         this.defaultClaims = defaultClaims;
+        this.requiredClaims = requiredClaims == null ? Set.of() : Set.copyOf(requiredClaims);
 
         var pwd = passwordProperty == null || passwordProperty.isBlank()
                 ? DEFAULT_PASSWORD_PROPERTY
@@ -159,9 +170,23 @@ public boolean isDenylisted(String claim) {
     public Map<String, Object> accountClaims(Map<String, ? super Object> properties, List<String> claimsOverride) {
         final var ret = new HashMap<String, Object>();
 
-        var claims = claimsOverride != null ? claimsOverride : defaultClaims;
+        if (properties == null) {
+            return ret;
+        }
+
+        var configured = claimsOverride != null ? claimsOverride : defaultClaims;
 
-        if (claims == null || properties == null) {
+        // Required claims are added whatever the effective list says. They are infrastructure the
+        // deployment depends on — e.g. on a multi-tenant node a claim naming the node that issued
+        // the token, checked on every subsequent request — so a tenant supplying its own list must
+        // not be able to drop them and lock itself out. The denylist still wins over this.
+        var claims = new java.util.LinkedHashSet<String>();
+        claims.addAll(requiredClaims);
+        if (configured != null) {
+            claims.addAll(configured);
+        }
+
+        if (claims.isEmpty()) {
             return ret;
         }
 
@@ -177,7 +202,7 @@ public Map<String, Object> accountClaims(Map<String, ? super Object> properties,
                 continue;
             }
 
-            var value = Utils.find(properties, path, true);
+            var value = valueAt(properties, keys);
 
             if (value != null) {
                 addClaim(ret, keys, value);
@@ -302,6 +327,36 @@ public Builder withClaim(final Builder b, final String k, final Object v) {
         };
     }
 
+    /**
+     * Navigates {@code properties} following {@code keys}, returning the value found or
+     * {@code null}.
+     *
+     * <p>Deliberately a plain map walk rather than {@code Utils.find}'s JXPath: that helper is
+     * meant for the YAML configuration tree, and it resolves against account properties only for
+     * some account types — {@code FileRealmAccount} hands back the map it parsed, whereas
+     * {@code MongoRealmAccount} rebuilds it through GSON. Worse, it swallows every failure
+     * ({@code catch (Throwable)} with {@code silent = true}), so a claim silently vanished from
+     * the token with nothing in the logs. A map lookup has none of those problems and is what
+     * this actually needs.
+     */
+    static Object valueAt(Map<String, ? super Object> properties, String[] keys) {
+        Object current = properties;
+
+        for (var key : keys) {
+            if (!(current instanceof Map<?, ?> map)) {
+                return null;
+            }
+
+            current = map.get(key);
+
+            if (current == null) {
+                return null;
+            }
+        }
+
+        return current;
+    }
+
     /** Splits {@code a/b/c} into its keys, dropping empty segments. */
     static String[] keysFromPath(final String path) {
         var ret = path.contains("/") ? path.split("/") : new String[]{path};

security/src/main/java/org/restheart/security/tokens/JwtTokenManager.java

@@ -191,7 +191,8 @@ JwtIssuer issuer() {
             synchronized (this) {
                 local = this.issuerImpl;
                 if (local == null) {
-                    local = new JwtIssuer(algo, issuer, audience, accountPropertiesClaims, passwordProperty());
+                    local = new JwtIssuer(algo, issuer, audience, accountPropertiesClaims,
+                            jwtConfig.requiredAccountPropertiesClaims(), passwordProperty());
                     this.issuerImpl = local;
                 }
             }