Skip to content

Commit

Permalink
gplazma: multimap fix op regression
Browse files Browse the repository at this point in the history
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
  • Loading branch information
paulmillar authored and lemora committed Sep 10, 2024
1 parent 5708994 commit 729ce8f
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 2 deletions.
5 changes: 5 additions & 0 deletions modules/gplazma2-multimap/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}
}
}

0 comments on commit 729ce8f

Please sign in to comment.