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

feat: Add create session token given a template slug endpoint #238

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

soulcodex
Copy link

  • Add clerk backend api endpoint implementation to create a session token given a jwt template slug

@soulcodex soulcodex marked this pull request as ready for review February 9, 2024 19:46
@soulcodex soulcodex requested a review from a team as a code owner February 9, 2024 19:46
@soulcodex soulcodex force-pushed the create-session-jwt-from-template-slug branch from c57e0bc to c0b3b0e Compare February 9, 2024 19:50
Copy link
Member

@gkats gkats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution @soulcodex!! 😍

Overall looks great to me. I've left some comments regarding variable naming and an alternative assertion for the tests.

Please remove the integration test, it depends on the instance setup and cannot be trusted. It's not your fault, but ours. 😄

Out of curiosity, what led you to add support in the SDK for this endpoint? Do you intend to use it?

clerk/sessions.go Outdated Show resolved Hide resolved
clerk/sessions.go Outdated Show resolved Hide resolved
tests/integration/sessions_test.go Outdated Show resolved Hide resolved
clerk/sessions_test.go Outdated Show resolved Hide resolved
clerk/sessions_test.go Outdated Show resolved Hide resolved
clerk/sessions_test.go Outdated Show resolved Hide resolved
clerk/sessions_test.go Outdated Show resolved Hide resolved
@soulcodex soulcodex force-pushed the create-session-jwt-from-template-slug branch from c0b3b0e to 0e4a2fc Compare February 12, 2024 17:52
@soulcodex soulcodex force-pushed the create-session-jwt-from-template-slug branch from 0e4a2fc to 327774e Compare February 12, 2024 18:04
@soulcodex
Copy link
Author

soulcodex commented Feb 12, 2024

Thank you for your contribution @soulcodex!! 😍

Overall looks great to me. I've left some comments regarding variable naming and an alternative assertion for the tests.

Please remove the integration test, it depends on the instance setup and cannot be trusted. It's not your fault, but ours. 😄

Out of curiosity, what led you to add support in the SDK for this endpoint? Do you intend to use it?

The changes requested has been made 😉

The main motivation for this proposal is because I'm trying to run on my project an acceptance / E2E tests suite in order to verify that my securized endpoints works as expected.

image

Unfortunately to be able to run E2E tests on my backend I need to accomplish several steps, such as:

  1. Issue a session using the Clerk FAPI
  2. With the session_id (sess_*) and one JWT template slug request a JWT token using the Clerk SDK
  3. Store the JWT token in a Redis during a certain period of time (TTL) to avoid reach the api "rate limiter" issuing the token on each scenario / iteration of the test suite 😞 🎢

However feel free to request any other change but this is basically my adventure / motivation about this PR 😅

@gkats
Copy link
Member

gkats commented Feb 13, 2024

However feel free to request any other change but this is basically my adventure / motivation about this PR 😅

Thanks for providing such detailed information! I just wanted to get a sense of real-world usage patterns.

Thank you for your contribution!

@agis agis enabled auto-merge (rebase) February 13, 2024 11:37
@agis agis disabled auto-merge February 13, 2024 11:38
@agis agis merged commit ef634cd into clerk:main Feb 13, 2024
1 check failed
@soulcodex soulcodex deleted the create-session-jwt-from-template-slug branch February 13, 2024 22:50
@soulcodex
Copy link
Author

Hi @gkats

Thanks for the approve, apart of that I would like to share with you a thread at Clerk Discord channel that also give you a really meaningful context and better explained idea about the PR

Thanks and regards 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants