-
Notifications
You must be signed in to change notification settings - Fork 81
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 <[email protected]>
style should match property definition in issue #383 removed deprecated incomingHttpLinkSettings and outgoingHttpLinkSettings accessors Signed-off-by: nhartner <[email protected]>
* 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 <[email protected]> * 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 <[email protected]> * guava android compatibility fix Signed-off-by: nhartner <[email protected]> * remove unsed SharedSecretTokenSettings Signed-off-by: nhartner <[email protected]> * 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 <[email protected]> * disable docker container logs Signed-off-by: nhartner <[email protected]> * remove fallback logic to deprecated property that no longer exists Signed-off-by: nhartner <[email protected]> * 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 <[email protected]> * fix typo Signed-off-by: nhartner <[email protected]> * fix javadoc to include auth_type as a required property Signed-off-by: nhartner <[email protected]> * remove deprecated fallback that has been removed Signed-off-by: nhartner <[email protected]> * Only require an outgoing url for IlpOverHttpLink instead of link settings Signed-off-by: nhartner <[email protected]> * nuke deprecated constructor Signed-off-by: nhartner <[email protected]> * fix typo Signed-off-by: nhartner <[email protected]> * make toCustomSettingsMap auxiliary so it doesn't get include in toString/hashCode etc Signed-off-by: nhartner <[email protected]> * 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 <[email protected]>
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 <[email protected]> * 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 <[email protected]> * guava android compatibility fix Signed-off-by: nhartner <[email protected]> * remove unsed SharedSecretTokenSettings Signed-off-by: nhartner <[email protected]> * 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 <[email protected]> * disable docker container logs Signed-off-by: nhartner <[email protected]> * remove fallback logic to deprecated property that no longer exists Signed-off-by: nhartner <[email protected]> * 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 <[email protected]> * fix typo Signed-off-by: nhartner <[email protected]> * fix javadoc to include auth_type as a required property Signed-off-by: nhartner <[email protected]> * remove deprecated fallback that has been removed Signed-off-by: nhartner <[email protected]> * Only require an outgoing url for IlpOverHttpLink instead of link settings Signed-off-by: nhartner <[email protected]> * nuke deprecated constructor Signed-off-by: nhartner <[email protected]> * fix typo Signed-off-by: nhartner <[email protected]> * make toCustomSettingsMap auxiliary so it doesn't get include in toString/hashCode etc Signed-off-by: nhartner <[email protected]> * 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 <[email protected]> Signed-off-by: nkramer44 <[email protected]>
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: