LG-7434: Allow cross origin for POST OIDC Logout

[lmgeorge]

🎫 Ticket

Link to the relevant ticket:
LG-7434: Add support for POST OIDC logout requests (this ticket has been superseded by this GitLab issue).

🛠 Summary of changes

• Updated the controller to use separate methods for GET and POST
• The POST route redirects to the GET route for better UX when using browser navigation
• Added analytics field original_method to capture redirected method for OIDC Logout Requested event. See LG-7434: Allow cross origin for POST OIDC Logout #10697 (comment) for details
• Updated tests
• Updated doc comments

📜 Testing Plan

Requires:

• A modified sample OIDC app to sign-out users with POST
• identity-idp app to have reject_id_token_hint_in_logout: true in application.yml OR the partner app must not send an id_token_hint

Steps

1. Sign in to the partner app (see this branch to get spun up quickly)
2. Click Sign out
3. Confirm that the appropriate confirmation view is shown
4. On Do you want to sign out of Login.gov?, ensure you are redirected to the sample app home page (or other configured redirect URI).

---

Why:

• It is expected that requests will be made by relying parties on verified external domains

• The specification for OpenID Connect RP-Initiated Logout 1.0 requires both HTTP GET and POST methods to be supported. See: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout

• Data sent using the POST method remains encrypted during transport in the browser and in web application logs, preventing leakage of sensitive information

How:

• The same endpoint shall be used, /openid_connect/logout, but the request data must be sent as part of the body and use form serialization as required for HTTP POST requests (RFC 9110, sec. 9.3.3).

• Disables Rail's CSRF token verification for the POST route only

resolves https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/backlog-fy24/-/issues/20

changelog: Bug Fixes, Security, Fix CORS stopping POST for OIDC RP-Initiated Logout 1.0

[lmgeorge]

Context: the original implementation of this did not handle the CSRF logic correctly, this fixes that.

[mitchellhenke]

I see that the status: :temporary_redirect is to preserve the method and body of the request, but what makes that necessary?

[lmgeorge]

In effect, when testing with 302 (the default response for POST and PUT requests) and 303, I found Chrome (in particular) would cache the action chosen on the logout confirmation page, so subsequent logout requests would convert the request to a GET of the redirect URI, skipping the confirmation page altogether sometimes, which we don't want.

[mitchellhenke]

My understanding is we always want a GET for the redirect back, the POST support is only needed for the incoming request from SPs.

[lmgeorge]

The subsequent GET request of the redirect URI always happens, but do we always want to let the browser skip the confirmation page if the user selected a particular option previously?

[lmgeorge]

Updated this to not use a different status code. The routing behavior I described was misinformed by a previous implementation that used match to send both POST and GET to the same action.

[lmgeorge]

@zachmargolis - we would probably need either a new event field (i.e., original_method) or a new event type that could be logged in the #create action. As far as I can see in my local logs, we don't populate event_properties.method in the analytics event.

[zachmargolis]

New event field makes sense to me!

[lmgeorge]

We'd still have the generic web request logs and I don't know if those are bundled into analytics some other way.

[zachmargolis]

those are a separate log file (production.log) which doesn't always have SP available, so if we want to analyze HTTP method in events.log (by SP it sounds like), then we need to augment analytics events

[lmgeorge]

Added the new field. method and original_method only get populated for the OIDC Logout Requested event.

GET only:

{
    "name": "OIDC Logout Requested",
    "properties": {
        "event_properties": {
            "success": true,
            "client_id": "urn:gov:gsa:openidconnect:sp:sinatra",
            "client_id_parameter_present": true,
            "id_token_hint_parameter_present": false,
            "errors": {},
            "error_details": null,
            "sp_initiated": true,
            "oidc": true,
            "saml_request_valid": null,
            "method": "GET",
            "original_method": null
        },
        "new_event": true,
        "path": "/openid_connect/logout",
        "session_duration": 20.555755
        // ...
    },
    // ...
    "log_filename": "events.log"
}

POST redirected to GET:

{
    "name": "OIDC Logout Requested",
    "properties": {
        "event_properties": {
            "success": true,
            "client_id": "urn:gov:gsa:openidconnect:sp:sinatra",
            "client_id_parameter_present": true,
            "id_token_hint_parameter_present": false,
            "errors": {},
            "error_details": null,
            "sp_initiated": true,
            "oidc": true,
            "saml_request_valid": null,
            "method": "GET",
            "original_method": "POST"
        },
        "new_event": false,
        "path": "/openid_connect/logout",
        "session_duration": 781.444997,
        // ...
    },
    // ...
    "log_filename": "events.log"
}

[lmgeorge]

Converting to draft since the analytics changes have broken the tests.

[lmgeorge]

Converting to draft since the analytics changes have broken the tests.

Fixing the tests was pretty straightforward. This is ready for review again.