ubccr / mokey

FreeIPA self-service account management portal
BSD 3-Clause "New" or "Revised" License
189 stars 45 forks source link

Logout fails with hydra integration #70

Closed jvinolas closed 3 years ago

jvinolas commented 3 years ago

When a client app calls the logout endpoint (that seems to be the same for oauth and basic auth in mokey) it seems not to be handling the current logout flow as described in hydra: https://www.ory.sh/hydra/docs/guides/logout

The current implementation for logout it seems to be this one: https://github.com/ubccr/mokey/blob/2e243ee7ae81403c7ca49b2def95d6cf3b606d2c/server/auth.go#L102

But doesn't seem to be using the described hydra logout challenge flow. In the logs we also see: level=warning msg="Logout - Failed to revoke hydra authentication session" error="No sid or user found in session"

jvinolas commented 3 years ago

I've looked at the browser storage and the mokey-sessck is only set when doing login directly on mokey interface (https://login.server/auth/login), not if validating through hydra (https://login.server/oauth/login?challenge...), neither if bypassing hydra.

So, at logout, mokey cookie just doesn't exist. I think this is a bug.

aebruno commented 3 years ago

@jvinolas thanks for the report. This may or may not be related to your issue. But there was definitely a bug in the logout/revoke session handling in mokey. This has been fixed in f8c8e3590f2328ecbb49327356f92c27f3115545. The error was:

level=warning msg="Logout - Failed to revoke hydra authentication session" error="response status code does not match any response statuses defined for this endpoint in the swagger spec (status 502): {}"

The way the current logout/revoke works in mokey is that it assumes you're running hydra and mokey on the same server. It also assumes you're running the hydra administrative API endpoint (port 4445) bound to localhost with fake-tls-termination enabled. When a user signs out, mokey makes an admin API request to hydra to destroy all sessions for that user. In this case there is no need to implement the user logout flow.

jvinolas commented 3 years ago

Sorry, I did not explain well. I'm using freeipa+mokey+hydra in a docker-compose, all in the same internal network (what it is almost the same as having it in the same host).

Validation is going correctly from my client apps (moodle/nextcloud) but when I go to logout from any app it fails:

level=warning msg="Logout - Failed to revoke hydra authentication session" error="No sid or user found in session" Even with the fake-tls-termination enabled. When I logout from apps they redirect to /auth/logout, but as you can see from the code and from the error, mokey is trying to guess the user id from mokey-sessck cookie that was not set on login. I've checked that the mokey-sessck is not being set when doing this login flow through hydra but correctly set if not using oauth at all (validating in mokey with basic auth, not from app/hydra).

aebruno commented 3 years ago

@jvinolas Thanks so much for the additional details. Think I got this fixed up. Can you test? Let me know and I'll close this out.

jvinolas commented 3 years ago

Perfect! So quick, thanks! You can close this one.