quilt icon indicating copy to clipboard operation
quilt copied to clipboard

Align IlpOverHttp Settings with CustomProperties

Open sappenin opened this issue 6 years ago • 5 comments

In the current implementation, it's possible that an IlpOverHttpLinkSettings object will have differing values in its map of custom settings as compared to the Java code. For example, a test-case might load an instance of IlpOverHttpLinkSettings by calling IlpOverHttpLinkSettings.fromCustomSettings but then manually set the value of the auth-subject, for example.

We should tighten this up, perhaps in the Immutable, such that an instance of IlpOverHttpLinkSettings that has values set in Java uses those instead of any divergent value in the CustomSettings Map.

This gets weird though when the two values differ -- which one should take precedence? If we say Java always takes precendence, then what if Java is unset, but the Map is set. Perhaps we should look into the Immutables normalization method here to work this out.

sappenin avatar Nov 27 '19 20:11 sappenin

Another thing that shouldn't be possible this is:

IncomingLinkSettings.builder()
            .authType(IlpOverHttpLinkSettings.AuthType.SIMPLE)
            .tokenIssuer(HttpUrl.parse("https://incoming-issuer.example.com"))
            .tokenAudience(HttpUrl.parse("https://incoming-audience.example.com/"))
            .encryptedTokenSharedSecret(SHH)
            .minMessageWindow(Duration.ofMillis(30))
            .build();

 assertThat(incomingLinksettings.tokenIssuer().get())
        .isEqualTo(HttpUrl.parse("https://incoming-issuer.example.com/"));
    assertThat(incomingLinksettings.tokenAudience().get())
        .isEqualTo(HttpUrl.parse("https://incoming-audience.example.com/"));

There should maybe be a normalization check in the Immutable to prevent this from happening.

sappenin avatar Nov 27 '19 23:11 sappenin

@nhartner can you add examples of our new custom-property scheme for the simple & JWT auth properties?

sappenin avatar Dec 18 '19 18:12 sappenin

New property scheme:

Simple:

"ilpOverHttp.incoming.simple.auth_token": "password",
"ilpOverHttp.outgoing.simple.auth_token": "password",
"ilpOverHttp.outgoing.url": "http://java1.connector.com/accounts/foo/ilp"

JWT HS256

"ilpOverHttp.incoming.jwt.shared_secret": "adsfasdfads",
"ilpOverHttp.incoming.jwt.subject": "foo@me",
"ilpOverHttp.outgoing.jwt.shared_secret": "adsfasdfads",
"ilpOverHttp.outgoing.jwt.subject": "foo@them",
"ilpOverHttp.outgoing.jwt.expiryDuration":  "pt30m", // 30 minutes
"ilpOverHttp.outgoing.url": "http://java1.connector.com/accounts/foo/ilp"

JWT RS256

"ilpOverHttp.incoming.jwt.issuer": "http://them.example.com",
"ilpOverHttp.incoming.jwt.audience": "https://me.example.com",
"ilpOverHttp.incoming.jwt.subject": "foo@me",
"ilpOverHttp.outgoing.jwt.issuer": "http://me.example.com",
"ilpOverHttp.outgoing.jwt.audience": "https://me.example.com",
"ilpOverHttp.outgoing.jwt.subject": "foo@me",
"ilpOverHttp.outgoing.jwt.expiryDuration": "pt500s", // 500 seconds
"ilpOverHttp.outgoing.url": "http://java1.connector.com/accounts/foo/ilp"

nhartner avatar Dec 20 '19 18:12 nhartner

@nhartner This is complete now since #408 was merged? If so let's close it.

sappenin avatar Jan 06 '20 23:01 sappenin

After speaking more with @nhartner, we think the best solution here is to decouple the API object (e.g., the thing with customSettings) from the internal business object, which is strongly typed and has no customSettings map). This will avoid any ambiguity in code.

As background, there generally shouldn't be a case where a LinkSettings object is being created by a developer who is setting both customSettings and setting Java properties in the immutables builder directly. The reason being is that customSettings really only exists to placate the REST API, whereas we expect a Java developer constructing links to not use custom settings.

This falls down in two places. The first is the ITs, many of which were written in a "customSettings" world (i.e., a world where the Java Immutables API had not settled down yet), so we see examples where ITs construct Links using both customSettings and overt Java builder calls.

The second place this falls down is in the LinkFactory, where it accepts a LinkSettings object, and then overrides any builder properties with custom settings from the Map.

To make it clearer for everyone, we can have two objects - one for the REST API, and one for the internal business logic, that more clearly defines what's involved and allowed for each object.

sappenin avatar Jan 07 '20 01:01 sappenin