Skip to content

Commit

Permalink
gplazma: oidc more descriptive offline verification failures
Browse files Browse the repository at this point in the history
Motivation:

OIDC uses caches to record the OIDC discover document and the issuer's
public keys (for offline varification).

Currently, if there is a problem with the discovery document or with the
JWKS document then those problems are logged when the document is
loaded, but subsequent attempts for offline verification of a token fail
with an opaque and cryptic message.

Modification:

Update Result to support additional (Option-like) methods: `map` and
`flatMap`.

Introduce a new MissingNode class (ReasonBearingMissingNode) that can
carry the reason why a JSON node is missing.  Update discovery document
fetching to take advantage of this new class.

Update MemoriseMapWithExpiry to work on Results rather than Maps.  The
class is renamed to reflect this change.

Issuer is updated to work with Result objects rather than Optional
objects.  These Result objects, if representing a failure, describe why
the operation failed.  This is used to generate a more descriptive
AuthenticationException message.

Suppress hamcrest-core from junit as this brings in an ancient
version.

Result:

During offline verification, the details regarding any failure to fetch
documents from the issuer or to parse the issuer-supplied documents are
now used to build the error message when the plugin rejects a token.

Target: master
Requires-notes: yes
Requires-book: no
Closes: dCache#7553
  • Loading branch information
paulmillar committed Jun 2, 2024
1 parent 6a8dc21 commit edd80b2
Show file tree
Hide file tree
Showing 10 changed files with 924 additions and 65 deletions.
45 changes: 44 additions & 1 deletion modules/common/src/main/java/org/dcache/util/Result.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* dCache - http://www.dcache.org/
*
* Copyright (C) 2022 Deutsches Elektronen-Synchrotron
* Copyright (C) 2022-2024 Deutsches Elektronen-Synchrotron
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
Expand Down Expand Up @@ -48,6 +48,49 @@ public abstract class Result<S,F> {
*/
public abstract <C> C map(Function<S,C> mapFromSuccess, Function<F,C> mapFromFailure);

/**
* Convert a successful result to another type. This is equivalent to
* {@link Optional#map(java.util.function.Function)}. If this Result is
* a failure then that failure value is returned, otherwise this Result's
* success value is passed to <tt>mapSuccess</tt> and a new successful
* Result is returned with the value returned by <tt>mapSuccess</tt>.
* <p>
* In effect, this processes a successful result using a procedure that
* cannot fail.
* <p>
* Note: this is really just syntactical sugar over
* {@link #map(java.util.function.Function, java.util.function.Function) },
* but it helps maintain readability.
* @param <U> The new successful type
* @param mapSuccess The function that maps the successful result.
*/
public <U> Result<U,F> map(Function<S,U> mapSuccess) {
return map(s -> Result.success(mapSuccess.apply(s)), Result::failure);
}

/**
* Further process a successful result with the possibility of failure.
* This is equivalent to
* {@link Optional#flatMap(java.util.function.Function)}. If this Result is
* failure then this method returns a Result with that failure.
* If this Result is a success then the Function <tt>processSuccess</tt> is
* applied to this Result's successful result and the value returned by
* <tt>processSuccess</tt> is returned.
* <p>
* In effect, this processes a successful result by a procedure that could
* fail.
* <p>
* Note: this is really just syntactical sugar over
* {@link #map(java.util.function.Function, java.util.function.Function) },
* but it helps maintain readability.
* @param <U> The new successful type
* @param processSuccess The function that processes a successful result
* @return an updated result.
*/
public <U> Result<U,F> flatMap(Function<S,Result<U,F>> processSuccess) {
return map(processSuccess, Result::failure);
}

/**
* Consume this result. The consumers are most likely using side-effects
* (e.g., mutating an object's state or creating a log entry) to propagate
Expand Down
144 changes: 143 additions & 1 deletion modules/common/src/test/java/org/dcache/util/ResultTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* dCache - http://www.dcache.org/
*
* Copyright (C) 2022 Deutsches Elektronen-Synchrotron
* Copyright (C) 2022-2024 Deutsches Elektronen-Synchrotron
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
Expand All @@ -17,10 +17,15 @@
*/
package org.dcache.util;

import static com.github.npathai.hamcrestopt.OptionalMatchers.isEmpty;
import static com.github.npathai.hamcrestopt.OptionalMatchers.isPresentAnd;
import java.util.List;
import org.junit.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class ResultTest
{
Expand All @@ -46,6 +51,19 @@ public void shouldHaveSameHashCodeWhenBothResultsAreFailuresWithSameResult()

// Tests focusing on Result#equals

@Test
public void shouldEqualItself()
{
var r = Result.success("the result");
assertThat(r, equalTo(r));
}

@Test
public void shouldNotEqualAnotherObject()
{
assertThat(Result.success("the result"), not(equalTo(new Object())));
}

@Test
public void shouldEqualSuccessfulResultWithEqualValueWhenSuccessful()
{
Expand Down Expand Up @@ -99,4 +117,128 @@ public void shouldNotEqualSuccessfulResultWhenFailed()

assertThat(r1, is(not(equalTo(r2))));
}

@Test
public void shouldBeSuccessfulWhenSuccess() {
Result<String,String> r = Result.success("success!");

assertTrue(r.isSuccessful());
}

@Test
public void shouldNotBeFailureWhenSuccess() {
Result<String,String> r = Result.success("success!");

assertFalse(r.isFailure());
}

@Test
public void shouldNotBeSuccessfulWhenFailure() {
Result<String,String> r = Result.failure("it went wrong");

assertFalse(r.isSuccessful());
}

@Test
public void shouldBeFailureWhenFailure() {
Result<String,String> r = Result.failure("it went wrong!");

assertTrue(r.isFailure());
}

@Test
public void shouldHaveSuccessResultWhenSuccess() {
Result<String,String> r = Result.success("success!");

assertThat(r.getSuccess(), isPresentAnd(equalTo("success!")));
}

@Test
public void shouldNotHaveFailureResultWhenSuccess() {
Result<String,String> r = Result.success("success!");

assertThat(r.getFailure(), isEmpty());
}

@Test
public void shouldNotHaveSuccessResultWhenFailure() {
Result<String,String> r = Result.failure("it went wrong");

assertThat(r.getSuccess(), isEmpty());
}

@Test
public void shouldHaveFailureResultWhenFailure() {
Result<String,String> r = Result.failure("it went wrong");

assertThat(r.getFailure(), isPresentAnd(equalTo("it went wrong")));
}

@Test
public void shouldNotThrowExceptionWhenSuccess() {
Result<String,String> r = Result.success("success!");

r.orElseThrow(IllegalArgumentException::new);
}

@Test(expected=IllegalArgumentException.class)
public void shouldThrowExceptionWhenFailure() {
Result<String,String> r = Result.failure("it went wrong");

r.orElseThrow(IllegalArgumentException::new);
}

@Test
public void shouldMapSuccess() {
Result<List<String>,String> input = Result.success(List.of("1"));

Result<List<Integer>,String> output = input.map(s -> s.stream()
.map(Integer::valueOf)
.toList());

assertThat(output.getFailure(), isEmpty());
assertThat(output.getSuccess(), isPresentAnd(contains(1)));
}

@Test
public void shouldNotMapFailure() {
Result<List<String>,String> input = Result.failure("Not an integer");

Result<List<Integer>,String> output = input.map(s -> s.stream()
.map(Integer::valueOf)
.toList());

assertThat(output.getSuccess(), isEmpty());
assertThat(output.getFailure(), isPresentAnd(equalTo("Not an integer")));
}

@Test
public void shouldNotFlatMapFailure() {
Result<List<String>,String> input = Result.failure("Not an integer");

Result<List<Integer>,String> output = input.flatMap(s -> Result.success(List.of(1)));

assertThat(output.getSuccess(), isEmpty());
assertThat(output.getFailure(), isPresentAnd(equalTo("Not an integer")));
}

@Test
public void shouldFlatMapToSuccess() {
Result<List<String>,String> input = Result.success(List.of("1"));

Result<List<Integer>,String> output = input.flatMap(s -> Result.success(List.of(1)));

assertThat(output.getFailure(), isEmpty());
assertThat(output.getSuccess(), isPresentAnd(contains(1)));
}

@Test
public void shouldFlatMapToFailure() {
Result<List<String>,String> input = Result.success(List.of("1"));

Result<List<Integer>,String> output = input.flatMap(s -> Result.failure("invalid value"));

assertThat(output.getSuccess(), isEmpty());
assertThat(output.getFailure(), isPresentAnd(equalTo("invalid value")));
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* dCache - http://www.dcache.org/
*
* Copyright (C) 2018 Deutsches Elektronen-Synchrotron
* Copyright (C) 2018-2024 Deutsches Elektronen-Synchrotron
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
Expand Down Expand Up @@ -36,6 +36,7 @@
import java.util.Map;
import java.util.Optional;
import org.apache.http.client.HttpClient;
import org.dcache.gplazma.oidc.helpers.ReasonBearingMissingNode;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -126,7 +127,7 @@ public synchronized JsonNode discoveryDocument() {
nextDiscoveryFetch = now.plus(cacheDurationWhenSuccessful);
} catch (IOException e) {
LOGGER.warn("Failed to fetch discovery document for {}: {}", name, e.toString());
discoveryDocument = MissingNode.getInstance();
discoveryDocument = new ReasonBearingMissingNode(e.toString());
nextDiscoveryFetch = now.plus(CACHE_DURATION_WHEN_UNSUCCESSFUL);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/* dCache - http://www.dcache.org/
*
* Copyright (C) 2024 Deutsches Elektronen-Synchrotron
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package org.dcache.gplazma.oidc.helpers;

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.node.JsonNodeType;
import com.fasterxml.jackson.databind.node.MissingNode;
import com.fasterxml.jackson.databind.node.ValueNode;
import java.io.IOException;
import java.util.Objects;

/**
* This is like Jackson's <tt>MissingNode</tt> but it carries a reason why the
* node is missing. Unfortunately, <tt>MissingNode</tt> is declared final, so
* we cannot simply extend it.
*/
public class ReasonBearingMissingNode extends ValueNode {

private final String reason;

public ReasonBearingMissingNode(String reason) {
this.reason = Objects.requireNonNull(reason);
}

public String getReason() {
return reason;
}

@Override
public final void serialize(JsonGenerator g, SerializerProvider provider)
throws IOException
{
g.writeNull();
}

@Override
public JsonToken asToken() {
return JsonToken.NOT_AVAILABLE;
}

@Override
public int hashCode() {
return reason.hashCode();
}

@Override
public JsonNodeType getNodeType() {
return JsonNodeType.MISSING;
}

@Override
public boolean isMissingNode() {
return true;
}

@Override
public String asText() {
return "";
}

@Override
public String toString() {
return "";
}

@Override
public String toPrettyString() {
return "";
}


@Override
public String asText(String defaultValue) {
return defaultValue;
}

@Override
public boolean equals(Object other) {
if (!(other instanceof ReasonBearingMissingNode)) {
return false;
}

ReasonBearingMissingNode o = (ReasonBearingMissingNode)other;
return reason.equals(o.reason);
}
}
Loading

0 comments on commit edd80b2

Please sign in to comment.