Expose end_session endpoint to perform rp_initiated_logout #5135
Conversation
mcalinghee
changed the title
mcalinghee
force-pushed
the
element-hq/rp_initiated_logout
branch
from
59b1593 to
701d48a
Compare
|
@mcalinghee mcalinghee force-pushed the element-hq/rp_initiated_logout branch from 59b1593 to 701d48a sorry performed a rebase branch by mistake instead of merge branch to update the branch |
sandhose
left a comment
sandhose
left a comment
There was a problem hiding this comment.
Thanks for working on this! Generally agree with having this, but there are a few things to get right
| // #[error("unsupported token type")] | ||
| // UnsupportedTokenType, |
There was a problem hiding this comment.
| // #[error("unsupported token type")] | |
| // UnsupportedTokenType, |
| ) | ||
| .into_response(), | ||
|
|
||
| // Self::ClientNotAllowed | |
There was a problem hiding this comment.
| // Self::ClientNotAllowed | |
| // Self::UnsupportedTokenType => ( | ||
| // StatusCode::BAD_REQUEST, | ||
| // Json(ClientError::from(ClientErrorCode::UnsupportedTokenType)), | ||
| // ) | ||
| // .into_response(), | ||
|
|
There was a problem hiding this comment.
| // Self::UnsupportedTokenType => ( | |
| // StatusCode::BAD_REQUEST, | |
| // Json(ClientError::from(ClientErrorCode::UnsupportedTokenType)), | |
| // ) | |
| // .into_response(), |
| let oauth_session = repo | ||
| .oauth2_session() | ||
| .find_by_browser_session(browser_session.id) | ||
| .await? | ||
| .ok_or(RouteError::BadRequest)?; |
There was a problem hiding this comment.
You can have many OAuth session started from a single browser session, so this way is not the right approach here.
Instead, best is to decode the JWT (but not necessarily verify it, maybe verify_id_token should be changed a little bit?) to get the claims we need and go from there? I'm unclear what we should be checking exactly on that JWT though, I don't know whether there is something that would let us link it to the current browser session or not 🤔
| } | ||
|
|
||
| // Now that we checked everything, we can end the session. | ||
| repo.oauth2_session().finish(&clock, oauth_session).await?; |
There was a problem hiding this comment.
Not sure about that semantic. I don't know whether we should finish the oauth session at this point or let the client still call the revocation endpoint.
Either way, we need a better way to find the right OAuth session, and I think this should be done by injecting the OAuth session ID in the id_token sid claim when we give out the id_token
| // invalid | ||
| let cookie_jar = cookie_jar.update_session_info(&session_info.mark_session_ended()); | ||
|
|
||
| Ok((cookie_jar, Redirect::to(¶ms.post_logout_redirect_uri)).into_response()) |
There was a problem hiding this comment.
The spec says we really should be validating the post_logout_redirect_uri, it should be registered by the client first
The value MUST have been previously registered with the OP, either using the post_logout_redirect_uris Registration parameter or via another mechanism.
| // .into_response(), | ||
|
|
||
| // If the token is unknown, we still return a 200 OK response. | ||
| Self::UnknownToken => StatusCode::OK.into_response(), |
There was a problem hiding this comment.
We should have a nice view in some of those cases, in the end that is going to be a redirect, potentially where it doesn't go back to the client
Co-authored-by: Quentin Gliech <[email protected]>
Co-authored-by: Quentin Gliech <[email protected]>
2 participants
Contexte
Add
end_sessionendpoint to perform a RP-Initiated Logout according to the spec : https://openid.net/specs/openid-connect-rpinitiated-1_0.htmlThis endpoint will allow clients to disconnect from the MAS when they perform a log out.
NOTE :