-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Save OIDC tokens to OpenProject database #16940
base: dev
Are you sure you want to change the base?
Conversation
9a35b87
to
67f649f
Compare
deac74f
to
658fbe9
Compare
An alternative approach to storing the tokens on the In general, we are not done yet: One next step will be to write code that can take a valid access token stored here and exchange it through OAuth 2.0 Token Exchange for a token that can be used to access a third party service. Eventually there will be multiple third party services, that we want to exchange tokens for and then we want to reuse those tokens until they expire. This means we could store all of those tokens in the same table (e.g. with an additional |
658fbe9
to
9f8fff8
Compare
I did what I suggested above. This PR now contains both, the original version (check the diff of commit 1) and the one suggested by me (check the diff of the total PR). My personal prefererence is the current version of this PR. |
03cb4fb
to
14c5ed9
Compare
b59ad82
to
201f5cd
Compare
modules/openid_connect/app/services/openid_connect/create_open_project_token.rb
Outdated
Show resolved
Hide resolved
One other thought that crossed my mind today and was then repeated by @wielinde: Should we store the tokens on the session or on the user? Arguments against session:
The argument for session:
|
201f5cd
to
1130ff2
Compare
For Keycloak in default config, this is possible by adding the So we could attach tokens to the user instead of the session, if we can ensure that they are not expiring. I see elevated potential for misconfiguration here, but on the other hand it still sounds like the cleanest available solution to me. edit: However, seeing that |
Storing tokens in the database to have them available for requests to third parties (e.g. Nextcloud) later. The OIDC session is now marked as optional, since the session link is also used to store access and refresh tokens associated with the session. Those tokens might be present, even if the session id (which belongs to the optional OIDC Back-Channel Logout specification) is missing.
This commit provides an alternative implementation for storing tokens compared to the parent commit. The idea is that we will not only need to store access and refresh tokens obtained via Omniauth, but also the ones to access third party services that will most likely be obtained through OAuth 2.0 Token Exchange. This structure allows to store all of these tokens in the same data model, while keeping the implementation separated from the back-channel logout logic.
Doing so hopefully simplifies token handling a bit. It's now not required to pass specific sessions into services as long as a user is passed. This theoretically also enables us to act in the name of a user from a background job, though we have no specific plans for that yet. A possible downside is, that we now require being handed long-term tokens (i.e. tokens with offline_access scope). On the other hand, we'd have had to consider keeping our tokens fresh for the previous implementation, which we also didn't solve yet.
1ab8a1c
to
06f4d7d
Compare
Ticket
https://community.openproject.org/wp/58365
What are you trying to accomplish?
Storing access and refresh tokens for users that initiate their session through OIDC, so that we can later use them to obtain tokens for accessing third party resources, such as Nextcloud.
What approach did you choose and why?
Storing the access and refresh token on a new table that is intended to store all access tokens for the current session's user, including the ones later obtained through Token Exchange. This provides a clear place to look for credentials and additionally keeps the back-channel logout logic separated from token storage (as opposed to storing the tokens on the
user_session_link
.Merge checklist
Added/updated documentation in Lookbook (patterns, previews, etc)