NOAA-OWP / wres

Code and scripts for the Water Resources Evaluation Service
Other
2 stars 1 forks source link

As a developer, I want to specify the project, WRES, in all WRDS service calls #282

Open epag opened 4 weeks ago

epag commented 4 weeks ago

Author Name: Hank (Hank) Original Redmine Issue: 97763, https://vlab.noaa.gov/redmine/issues/97763 Original Date: 2021-10-21


From Swagger, the field is @proj@. For example,

https://nwcal-wrds.[host]/docs/location/v3.0/swagger/

From that documentation,

The project name that is accessing the API. Use _ in place of spaces. Using this parameter is necessary in order to streamline debugging any issues.

Examples: proj=WRES

This needs to be added to +all+ calls to WRDS. It will become a required field, per MarkW.

When development begins, a checklist should be prepared with all the WRDS service interactions.

For now, the priority is high, but it will likely be upped to urgent in the near future. Thanks,

Hank


Redmine related issue(s): 100164


epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T17:37:01Z


I did not comment during the meeting about this anti-feature of WRDS because it was neither here nor there. We could safely ignore it. But if it is going to become a breaking change, then I guess now is the time to push back on it.

The WRDS team already has the IP addresses of the caller. There is also already an HTTP header called @User-Agent@ that can be used for this purpose. Both are already available to WRDS. They can use them today. Whether every team on the client side is actually identifying their applications using the @User-Agent@ header is another question.

By attempting to require this parameter, the WRDS team can expect what Chris described in the meeting: either garbage or fraudulent values, not out of malice but out of an effort to get it to work, e.g. by people attempting to get work done.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2021-10-21T17:44:46Z


Jesse,

The WRDS team already has the IP addresses of the caller. There is also already an HTTP header called User-Agent that can be used for this purpose. Both are already available to WRDS. They can use them today. Whether every team on the client side is actually identifying their applications using the User-Agent header is another question.

Do we identify our applications using the User-Agent header? Just curiosity.

I've never been a big fan of this feature as it felt ripe for abuse, either accidental or purposeful (and not necessarily by the user). I may just be paranoid. :)

Hank

epag commented 4 weeks ago

Original Redmine Comment Author Name: Chris (Chris) Original Date: 2021-10-21T18:12:50Z


Now, I'm not saying I do this, but it's currently possible to hit the service without even a cert (lol), so I don't see a lot of virtue from a user's perspective to stick anything but garbage in there.

I touched on this in the chat, but if they really have to have a tight understanding as to who is doing what, that's what API keys are for. I'd say that requiring an API key is the right way to do the wrong thing, so doing things in their proposed way is essentially the wrong way to do the wrong thing. This might make me a pedant, but you really shouldn't be able to modify data on a server with a @get@ request.

So, let's say someone enters @proj=YoMomma@. What sort of trouble would they be in? Asking for a friend.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T18:30:07Z


Right now it's something like @Java/11.0.12/OkHttp-something@, in other words "obviously WRES" but I'm going to enable debug logging on OkHttp and see the exact string. We should append a WRES at least, and a version id if that's easy enough.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T19:07:13Z


So it was much more difficult than I hoped to enable logging of the actual headers (not just added headers as we do currently in @WebClient@), went a different route and had WRES connect to a local nc server:

$ nc -l 5000
GET /test?endDT=2018-01-01T00%3A00%3A00Z&format=json&parameterCd=00060&sites=09165000&startDT=2017-01-01T00%3A00%3A01Z HTTP/1.1
Accept-Encoding: gzip
Host: localhost:5000
Connection: Keep-Alive
User-Agent: okhttp/4.10.0-RC1

So there's your answer: @okhttp/4.10.0-RC1@ but we can modify it to something else, which I think would be a fair resolution to this ticket.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T19:08:59Z


Without objection, setting it to @User-Agent: YoMomma@

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2021-10-21T19:11:06Z


Is it "YoMomma" or "YoMamma"? We should make sure of the spelling, first. :)

We'll have to get the WRDS team to buy in on doing something different. I'll bring it up at the next WRES/WRDS meeting in a couple weeks. If they require it, they require it, and we'll have no option but to play along. But if we can talk them into a different approach, "the right way to do the wrong thing" (what Jesse found or keys), or, better still, "just don't do the wrong thing", that would be an improvement.

Sounds like my suspicions, that this is a bad idea, were well founded even if I didn't have confidence in my opinion. Thanks,

Hank

epag commented 4 weeks ago

Original Redmine Comment Author Name: James (James) Original Date: 2021-10-21T19:12:52Z


I thought it was jo mamma.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Chris (Chris) Original Date: 2021-10-21T19:24:12Z


I've seen yo mama. Laugh Factory labels it as "Yo Momma", so there's that. We can always just have:

@User-Agent: TeaIsJustBrownDirtWater@

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T21:59:17Z


commit:626647401cf59f11e32f2093a658016191a22454 sets it to wres-io/version, e.g. @User-Agent: wres-io/20211021-67c095a-dev@ from my machine. This actually might be helpful to see whether it is a clean build of wres-io (no @-dev@ dirty flag) versus a dirty build of wres-io (@-dev@ dirty flag).

As far as the parameter for the project, I believe our users have the ability to add the flag with existing mechanisms, do they not? Only in dev/test would we need to say "this is from the WRES project" right?

It's a bad-faith request, smells bad, looks bad... etc.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T22:00:33Z


In other words I acknowledge there is a difference between identifying the @User-Agent@ (a piece of software) and identifying the project that is associated with the call. The latter is a runtime thing, the former is a compile-time thing.

epag commented 4 weeks ago

Original Redmine Comment Author Name: Jesse (Jesse) Original Date: 2021-10-21T22:02:21Z


Oh, and before I forget, this only affects requests that go through @WebClient@, so it wouldn't affect the requests that use the @cdm@ aka @netcdf-java@ library (those use something else under the hood, apache httpclient or otherwise).

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2022-04-06T19:36:32Z


Looked briefly at a solution for the WRDS observed service. For that, I plan is to edit @WebSource.java@ to include the project field automatically when it builds WRDS URIs if the user does not already include it. That should impact the WRDS AHPS forecast and observation services. However, the NWM service appears to be handled elsewhere. I'll see if I can find it.

Hank

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2022-04-06T19:44:09Z


Nevermind. The NWM WRDS services are handled in @WebSource.java@. So, to implement proj, we should be able to make the changes in @WebSource.java@ for all WRDS time series services.

I'll take a look at the WRDS location and threshold services later,

Hank

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2022-04-07T12:59:14Z


commit:28e49118a3defded0f49c6b45586a791c7611014 has changes so that the proj field is used in all calls to WRDS time series services; i.e., the WRDS NWM, AHPS, and observation services.

Still need to resolve this for the location service calls and threshold service calls.

Thanks,

Hank

EDIT: Fixed the commit link

epag commented 4 weeks ago

Original Redmine Comment Author Name: Hank (Hank) Original Date: 2022-04-07T13:01:02Z


Also, note that the constant I defined to support that,

    private static final String DEFAULT_WRDS_PROJ = "WRES";
</code>

should perhaps be moved to a more central location in order to be used in changes for the location service, including thresholds.

Hank