refactor basic authentication and session management

[adamkorynta]

Problem Description

All jax-rs resource endpoints include the user token parameter that is redundant with the authorization header. In order to implement CWMS CAC authentication, all basic authentication needs to get refactored so that it is not enabled in the CWMS/CCP version of the REST API.

Solution

• Removed query parameter user token. Authorization headers are now the only way to authenticate with the REST API when using basic authentication.
• Refactored token authentication to a ContainerRequestFilter in order to facilitate a separate implementation for CWMS CAC authentication.
• The token is now moved into the context session.
• There is still a thread monitoring for stale ApiEventClient and ApiLddsClients, though they are keyed to session ids so that they can get shutdown when the session is destroyed.
• removed the -s secure mode flag given that the insecure query parameters were gated by the flag and have been removed
• Added authorization for JWT and Single Sign-On JSESSIONIDSSO cookie checks.

how you tested the change

Used IntelliJ HTTP client checking authenticated vs non-authenticated endpoints.
Added unit tests with heavy (probably too heavy) usage of Mockito.

Where the following done:

• Tests. Check all that apply:
  • Unit tests created or modified that run during gradle test.
  • Integration tests created or modified that run during integration testing
    (Formerly called regression tests.)
  • Test procedure descriptions for manual testing
• Was relevant documentation updated? updated README.md to document the update.
• Were relevant config element (e.g. XML data) updated as appropriate

Relevant discussion page for removing the query parameters: #185

[MikeNeilson]

Looks reasonable to me. Haven't been able to test it, of course.

[MikeNeilson]

Oh, there's an additional link here: #18

I think we'll want to go there eventually, but this PR is a solid first step.

[MikeNeilson]

just marking for future diffs.

[MikeNeilson]

This api, except login, should probably not allow guest operations even for get. While 7.0.13 has introduced a why to hide secrets they will still end up in various properties and allowing guest access without strong consideratio is likely to leak credentials like a seeve.

…

On Fri, Sep 13, 2024, 4:22 PM sonarcloud[bot] ***@***.***> wrote: [image: Quality Gate Failed] <https://sonarcloud.io/dashboard?id=opendcs_rest_api&pullRequest=186> *Quality Gate failed* Failed conditions <https://camo.githubusercontent.com/bd1d5234b37410841ad8cd9d85c84eeecaec92ae18a95c926ae3a59a17c29a65/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f6661696c65642d313670782e706e67> 9.8% Coverage on New Code <https://sonarcloud.io/component_measures?id=opendcs_rest_api&pullRequest=186&metric=new_coverage&view=list> (required ≥ 80%) <https://camo.githubusercontent.com/bd1d5234b37410841ad8cd9d85c84eeecaec92ae18a95c926ae3a59a17c29a65/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f6661696c65642d313670782e706e67> C Reliability Rating on New Code <https://sonarcloud.io/dashboard?id=opendcs_rest_api&pullRequest=186> (required ≥ A) See analysis details on SonarCloud <https://sonarcloud.io/dashboard?id=opendcs_rest_api&pullRequest=186> <https://camo.githubusercontent.com/0d8093b061da31b2dde5907c87c133451a9fed9ab208145ec3750960f13f438a/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f6c696768745f62756c622d313670782e706e67> Catch issues before they fail your Quality Gate with our IDE extension <https://camo.githubusercontent.com/cf28d9441c2be84b83964b59542f8523d293ea1d96a9d3cea77e7962537f0175/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f736f6e61726c696e742d313670782e706e67> SonarLint <https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request> — Reply to this email directly, view it on GitHub <#186 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB44KCAUSJO4NPKW5DCS53LZWNXUXAVCNFSM6AAAAABN7LV2FGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJQGY4TIOBTGI> . You are receiving this because you commented.Message ID: ***@***.***>

[MikeNeilson]

Definitely looking more flexible.

Some notes, nothing that really needs to be taken care of.

[MikeNeilson]

Just marking.. again.

[MikeNeilson]

Overall the change looks good and is in line with what I was thinking.

I was planning to use plain Java ServiceLoader; however, if you think there's a lot of value in the HEC lookup system, and perhaps there is. We should investigate moving the lookup code to github (the HEC one) and make sure the artifact gets to maven central to avoid that coupling to HEC itself.

[MikeNeilson]

Updated responses.

[adamkorynta]

Yeah, I was more referring to trying to keep the DAO's operating with DTO adjacent data types, and keep Principal/sessioning up at a separate layer.

[MikeNeilson]

Yeah, I was more referring to trying to keep the DAO's operating with DTO adjacent data types, and keep Principal/sessioning up at a separate layer.

Okay, that's what I thought after reading it more, though I would still posit that returning a full javax.security.Principal is reasonable. But I'm not set one it enough to do anything but provide the suggestion. The interface looks nice and generic enough to me know.

[rma-psmorris]

Finished review from sec on.

[MikeNeilson]

Marking for next batch.

[sonarqubecloud]

Quality Gate passed

Issues
45 New issues
0 Accepted issues

Measures
0 Security Hotspots
44.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[rma-psmorris]

Approved, no further comments

[MikeNeilson]

Oh right that particular colulm is populated by the user managmemt system and not used in CWMS.

…

On Mon, Sep 30, 2024, 1:58 PM Adam Korynta ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In opendcs-rest-api/src/main/java/org/opendcs/odcsapi/sec/cwms/CwmsAuthorizationDAO.java <#186 (comment)>: > @@ -44,11 +44,18 @@ public Set<OpenDcsApiRoles> getRoles(String username) throws DbException { Set<OpenDcsApiRoles> roles = EnumSet.noneOf(OpenDcsApiRoles.class); roles.add(OpenDcsApiRoles.ODCS_API_GUEST); - String q = "SELECT user_group_id" + - " FROM av_sec_users" + - " WHERE db_office_code = cwms_util.get_db_office_code(?)" + - " AND username = ?" + - " AND is_member = 'T'"; + String q = "select user_group_id " + + "from cwms_20.av_sec_users " + + "where db_office_code = cwms_20.cwms_util.get_db_office_code(:input_db_office_code) " + + " and upper(username) = case " + + " when instr(:username_str, '.', -1) > 0 then " + + " (select userid " + + " from cwms_20.at_sec_cwms_users " + + " where edipi = substr(:username_str, instr(:username_str, '.', -1) + 1)) " + av_sec_users is the one with the office code, at_sec_cwms_users has an 'office' column looks to be used for something else (it's null in our database exports and 16 bytes). — Reply to this email directly, view it on GitHub <#186 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB44KCF36CIJM3TLSLQGE4DZZG3NXAVCNFSM6AAAAABN7LV2FGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZYGU2TAOJZGE> . You are receiving this because you commented.Message ID: ***@***.***>