Add ODIC Back-Channel Logout #304
Conversation
|
Uh, whoops. Literally just heavily modified the session system for MFA. I'll take a look and see what works for both OIDC and MFA. |
|
This is ready to be reviewed now |
DecDuck
left a comment
DecDuck
left a comment
There was a problem hiding this comment.
Just some nitpick stuff, will have more of a look over when I have a wider screen (I'm on mobile).
|
Fixed |
Huskydog9988
force-pushed
the
oidc-logout
branch
from
04d4817 to
b40eee2
Compare
|
Typecheck is very unhappy |
|
Fixed |
| }); | ||
| return { userId: authMek.userId, result }; | ||
|
|
||
| return { result: true, userId: authMek.userId }; |
There was a problem hiding this comment.
Result needs to be returned, it's a enum containing "fail", "2fa" or "success" - the client uses it to redirect.
| import sessionHandler from "../../session"; | ||
| import type { SessionSearchTerms } from "../../session/types"; | ||
|
|
||
| // TODO: monitor https://github.com/goauthentik/authentik/issues/8751 for easier?? OIDC setup by end users |
There was a problem hiding this comment.
Do we know if any self-hosted IDP that has this spec implemented? Would love to have the feature.
| if (!configuration) | ||
| throw new Error("OIDC try to init without configuration"); | ||
|
|
||
| if (systemConfig.shouldOidcRequireHttps()) { |
There was a problem hiding this comment.
Maybe refactor into an array and a loop to check them all?
| issuer: this.oidcConfiguration.issuer.toString(), | ||
| }); | ||
| } catch (e) { | ||
| console.error("Failed to verify OIDC logout token:", e); |
| } | ||
| for (const [key, value] of Object.entries(options.data || {})) { | ||
| // stringify to do deep comparison | ||
| if (JSON.stringify(session.data[key]) !== JSON.stringify(value)) { |
There was a problem hiding this comment.
Need to use JSON.stringify(sortMyObj, Object.keys(sortMyObj).sort()) to sort keys to ensure this check works if the objects are modified in different orders.
Alternatively, keep it this way and add comment explaining we don't sort keys because we want the objects to be truly identical.
There was a problem hiding this comment.
I need to double check it, but you can't use Object#getKeys on a non object.
| // this.sessionProvider = createMemorySessionProvider(); | ||
| } | ||
|
|
||
| // async signin( |
|
Just reviewed. Also, does this need a test with the new MFA stuff? |
|
Yea, I don't have the setup to properly test the changes rn |
2 participants
Let an ODIC provider sign out users. Also attempted to make the server more spec compliant by verifying responses and enforcing HTTPS for providers by default.