Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

Align IlpOverHttp Settings with CustomProperties #383

Open
sappenin opened this issue Nov 27, 2019 · 5 comments
Open

Align IlpOverHttp Settings with CustomProperties #383

sappenin opened this issue Nov 27, 2019 · 5 comments

Comments

@sappenin
Copy link
Contributor

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 sappenin added ilp-link v1.0.2 v1.0.3 Issues slated for the 1.0.3 release v1.0 and removed v1.0.2 labels Nov 27, 2019
@sappenin
Copy link
Contributor Author

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.

@nhartner nhartner self-assigned this Dec 4, 2019
@sappenin
Copy link
Contributor Author

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

@nhartner
Copy link
Contributor

nhartner commented Dec 20, 2019

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 added a commit that referenced this issue Dec 23, 2019
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>
nhartner added a commit that referenced this issue Dec 23, 2019
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>
nhartner added a commit that referenced this issue Jan 2, 2020
* align Ilp-over-http custom settings to match update property style
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>

* align Ilp-over-http custom settings to match update property style
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>

* guava android compatibility fix

Signed-off-by: nhartner <nhartner@gmail.com>

* remove unsed SharedSecretTokenSettings

Signed-off-by: nhartner <nhartner@gmail.com>

* bringing back AuthType on settings.
It was getting difficult to provide meaning error messages for missing fields without knowing the intention of what type of settings were being provided

Signed-off-by: nhartner <nhartner@gmail.com>

* disable docker container logs

Signed-off-by: nhartner <nhartner@gmail.com>

* remove fallback logic to deprecated property that no longer exists

Signed-off-by: nhartner <nhartner@gmail.com>

* make jwt.token_audience a string instead of url because the JWT RFC
does not strictly require it to be a url and in the case of auth0, it's not a url for web flows

Signed-off-by: nhartner <nhartner@gmail.com>

* fix typo

Signed-off-by: nhartner <nhartner@gmail.com>

* fix javadoc to include auth_type as a required property

Signed-off-by: nhartner <nhartner@gmail.com>

* remove deprecated fallback that has been removed

Signed-off-by: nhartner <nhartner@gmail.com>

* Only require an outgoing url for IlpOverHttpLink instead of link settings

Signed-off-by: nhartner <nhartner@gmail.com>

* nuke deprecated constructor

Signed-off-by: nhartner <nhartner@gmail.com>

* fix typo

Signed-off-by: nhartner <nhartner@gmail.com>

* make toCustomSettingsMap auxiliary so it doesn't get include in toString/hashCode etc

Signed-off-by: nhartner <nhartner@gmail.com>

* fix testConnection to not use linkSettings and just use outgoingUrl
don't need to log authType either since it's not used

Signed-off-by: nhartner <nhartner@gmail.com>
@sappenin sappenin added enhancement v1.1 and removed v1.0 v1.0.3 Issues slated for the 1.0.3 release labels Jan 6, 2020
@sappenin
Copy link
Contributor Author

sappenin commented Jan 6, 2020

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

@sappenin
Copy link
Contributor Author

sappenin commented Jan 7, 2020

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.

nkramer44 pushed a commit that referenced this issue Jan 28, 2020
* align Ilp-over-http custom settings to match update property style
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>

* align Ilp-over-http custom settings to match update property style
style should match property definition in issue #383
removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors

Signed-off-by: nhartner <nhartner@gmail.com>

* guava android compatibility fix

Signed-off-by: nhartner <nhartner@gmail.com>

* remove unsed SharedSecretTokenSettings

Signed-off-by: nhartner <nhartner@gmail.com>

* bringing back AuthType on settings.
It was getting difficult to provide meaning error messages for missing fields without knowing the intention of what type of settings were being provided

Signed-off-by: nhartner <nhartner@gmail.com>

* disable docker container logs

Signed-off-by: nhartner <nhartner@gmail.com>

* remove fallback logic to deprecated property that no longer exists

Signed-off-by: nhartner <nhartner@gmail.com>

* make jwt.token_audience a string instead of url because the JWT RFC
does not strictly require it to be a url and in the case of auth0, it's not a url for web flows

Signed-off-by: nhartner <nhartner@gmail.com>

* fix typo

Signed-off-by: nhartner <nhartner@gmail.com>

* fix javadoc to include auth_type as a required property

Signed-off-by: nhartner <nhartner@gmail.com>

* remove deprecated fallback that has been removed

Signed-off-by: nhartner <nhartner@gmail.com>

* Only require an outgoing url for IlpOverHttpLink instead of link settings

Signed-off-by: nhartner <nhartner@gmail.com>

* nuke deprecated constructor

Signed-off-by: nhartner <nhartner@gmail.com>

* fix typo

Signed-off-by: nhartner <nhartner@gmail.com>

* make toCustomSettingsMap auxiliary so it doesn't get include in toString/hashCode etc

Signed-off-by: nhartner <nhartner@gmail.com>

* fix testConnection to not use linkSettings and just use outgoingUrl
don't need to log authType either since it's not used

Signed-off-by: nhartner <nhartner@gmail.com>
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>
@sappenin sappenin removed the v1.1 label Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants