huggingface / chat-ui

Open source codebase powering the HuggingChat app
https://huggingface.co/chat
Apache License 2.0
7.19k stars 1.04k forks source link

For openID authentication, logout button should call /logout url of Authentication service #577

Open martindetournay-bw opened 9 months ago

martindetournay-bw commented 9 months ago

Situation: I'm using openID connect to authenticate my users through the app. Login works perfectly

Complication: The sign-out button doesn't do anything because it only deletes local cookies. If I press "sign in" again, it goes straight through login as we didn't clear the session in the OpenID authentication service (I think that's what happens?)

Solutions: I'm not familiar with authentication protocols, but reading a bit I understand we would need to fully logout the user by calling the url: https://login.microsoftonline.com/{tenant}/oauth2/v2.0/logout?

Currently the logout button only deletes cookies:

    cookies.delete(COOKIE_NAME, {
        path: "/",
        // So that it works inside the space's iframe
        sameSite: dev ? "lax" : "none",
        secure: !dev,
        httpOnly: true,
    });

I'm not familiar with authentication protocols, but reading a bit I understand we would need to fully logout

martindetournay-bw commented 9 months ago

I would have loved to contribute and submit a PR but I have no .js experience😅

nsarrazin commented 9 months ago

I'm not super familiar with how it's supposed to work so pinging @coyotte508 but that seems reasonable to me

coyotte508 commented 9 months ago

The Open ID authentication service is shared amongst all your apps, we can't really force a session clear on the IdP's side.

What you can do is on HF's side either add prompt=login or max_age=0, see https://auth0.com/docs/authenticate/login/max-age-reauthentication, probably by adding a parameter here: https://github.com/huggingface/chat-ui/blob/bb33d30b1d97788c7b948748fb2bd60cb53093ba/src/lib/server/auth.ts#L99

I don't think we want to do it on hf.co/chat, maybe make it an optional param?

coyotte508 commented 9 months ago

We don't store the oauth token after the sign in, we only use the sign in to extract the user's id and then store it in a cookie of our own.

We don't maintain a persistent connection with the IdP or refresh anything, so the existing logout mechanism of the OpenId Connect protocol doesn't fit with the app.

And I don't think it would solve your current issue, as it would only clean ressources (like refresh tokens) associated to the app but not the app authorization itself, meaning that the next "sign in" click would probably be instant as well (as long as the user used the IdP recently, for hf chat or any other app).

nsarrazin commented 9 months ago

Thanks for clarifying! It's a lot clearer for me.

I guess if the desired behaviour for @martindetournay-bw is just to have to login again through the identity provider, we could add a flag to the OPENID_CONFIG that sets prompt=login like you mentioned, or a max_age param.

martindetournay-bw commented 9 months ago

Hi @nsarrazin and @coyotte508, thanks for getting back to me! I think the max age parameter makes a lot of sense. The reason for me posting this issue is:

  1. Using the "sign out" button currently has no action when using OpenID connect, since the user is logged right back in anyways

  2. I'm connecting sensitive internal data to my app, I want to make sure users are asked to log back in regularly --> Max age parameter seems to solve this

@nsarrazin I think it makes sense to keep this issue in the backlog?

Appreciate the help🙏

coyotte508 commented 9 months ago
2. I'm connecting sensitive internal data to my app, I want to make sure users are asked to log back in regularly --> Max age parameter seems to solve this

Setting max_age alone won't solve this, instead, reworking the session system to separate sessions & users + setting an expiration date on sessions

martindetournay-bw commented 9 months ago

Ok got it, today auth tokens are at user level not session level and we need to split those to force session refreshes.

Is this a functionality that is useful for the public huggingchat or should I plan to build this on my side within a fork ?

I'm barely ramping up on .js / svelte so not sure what is the implication of adding this in the current codebase (if it is a massive rework or not)

coyotte508 commented 9 months ago

cc @nsarrazin

golgeek commented 6 months ago

Hi, was there any progress on this issue?