Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consent: Share session state between login and consent #1003

Closed
MOZGIII opened this issue Aug 22, 2018 · 20 comments

Comments

Projects
None yet
3 participants
@MOZGIII
Copy link
Contributor

commented Aug 22, 2018

Do you want to request a feature or report a bug?

It's kind of both.

What is the current behavior?

Accept login request API call does not allow setting extra accessToken data.
Get consent request does not provide accessToken data.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

Just use the standard example nodejs app.

What is the expected behavior?

Accept login request API call allows setting extra accessToken data.
Get consent request provides extra accessToken data set when accepting login request.

Which version of the software is affected?

1.0.0-beta.8

The story & rationale

Hey guys, I have an issue with the new login/consent logic.

It was charged since v0.11, and we're now updating to v1.0.0-beta.8.

Now it first redirects the user to login request verification callback, and allows to set subject when accepting login request.
Then it assumes that the consent endpoint, given just the subject can provide the accessToken information when accepting the consent request.

This became problematic for us due to a disconnect between assigning the subject and extra accessToken data. The thing is, in our system, just the subject is not enough to fetch the internal user object, and therefore to get the context information. We put additional parameters (semantically similar to "region", or "rack id") in the accessToken, that allow us to reference the internal user.

This is why we simply can't fetch the user given just the subject, and can't properly perform consent (and assign accessToken).
It looks like a dead end to me.

There are possible workarounds to this issue, like storing the additional data in memory for the short period of time on the callback handler server side, but it's obviously broken and ugly, yet insecure.

Can we allow assigning extra accessToken data when accepting the login request, then accessing that data when reading consent request and reassigning them when accepting consent request?

@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

So the issue is that the access token session data is (usually) coupled to the consent state, not to authentication. Personally, I see access token data as internal session data that should not be available to the outside world. The issue is that many people see this differently and want JWT with transparent session data. As we added JWT due to popular demand, the session data is no longer internal and should thus rely on the scopes that have been granted. Therefore, it makes most sense to set the session data during the consent acceptance.

We had an issue where we discussed that it would be great to set additional data ("metadata") during consent and login. This would allow the consent app to delegate state management to hydra instead of implementing a DB connectivity for this. Would this improve your situation?

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

If you mean that instead of setting the accessToken I’ll be able to set and then read some other metadata field that would be not be shown anywhere in the JWT or token introspection - or even be available solely in the current authentication flow (and consequent “skipped” ones) - that would also solve the issue, as I could read that metatada at the consent callback and properly reference the the correct the user object.

However, I’m not sure that adding a generic thing to manage store (i.e. with an unrestricted flow of updates) is a great idea. My guess that having a “hidden” piece of data that you can assign only when accepting login request and then read everywhere else where you can read the subject might be a simpler to maintain and less error prone thing.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

If you mean that instead of setting the accessToken I’ll be able to set and then read some other metadata field that would be not be shown anywhere in the JWT or token introspection - or even be available solely in the current authentication flow (and consequent “skipped” ones) - that would also solve the issue, as I could read that metatada at the consent callback and properly reference the the correct the user object.

The data would be available when

  1. the same user is found during login dance (requires login.remember: true)
  2. the same user is found during consent dance (does not require login.remember: true)

You're right however that writing to this store unhinged is prone to conflict. It's probably not such a great idea after all.

Another possibility would be to include the login challenge in the consent request payload. That way you could set up a store where you temporarily keep the data you need during login (the key would be the login challenge) and retrieve (and optionally delete) that data during consent by looking up the login challenge.

With that, you can implement what you're asking and we're not adding stuff to the hydra functionality that might work for some cases but not all.

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

Data availability you're describing would solve the issue we have.

I don't like the idea with login challenge being passed because currently the consent/login app is stateless. Hydra already persists the data related to login/consent, so in a general, I'd prefer it to handle the state. For our case it's not better than the workaround I described in the issue - to maintain internal state per subject ID in the consent/login app. I don't like it because it becomes too complicated.

Since our case can be generalized as referencing sharded data with explicit sharding key (i.e. it's not derived from ID) having the ability to pass some additional state via hydra in there would help a whole class of systems.

@aeneasr aeneasr changed the title Issue with new login/consent flow consent: Share session state between login and consent Aug 23, 2018

@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I don't think it's that easy generalizable. Just off the top of my hat, I already see two distinctions:

  1. You set data during login and it is available for the related consent request. If the user performs another OAuth 2.0 flow the data will not be available, unless you explicitly set it there as well.
  2. You set data during login which is available for the remainder of the login session validity (if remember is true that's e.g. 30 days) to both login and consent. Assuming the user performs another OAuth 2.0 flow, the data will be available for both login and consent.
@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

For the case of just passing state from login to consent handlers we only need the first (1) option. (2) is something else, as it shares the session data. Might be useful too, but not clear when.

(1) with the skip mode should also allow access to data, so (2) is not needed in my opinion.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

(2) is not needed in my opinion.

Well...it's not needed for your use case, which is why I said that it's not that easily generalizable.

Nevertheless, I think including the login challenge during consent is a good start. We might even include the login session ID if it exists, that way you can persist data across multiple login requests. Storing the data for you isn't something I'd put on the priority list. First it's not something required for getting to 1.0.0 stable and second is that ORY Hydra isn't a datastore. I mean sure, we can have store a JSON object for you, but you can't query it. You can't update the schema. You can't migrate. You can't test it. I'm doubting if that would be a good idea. Your consent/login app is connected to a store anyways (where would you get your user data from otherwise?) so the added complexity of storing something along a key in a format that you see fit isn't something that is difficult to achieve.

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

I'm trying to elaborate on the generic case, not just my case.
And I'm not saying that (2) is not needed in general, I mean that it's not need to pass the data from login to consent because it's solved by (1) already, and that (2), being not bound to consent/login session but to just login session, is something else.

I didn't mean to say I know what's needed and what's not, sorry if I miscommunicated that.

Definetly, adding logic challenge is a good idea. :) But for me it doesn't improve the situation.

On the other hand, if we had both login challenge and the ability to store metadata in the login request record, I could fetch login request from the consent endpoint and it would solve my issue.

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

It's also true that we can add a database on our end. I just hope to find a way to keep this part stateless, because we already have a database for hydra, but the datastore for our "users" is kind of specific, so it's not a good option for us to extend that. Probably, therefore, our use case it's not that important for hydra - it's very restrictive. Although, I'm sure there are a lot like it in the wild.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I didn't mean to say I know what's needed and what's not, sorry if I miscommunicated that.

No need to apologize. I may sound harsh sometimes, but that's just my German nature ;)

Ok, I think the first actionable item will be to add the login challenge to the consent payload so we can track this across requests. Regarding storage of metadata, I really have to think about this. As I said in the previous comment, I see a lot of downsides to this. But maybe I can come up with something that eases those concerns! Please note though that this isn't high priority as there's a ton of stuff currently on my / our plates. If you need this rather soon, either work out a proposal for a PR or drop a line to hi@ory.sh to figure something out. Cheers!

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

Thanks @aeneasr. It's not urgent for us, since we implemented workaround already (we're pretty tight on time with this upgrade, so we're willing to exchange time for tech debt).
Looking to forward to help with elaborating, hope we'll be able to remove our workaround if favor of a proper way.

@aeneasr aeneasr added this to the unplanned milestone Aug 23, 2018

@aeneasr aeneasr modified the milestones: unplanned, v1.0.0-rc.1 Aug 27, 2018

aeneasr added a commit that referenced this issue Aug 27, 2018

consent: Forward session and login information
Consent and login requests now carry context information for previous requests.

Closes #1003
@aeneasr

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

We added login.session_id and consent.login_session_id as well as consent.login_challenge which allow you to link the request to one another. The store you requested will not make it in the product because of the issues I mentioned.

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Good start, thanks. This will improve the situation.

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Still hoping to see a proper solution though.

aeneasr added a commit that referenced this issue Aug 27, 2018

consent: Forward session and login information
Consent and login requests now carry context information for previous requests.

Closes #1003
@struhtanov

This comment has been minimized.

Copy link

commented Feb 12, 2019

I have very similar problem. I'm working on multi-tenant application, where tenant is retrieved from host. Similar to slack, where you go to a.slack.com or b.slack.com, depending on your organization.
I want to have one url to give to my oauth clients. Let it be auth.slack.com (in our slack example). When client is redirected to login-consent app(by hydra), I show screen to select organization. After that I redirect user to login screen, then to consent screen. In the end I want to put information about organization that user has entered into oauth token(using session field of acceptConsentRequest params). When client is making requests to my server with oauth token in headers, I can check if organizations in the token and in the url match.

Similar to @MOZGIII I need somehow store information between login and consent requests. In my case I want to store information about selected organization. As everything is going through hydra by redirects, I have no other options than to store some kind of session in my login-consent app. This looks error-prone to me, because hydra has session (using cookies) and it could be possible that I'm redirected to consent screen without visiting login screen(because there is already session in hydra). So there will be no place to get organization info.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Ok, let's reopen this - feel free to PR (please discuss the changes first in this issue).

@aeneasr aeneasr reopened this Feb 13, 2019

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

I'd just want login and consent flow to be stateless. I mean, we don't even have a meaningful consent semantics - but we still need to push the extra fields for the introspection to return for token. Would work best if we just could pass them from the login endpoint.
How about supporting two integration flows? This sounds bad, but maybe it's not really that bad.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Why don't we add another field to the /login/:challenge/accept endpoint with metadata json.RawMessage which is then returned at /consent/:challenge as login_metadata? I don't think this should persist though but be individual metadata for every login&consent flow

@MOZGIII

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

But then with autologin you won't be able to use that metadata.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

The login endpoint would have to resend the data. Keep in mind that every login request is being forwarded to the login endpoint, regardless of remember state.

@aeneasr aeneasr modified the milestones: v1.0.0-rc.1, v1.0.0 Apr 7, 2019

aeneasr added a commit that referenced this issue Apr 11, 2019

consent: Add ability to share data from login to consent request
Closes #1003

Signed-off-by: aeneasr <aeneas@ory.sh>

@aeneasr aeneasr closed this in 20aaa46 Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.