Align IlpOverHttp Settings with CustomProperties #383
Comments
Another thing that shouldn't be possible this is:
There should maybe be a normalization check in the Immutable to prevent this from happening. |
@nhartner can you add examples of our new custom-property scheme for the simple & JWT auth properties? |
New property scheme: Simple:
JWT HS256
JWT RS256
|
style should match property definition in issue #383 removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors Signed-off-by: nhartner <nhartner@gmail.com>
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> * 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>
After speaking more with @nhartner, we think the best solution here is to decouple the API object (e.g., the thing with As background, there generally shouldn't be a case where a LinkSettings object is being created by a developer who is setting both 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 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. |
* 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>
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 callingIlpOverHttpLinkSettings.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.
The text was updated successfully, but these errors were encountered: