Repository navigation
Establish incredibly simple time series auth restriction using the ex… - #2012
Conversation
…isting DMZ include list.
msweier
left a comment
msweier
left a comment
There was a problem hiding this comment.
Thanks for this PR. Much needed! I used an AI to find a couple things.
| return; | ||
| } else if (hasTsGroupInfo) { | ||
| // just group provided | ||
| latestValues = dao.findRecentsInRange(office, categoryId, groupId, pastLimit, futureLimit, unitSystem); |
There was a problem hiding this comment.
/timeseries/recent can be bypassed by asking for the whole group. The DMZ check works for when you specify TSIDs, but there doesn't appear to be a DMZ check if you ask for the whole group.
There was a problem hiding this comment.
Yeah, there wasn't a good way to handle that that I could think of. The group is defined and it's pulled in. Though we could loop through after and remove. Not great performance, but this is a quickly developed stop gap.
| .where( | ||
| DSL.and( | ||
| DSL.val(office).isNull().or(TS_ID.DB_OFFICE_ID.eq(office)), | ||
| TS_ID.CWMS_TS_ID.eq(names)) |
There was a problem hiding this comment.
Consider making this case insenstive. I think the endpoint is generally case insenstive for the TSIDs.
There was a problem hiding this comment.
Thanks, you're right. That should be case insensitive.
| .from(TS_ID) | ||
| .where( | ||
| DSL.and( | ||
| DSL.val(office).isNull().or(TS_ID.DB_OFFICE_ID.eq(office)), |
There was a problem hiding this comment.
If the office is null this condition will always be true and there is a chance if there is the same TSID across multiple offices that the DMZ screening logic may use the wrong office and expose a TSID that was intended to be private (although I think this is a remote chance). Consider requiring an OFFICE id or remove the isNull() and filter on an uppercased OFFICE id.
There was a problem hiding this comment.
unfortunately there's a few points where the office id isn't available so we're kinda stuck. I think that's fewer and far between but I've decided to err on the side of caution.
There was a problem hiding this comment.
Also it won't always be true. The query works out to
(? is null or db_office_id = ?) and cwms_ts_id = ?
That's why the DSL.and (condition 1, contion 2) construct is used, to force that grouping.
If the office bind has a value it can't be null and thus if not equal will comes out to false.
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Thanks. good points, a few of which I think I can "fix".
| .from(TS_ID) | ||
| .where( | ||
| DSL.and( | ||
| DSL.val(office).isNull().or(TS_ID.DB_OFFICE_ID.eq(office)), |
There was a problem hiding this comment.
unfortunately there's a few points where the office id isn't available so we're kinda stuck. I think that's fewer and far between but I've decided to err on the side of caution.
| return; | ||
| } else if (hasTsGroupInfo) { | ||
| // just group provided | ||
| latestValues = dao.findRecentsInRange(office, categoryId, groupId, pastLimit, futureLimit, unitSystem); |
There was a problem hiding this comment.
Yeah, there wasn't a good way to handle that that I could think of. The group is defined and it's pulled in. Though we could loop through after and remove. Not great performance, but this is a quickly developed stop gap.
| .where( | ||
| DSL.and( | ||
| DSL.val(office).isNull().or(TS_ID.DB_OFFICE_ID.eq(office)), | ||
| TS_ID.CWMS_TS_ID.eq(names)) |
There was a problem hiding this comment.
Thanks, you're right. That should be case insensitive.
f04f2c6 to
accc955
Compare
…isting DMZ include list.
Summary
Simplest possible way to allow restricting access to time series given current design.
Related Issue
Validation
Integration tests of the logic. Autheticated users with CWMS Users role can see everything, anyone else can only get values if the given time series in in the DMZ Include List. Basically what we had when the public database only received the public data.
NOTE: zero performance testing was done. difference should be minimal. Will have @msweier test in dev once merged and deployed.
Checklist