ory / hydra

The most scalable and customizable OpenID Certified™ OpenID Connect and OAuth Provider on the market. Become an OpenID Connect and OAuth2 Provider over night. Broad support for related RFCs. Written in Go, cloud native, headless, API-first. Available as a service on Ory Network and for self-hosters.
https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=hydra
Apache License 2.0
15.68k stars 1.5k forks source link

Implement the prompt=select_account OIDC parameter #1943

Closed SlevinWasAlreadyTaken closed 2 years ago

SlevinWasAlreadyTaken commented 4 years ago

Hello !

Is your feature request related to a problem? Please describe.

I work on a multi-account system and I must let user select the account they want to log in after having initiated an auth flow.

While accepting the login flow, the presence a session for Hydra at the init of an auth flow forces the subject value to correspond to the subject value detected in the session.

This is implemented on Hydra, and mentioned in the Hydra User Login guide, in the node-js pseudo code corresponding to the login accept request

[...]
const body = {
    // This is the user ID of the user that authenticated. If `skip` is true, this must be the `subject`
    // value from the `fetch('https://hydra/oauth2/auth/requests/login?' + querystring.stringify({ login_challenge: challenge }))` response:
    //
    // subject = response.subject
    //
    // Otherwise, this can be a value of your choosing:
    subject: "...",
[...]

To let the end-user choose a different account to log in that the one in session, it seems we should expire the session/remove the cookies then restart the flow from the beginning. But most of the time, the auth flow was initiated by some external relying parties, so we can't restart the flow for them...

The need to connect on a different account exists because of:

Describe the solution you'd like

It seems OIDC confirms this need since it has defined a standard to handle it: the prompt parameter select_account, defined the Authentication Request Section of the RFC.

This parameter asks the IdP to offers to the end-user the choice to connect the any account linked with the current session. They can also eventually add a new account which in not in the current session and wished to be added.

I have quickly checked the documentation and tested the UX regarding some existing implementation: Connect 2ID, Microsoft Azure, Google...

To me:

Code Analysis:

I have checked on Hydra and Fosite project, it looks like the select_account value for the prompt parameter is allowed but not taken into account (ignored, basically).

Additional context

Security Considerations:

It seems quite common to have this parameter available for the prompt.

If we don't manage the multi-subs session directly, we might want to expire the existing session received at the auth init flow before offering the new session for the new selected sub.

aeneasr commented 4 years ago

Would it make sense to implement prompt=select_account in such a way that Hydra behaves like prompt=login? So it would basically destroy the Hydra login session cookie and ask the login endpoint to log in a user?

SlevinWasAlreadyTaken commented 4 years ago

Usually, there is still the possibility to use the existing session cookie to not re-ask the end-user to authenticate (in the case the end-user chooses the corresponding account).

From the OIDC RFC - bold sentence:

select_account The Authorization Server SHOULD prompt the End-User to select a user account. This enables an End-User who has multiple accounts at the Authorization Server to select amongst the multiple accounts that they might have current sessions for. If it cannot obtain an account selection choice made by the End-User, it MUST return an error, typically account_selection_required.

To destroy the Hydra login session cookie would not correspond to this description then and to the feature suggestion :s.

aeneasr commented 4 years ago

Right, that makes sense - in that case allowing the login & consent app to send a different user would contradict:

This means that to implement this feature, we really need to be able to deal with multiple sessions in ORY Hydra. I'm not going to lie, this is a ton of work! Just getting the regular flow was already a lot of work, adding multiple users to it might be very hard to get right from a security-perspective.

SlevinWasAlreadyTaken commented 4 years ago

Indeed, I had also in mind the fact the multi-sessions management is a big thing to integrate!

However maybe the solution I am looking for is not about implementing the prompt=select_account parameter & the multi-sessions...

The functional issue is that users are not be able to change the account they have a current session for while being redirect from a relying party (RP) to our SSO system (because the valid session forces the subject):

  1. A user has an identity A and an identity B within our SSO.
  2. They have an active session linked to the identity A on Hydra.
  3. They are on any RP and click on connect, wishing to connect as Identity B.
  4. They will be connected as Identity A because of the valid session. We cannot re-init an authorization code flow for an external relying party once we are in it. We can't perform a logout/restart strategy then finish properly the authorization code flow (maybe it is possible but I do not see how today).
  5. The user is supposed to go on our SSO app in another context in order to logout then come back on the RP to re-connect and this time, they will be able to connect as Identity B.

Basically, we want to avoid the step 5 by making the 4th step more flexible.

We could allow to change the sub even if a valid session is detected, but the previous session would be expired and a new one would be generated and adopted by the login/consent apps.

A hydra configuration field could take care of it: it would have a default value set to the traditional way (forced to current session).

In term of implementation:

On Hydra, I don't know how complex it would be to finally not take into account the existing session at the end of the login flow, and generate a new one.

On the login app: I know there is the session_id in the return of the Get a login request endpoint. This allows the login app to know about which session is in use and then track it.

The fact this session_id is returned by Hydra, but might be finally not to use is might be error-prone. The login app must not use the session_id if the end-user authenticate using a different sub.

On the consent app: if Hydra does the job of replacing the previous session with the newly generated, I would say the Get a consent request endpoint must return the new session in login_session_id.


I understand this makes more complex the handling of the flow on the login/consent app.

I definitely don't have everything in mind regarding security considerations, what do you think about it? :)

Thanks :)

aeneasr commented 4 years ago

They will be connected as Identity A because of the valid session. We cannot re-init an authorization code flow for an external relying party once we are in it. We can't perform a logout/restart strategy then finish properly the authorization code flow (maybe it is possible but I do not see how today).

You can redirect to the request_url and append &prompt=login to it - would that solve your headaches here?

aberasarte commented 4 years ago

They will be connected as Identity A because of the valid session. We cannot re-init an authorization code flow for an external relying party once we are in it. We can't perform a logout/restart strategy then finish properly the authorization code flow (maybe it is possible but I do not see how today).

You can redirect to the request_url and append &prompt=login to it - would that solve your headaches here?

If it helps, this is exactly what we are doing to cover our select_account flow and it's been working perfectly fine.

SlevinWasAlreadyTaken commented 4 years ago

They will be connected as Identity A because of the valid session. We cannot re-init an authorization code flow for an external relying party once we are in it. We can't perform a logout/restart strategy then finish properly the authorization code flow (maybe it is possible but I do not see how today).

You can redirect to the request_url and append &prompt=login to it - would that solve your headaches here?

Thanks for the tip! I didn't see the Using login_hint with a different subject section which describes this possibility.

Unfortunately it doesn't fully solve my headaches ^^:

I was still counting on Hydra to inform my login service that a login session is valid before switching to another sub. In our business, we consider a login session on a sub as an potential authentication method (still equivalent to acr 0) to connect to another sub - if the two sub are linked via a special entity.

Because of the prompt=login ignores completely the current session, we cannot do it.

This is why the optional configuration solution would solve our problematic, since the multi session seems difficult to contribute on today. Would you accept a contribution about this?

aeneasr commented 4 years ago

The problem I have with proposal https://github.com/ory/hydra/issues/1943#issuecomment-654963353 is that you need session management on the login app side. But we explicitly tell people not to implement session management on the login app side because it is difficult to achieve OpenID Connect correctness wrt to consent, login, and none prompts.

The only way select_account can be implemented without hacking around this (e.g."don't so session management in your login app unless you do multi-account") is to properly implement it in ORY Hydra. This means that:

  1. The ORY Hydra authentication session needs to be able to track more than one user in it.
  2. The logout action must be able to distinguish between user sessions.
  3. The login challenge/response dance must be enhance to allow not only one subject field hint, but 1..n - or alternatively something like alternate_subjects (could use better naming...).
  4. Accepting login must be able to allow/prevent setting a multi-session cookie

All of this is possible IMO - but it's quite a lot of work unfortunately (especially all of the testing). I probably won't be able to work on this in the near future. I think we need to address a couple of other things first also, which should help reduce the complexity in the consent module:

  1. Refactor integration tests for better readability and more reusability. I really like the way we write the integration tests in ORY Kratos now, where we define helper functions and write concrete sub-tests instead of having these huge test-case structs. This also helps with running tests in isolation in VSCode or other IDEs: https://github.com/ory/kratos/blob/master/selfservice/strategy/password/registration_test.go#L181-L223
  2. Refactor the persistence layer to hide some of the SQL boilerplate. Again, I really like the way we approach this in ORY Kratos where we have one persistence package with different implementations (right now only SQL) and where we're using gobuffalo/pop to have a lightweight DBAL interface on top of sqlx. Compare that to this SQL boilerplate in ORY Hydra where we need to make sure that all the keys are 1:1 mirrored in the SQL statements.
  3. We remove the memory adapter with an SQLite in-memory database which allows us to do verification/testing on e.g. cascading events without writing a lot of useless business logic just to get fast testing locally.

The last thing is probably to rethink the consent strategy which is currently a 1000 LoC beast where even I have to re-read most of the code to understand what's going on.

Once all of that is done, it should be much easier to build in such a complex feature (whose need I absolutely see by the way!) into the code base. I'm sorry that I don't have a better answer for you ("let's implement this right away! :)") but that's simply the state of the project right now. And because we're still a very small company we (and I) simply don't have the time to work on all of this right now :/

SlevinWasAlreadyTaken commented 4 years ago

First of all, thank you for this complete answer.

I agree with all your points, the fact it touches most of Hydra's logic deserves some refactoring to refresh the architecture according to your new principles (functional testing, more horizontal layers...).

Since you cannot work on this (and I totally understand why) and I would not be able to handle this myself too - kind of the same situation plus I consider myself not being mature enough with the two codebases and paradigms, I will make compromise on the related features and see how much it becomes indispensable (or not) in the following months.

You may close this ticket if you think it is relevant.

Have a good day!

aeneasr commented 4 years ago

Thank you, I'll keep this issue open for others to see and because I think that the feature deserves attention. We'll get there eventually :)

sneko commented 3 years ago

Hi,

I was also looking for a multi-sessions management but I know it's a big deal. In the meantime I just want to simplify the flow of switching the user account. Since you were on this path, I'm interested to have your thoughts:

... my only problem is that I would like the user the be redirected onto Client B after logging in. So what I prepared was during the logout I set in the state some information about the login_challenge so once he logs in as User B he will use the same login_challenge as before the logout and he will get back to Client B.

Unfortunately it seems during the logout Hydra invalidates the login_challenge (probably because this one was linked to a remembered session, which makes sense to not just hijack the challenge). I'm totally fine with that but I'm trying to find a "workaround" to maybe reuse some information from the invalidated login_challenge and inject it in a new one?

Any thought what could help making this? Without this the user has to get back to Client B after the logout and starts a new flow from scratch, which is... weird and long.

Thank you,

EDIT: After reading the thread a new time I think I misunderstood at first this sentence

You can redirect to the request_url and append &prompt=login to it - would that solve your headaches here?

That's totally the point! It's like to "mimic" initiating the flow from the original client, and setting &prompt=login forces ignoring the current session in cookies. That's great!

Thank you the 3 of you for this discussion :) it helped!

github-actions[bot] commented 2 years ago

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️