[undertow] add functions to apply to every marked params in the runtime#255
Conversation
rubenfiszel
changed the title
|
@uschi2000 WC does not support safe body or header parameters. For header parameters it uses a whitelist instead. |
|
re header & body: what does Conjure support? |
|
@uschi2000 The conjure spec only mentions that they are represented by types in the IR without any requirement for supporting particular ones. I could not find a spec for conjure-java. There is a stronger argument imho which is the one of consistency: All params should be treated the same, as Argument of the endpoints, regardless of their actual wire reification. In that regard, I would agree that we would need to support Header and Body Param (although here we are storing safe and unsafe params, I am not sure we actually want to store heavy body params like binary stream as string attachment key of the exchange). Nevertheless, my opinion is that it is an under-specified hack that we implement currently to unblock ourselves but without any requirement to do more than what is necessary. |
|
I think filtering on “Safe” (case-insensitive) is my preferred option.
|
|
Let’s add header parameters, but not body ones. Could you document the behavior, please?
|
|
@uschi2000 Latest commit now only use 2 multimaps, one for safe and one for unsafe params. It supports header params and some docs has been added. It now filters on "Safe" name. |
uschi2000
left a comment
uschi2000
left a comment
There was a problem hiding this comment.
looks good modulo a few nitpicks
|
I’d like to take a quick pass over this PR this evening if you don’t mind tainting a bit!
…-ck
On Feb 28, 2019, at 10:24 AM, Ruben Fiszel ***@***.***> wrote:
@rubenfiszel commented on this pull request.
In conjure-undertow-lib/src/main/java/com/palantir/conjure/java/undertow/lib/Parameters.java:
> + public static final AttachmentKey<Multimap<String, Object>> SAFE_PARAMS_ATTACH_KEY =
+ AttachmentKey.create(Multimap.class);
+ public static final AttachmentKey<Multimap<String, Object>> UNSAFE_PARAMS_ATTACH_KEY =
+ AttachmentKey.create(Multimap.class);
+
+ private Parameters() {}
+
+ public static void putSafePathParam(HttpServerExchange exchange, String key, Object value) {
+ putParam(exchange, SAFE_PARAMS_ATTACH_KEY, key, value);
+ }
+
+ public static void putUnsafePathParam(HttpServerExchange exchange, String key, Object value) {
+ putParam(exchange, UNSAFE_PARAMS_ATTACH_KEY, key, value);
+ }
+
+ public static void putSafeQueryParam(HttpServerExchange exchange, String key, Object value) {
This is the last point to resolve. Do you feel strongly about this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@cakofony ofc! |
rubenfiszel
changed the title
|
|
||
| /** Provides the {@link MarkedParam} used to store safe parameters (header, query, body params) for further | ||
| * processing. (e.g logging) */ | ||
| MarkedParam markedParam(); |
There was a problem hiding this comment.
That makes sense, though I think it may be cleaner to leave mark as is since marking other types of data would use a different method signature. I don't have a strong opinion, markParam is also reasonable.
|
I like the simplicity of this approach. It doesn't force special-case code specifically for safe/unsafe parameters and maintains the same level of separation we've had in the jersey implementation. |
|
I'm happy with this. Any thoughts before it merges @iamdanfox @uschi2000? |
|
lgtm
|
…ices/JerseyServiceGenerator.java Co-Authored-By: rubenfiszel <rubenfiszel@gmail.com>
| ResourceIdentifier datasetRid = | ||
| runtime.plainSerDe().deserializeRid(pathParams.get("datasetRid")); | ||
| runtime.markers() | ||
| .param("com.palantir.redaction.Safe", "datasetRid", datasetRid, exchange); |
There was a problem hiding this comment.
👍 this is really nice, really like how it doesn't back us into a corner in terms of future development :)
…java into rf/undertowSafeTags
5 participants
Before this PR
It is not possible to do any actions on marked params.
After this PR
The runtime include functions that will be called on every marked params. This allow the possibility to to add safe logging among others.