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

Allow Session change without session token #7883

Closed
2 tasks done
livio-a opened this issue May 1, 2024 · 4 comments · Fixed by #7963
Closed
2 tasks done

Allow Session change without session token #7883

livio-a opened this issue May 1, 2024 · 4 comments · Fixed by #7963

Comments

@livio-a
Copy link
Member

livio-a commented May 1, 2024

The session update and deletion require the current session token as argument.
Since this adds extra complexity but no real additional security and prevents case like magic links, we want to remove this requirement.

We still require the session token on other resouces / endpoints, e.g. for finalizing the auth request or on idp intents.

Acceptance criteria

  • Deprecate the token field on update and delete endpoint
  • Update all documentation examples without token use
@livio-a
Copy link
Member Author

livio-a commented May 1, 2024

relates to #6099

@muhlemmer
Copy link
Contributor

@livio-a, I have 2 questions:

  1. If we remove the token from the delete endpoint, it is a breaking change. Do we really want this? The token was already optional.

Sessions can be terminated by either:
- the authenticated user
- a manager, who is granted `session.delete` (e.g. ORG_OWNER) on the authenticated users organisation
- providing the current session_token in the body.

As without the token we can't establish the "authenticated user" and the logic falls back to the session.delete permission. I kind of figured because the OIDC integration tests break, as the CTXLOGIN user does not have the permission through the ORG_USER_MANAGER role for example.

I have a draft PR here if you want to check what I mean: #7963

  1. Did we also say we no longer want to update the session token on write? I'm not sure anymore.

@livio-a
Copy link
Member Author

livio-a commented May 17, 2024

@livio-a, I have 2 questions:

  1. If we remove the token from the delete endpoint, it is a breaking change. Do we really want this? The token was already optional.

Sessions can be terminated by either:
- the authenticated user
- a manager, who is granted `session.delete` (e.g. ORG_OWNER) on the authenticated users organisation
- providing the current session_token in the body.

As without the token we can't establish the "authenticated user" and the logic falls back to the session.delete permission. I kind of figured because the OIDC integration tests break, as the CTXLOGIN user does not have the permission through the ORG_USER_MANAGER role for example.

I have a draft PR here if you want to check what I mean: #7963

  1. Did we also say we no longer want to update the session token on write? I'm not sure anymore.
  1. I would keep the token there as is (optional) and only change the update request.
  2. Always update and return the session token.

muhlemmer added a commit that referenced this issue May 22, 2024
# Which Problems Are Solved

The session update requires the current session token as argument.
Since this adds extra complexity but no real additional security and
prevents case like magic links, we want to remove this requirement.

We still require the session token on other resouces / endpoints, e.g.
for finalizing the auth request or on idp intents.

# How the Problems Are Solved

- Removed the session token verifier in the Update Session GRPc call.
- Removed the session token from login UI examples session update calls

# Additional Changes

- none

# Additional Context

- Closes #7883
Copy link

🎉 This issue has been resolved in version 2.53.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants