gplazma: multimap fix op regression

Motivation:

Commit ef2dc1e introduced a regression in the multimap plugin.  Where
the 'op' principal type is used, logins will fail with dCache logging
a stacktrace like:

    java.lang.RuntimeException: Failed to create principal: java.lang.NoSuchMethodException: org.dcache.auth.OAuthProviderPrincipal.<init>(java.lang.String)
            at org.dcache.gplazma.plugins.GplazmaMultiMapFile$MappablePrincipal.buildPrincipal(GplazmaMultiMapFile.java:148)
            at org.dcache.gplazma.plugins.GplazmaMultiMapFile$MappablePrincipal.buildMatcher(GplazmaMultiMapFile.java:163)
            at org.dcache.gplazma.plugins.GplazmaMultiMapFile.asMatcher(GplazmaMultiMapFile.java:270)
            at org.dcache.gplazma.plugins.GplazmaMultiMapFile.parseMapFile(GplazmaMultiMapFile.java:248)
            at org.dcache.gplazma.plugins.GplazmaMultiMapFile.mapping(GplazmaMultiMapFile.java:216)
            at org.dcache.gplazma.plugins.GplazmaMultiMapPlugin.map(GplazmaMultiMapPlugin.java:37)
            at org.dcache.gplazma.strategies.DefaultMappingStrategy.lambda$map$0(DefaultMappingStrategy.java:57)
            at org.dcache.gplazma.strategies.PAMStyleStrategy.callPlugins(PAMStyleStrategy.java:91)

This problem is because the above commit replaced the single-string
constructor that was being used by the multimap plugin via reflection.

Modification:

Define the following semantics:

When `op` is used as a principal matcher, the matcher will be selected
when the login has an OAuthProviderPrincipal with that value as its
(dCache) name; for example, `op:FOO` will match if the login has an
OAuthProviderPrincipal with name `FOO`.  The URL of the issuer (the
`iss` claim value) is not considered.  This recreates the previous
semantics.

When used as a principal, the `op` takes two colon-sparated arguments:
the (dCache) name for the OP and the issuer URL (the `iss` claim value).
For example, `op:FOO:https://my-op.example.org/` creates an OP with name
`FOO` and issuer URL `https://my-op.example.org/`.

For backwards compatibility, if the second colon and the issuer URL is
omitted then a placeholder URL is used and a warning is logged; for
example, `op:FOO` will add a OAuthProviderPrincipal with name `FOO` and
a placeholder issuer URL.

Unit tests are added that verify correct behaviour.

Result:

A regression is fixed in the multimap plugin when `op:` principal type
is used.

Target: master
Request: 10.1
Request: 10.0
Request: 9.2
Requires-notes: yes
Requires-book: no
Closes: #7654
Patch: https://rb.dcache.org/r/14314/
Acked-by: Tigran Mkrtchyan

modules/gplazma2-multimap/pom.xml

@@ -38,5 +38,10 @@
             <artifactId>jimfs</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>ch.qos.logback</groupId>
+            <artifactId>logback-classic</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>

modules/gplazma2-multimap/src/main/java/org/dcache/gplazma/plugins/GplazmaMultiMapFile.java

@@ -6,6 +6,8 @@
 import com.google.common.collect.ImmutableMap;
 import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributes;
@@ -44,6 +46,8 @@
 
 public class GplazmaMultiMapFile {
 
+    private static final URI PLACEHOLDER_ISSUER = URI.create("https://unknown-issuer.invalid/");
+
     /**
      * Information about the principals that may be mapped.
      */
@@ -116,7 +120,36 @@ public PrincipalMatcher buildMatcher(String value)
         UID("uid", UidPrincipal.class),
         USER_NAME("username", UserNamePrincipal.class),
         ENTITLEMENT("entitlement", EntitlementPrincipal.class),
-        OP("op", OAuthProviderPrincipal.class),
+        OP("op", OAuthProviderPrincipal.class) {
+            @Override
+            public Principal buildPrincipal(String value)
+                    throws GplazmaParseMapFileException {
+                List<String> items = Splitter.on(':').limit(2).splitToList(value);
+                URI issuer;
+                if (items.size() == 1) { // Backwards compatibility
+                    issuer = PLACEHOLDER_ISSUER;
+                    LOGGER.warn("Please replace \"op:{}\" with \"op:{}:URL\""
+                            + " where URL is the issuer URL (the 'iss' claim"
+                            + " value).", value, value);
+                } else {
+                    try {
+                        issuer = new URI(items.get(1));
+                    } catch (URISyntaxException e) {
+                        throw new GplazmaParseMapFileException("Invalid URL: "
+                                + e.getMessage());
+                    }
+                }
+                var name = items.get(0);
+                return new OAuthProviderPrincipal(name, issuer);
+            }
+
+            @Override
+            public PrincipalMatcher buildMatcher(String value)
+                  throws GplazmaParseMapFileException {
+                return (Principal p) -> p instanceof OAuthProviderPrincipal
+                        && ((OAuthProviderPrincipal)p).getName().equals(value);
+            }
+        },
         ROLES("roles", RolePrincipal.class);
 
         private final String label;

modules/gplazma2-multimap/src/test/java/org/dcache/gplazma/plugins/GplazmaMultiMapFileTest.java

@@ -8,6 +8,7 @@
 
 import com.google.common.jimfs.Configuration;
 import com.google.common.jimfs.Jimfs;
+import java.net.URI;
 import java.nio.file.FileSystem;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -22,21 +23,34 @@
 import org.dcache.auth.FQANPrincipal;
 import org.dcache.auth.GidPrincipal;
 import org.dcache.auth.GroupNamePrincipal;
+import org.dcache.auth.OAuthProviderPrincipal;
 import org.dcache.auth.OidcSubjectPrincipal;
 import org.dcache.auth.OpenIdGroupPrincipal;
 import org.dcache.auth.UidPrincipal;
 import org.dcache.auth.UserNamePrincipal;
 import org.globus.gsi.gssapi.jaas.GlobusPrincipal;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import org.junit.Before;
 import org.junit.Test;
-
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import static java.util.Objects.requireNonNull;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.hamcrest.TypeSafeMatcher;
+import org.slf4j.LoggerFactory;
 
 public class GplazmaMultiMapFileTest {
 
     private GplazmaMultiMapFile mapFile;
     private Set<Principal> mappedPrincipals;
     private Path config;
     private List<String> warnings;
+    private List<ILoggingEvent> logEvents;
 
     @Before
     public void setup() throws Exception {
@@ -46,6 +60,13 @@ public void setup() throws Exception {
         mapFile = new GplazmaMultiMapFile(config);
         warnings = new ArrayList<>();
         mapFile.setWarningConsumer(warnings::add);
+
+        // Capture logging activity
+        Logger logger = (Logger) LoggerFactory.getLogger(GplazmaMultiMapFile.class);
+        ListAppender<ILoggingEvent> appender = new ListAppender<>();
+        appender.start();
+        logger.addAppender(appender);
+        logEvents = appender.list;
     }
 
     @Test
@@ -431,6 +452,71 @@ public void shouldMatchNonPrimarySpecificGidWithNonPrimaryGid() throws Exception
         assertThat(mappedPrincipals, hasItem(new UidPrincipal(2000)));
     }
 
+    @Test
+    public void shouldMatchOpWithSameName() throws Exception {
+        givenConfig("op:FOO uid:1000 gid:2000");
+
+        whenMapping(new OAuthProviderPrincipal("FOO", URI.create("https://my-op.example.org/")));
+
+        assertThat(warnings, is(empty()));
+        assertThat(mappedPrincipals, hasItem(new UidPrincipal(1000)));
+        assertThat(mappedPrincipals, hasItem(new GidPrincipal(2000, false)));
+    }
+
+    @Test
+    public void shouldNotMatchOpWithDifferentName() throws Exception {
+        givenConfig("op:FOO uid:1000 gid:2000");
+
+        whenMapping(new OAuthProviderPrincipal("BAR", URI.create("https://my-op.example.org/")));
+
+        assertThat(warnings, is(empty()));
+        assertThat(mappedPrincipals, is(empty()));
+    }
+
+    @Test
+    public void shouldNotMatchOpWithDn() throws Exception {
+        givenConfig("op:FOO uid:1000 gid:2000");
+
+        whenMapping(new GlobusPrincipal("\"dn:/C=DE/S=Hamburg/OU=desy.de/CN=Kermit The Frog\""));
+
+        assertThat(warnings, is(empty()));
+        assertThat(mappedPrincipals, is(empty()));
+    }
+
+
+    @Test
+    public void shouldAddOpWithoutIssuer() throws Exception {
+        givenConfig("email:[email protected]  op:FOO");
+
+        whenMapping(new EmailAddressPrincipal("[email protected]"));
+
+        assertThat(logEvents, hasItem(new LogEventMatcher(Level.WARN,
+                allOf(containsString("op:FOO"),
+                containsString("'iss' claim value")))));
+
+        var opPrincipals = mappedPrincipals.stream()
+                .filter(OAuthProviderPrincipal.class::isInstance)
+                .toList();
+        // Avoid asserting the OP's placeholder issuer URL because we don't
+        // guarantee that value.
+        assertThat(opPrincipals.size(), equalTo(1));
+        var opPrincipal = opPrincipals.get(0);
+        assertThat(opPrincipal.getName(), is(equalTo("FOO")));
+    }
+
+    @Test
+    public void shouldAddOpWithIssuer() throws Exception {
+        givenConfig("email:[email protected]  op:FOO:https://my-op.example.org/");
+
+        whenMapping(new EmailAddressPrincipal("[email protected]"));
+
+        assertThat(warnings, is(empty()));
+        assertThat(logEvents, not(hasItem(new LogEventMatcher(Level.WARN,
+                containsString("op:FOO")))));
+        assertThat(mappedPrincipals, hasItem(new OAuthProviderPrincipal("FOO",
+                URI.create("https://my-op.example.org/"))));
+    }
+
     /*----------------------- Helpers -----------------------------*/
 
     private void givenConfig(String mapping) throws Exception {
@@ -454,4 +540,30 @@ private void whenMapping(Principal principal) throws Exception {
               .findFirst()
               .orElse(Collections.emptySet());
     }
+
+    /**
+     * A simple Hamcrest matcher that allows for assertions against Logback
+     * events.
+     */
+    private static class LogEventMatcher extends TypeSafeMatcher<ILoggingEvent> {
+        private final Level level;
+        private final Matcher<String> messageMatcher;
+
+        public LogEventMatcher(Level level, Matcher<String> messageMatcher) {
+            this.level = requireNonNull(level);
+            this.messageMatcher = requireNonNull(messageMatcher);
+        }
+
+        @Override
+        protected boolean matchesSafely(ILoggingEvent item) {
+            return item.getLevel() == level && messageMatcher.matches(item.getFormattedMessage());
+        }
+
+        @Override
+        public void describeTo(Description description) {
+            description.appendText("a log entry logged at ").appendValue(level)
+                    .appendText(" and with formatted message ")
+                    .appendDescriptionOf(messageMatcher);
+        }
+    }
 }