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(backend): Introduce handshake & extract logic auth logic to separate classes #45

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Feb 16, 2024

Changes made in this PR

  • Drop Clerk::SDK#interstitial method (it should not be used by any customer since this is internal)
  • Replace the interstitial mechanism with the handshake mechanism
  • Extract logic from the RackMiddlewareV2 to AuthenticateRequest class
  • Extract data used from the request and the configuration to AuthenticateContext class (Parameter object pattern)
  • Introduce constants module to centralize and re-use the constants value used across the SDK

Customer impact

  • Interstitial HTML page rendered when we cannot determine the request state is replaced with 2 HTTP 307 redirects
  • Introduced CLERK_PUBLISHABLE_KEY env which is required for the handshake to work properly
  • CLERK_SECRET_KEY should use the format sk_test | sk_live format

TODO

  • Add tests
  • Check all uses cases

Next Steps

  • Support multi-domain logic

@dimkl dimkl requested review from agis and SokratisVidros February 16, 2024 11:24
@dimkl dimkl self-assigned this Feb 16, 2024
def resolve_cookie_token(env)
# in cross-origin XHRs the use of Authorization header is mandatory.
# TODO: add reason
return signed_out if auth_context.cross_origin_request?
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Shouldn't this check go to the resolve_header_token method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the condition used to execute the resolve_header_token flow is the existence of a header_token and the case described here is that we want to return a signed_out state if it's a cross-origin request and there is no header_token, it will never be triggered in the resolve_header_token flow.

@dimkl dimkl force-pushed the introduce-handshake branch 4 times, most recently from e311ddf to e7ef512 Compare February 20, 2024 16:26
@dimkl dimkl requested a review from SokratisVidros February 20, 2024 16:27
Copy link

@adentaofftake adentaofftake left a comment

Choose a reason for hiding this comment

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

lot of this goes over my head. If it works, I'm excited!

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.

Looks good to me overall, but I admit that the handshake implementation is a bit hard to follow.

Might be just me though.

Great work on a tough problem!

@dimkl dimkl force-pushed the introduce-handshake branch from dd05709 to c7ef5f1 Compare February 22, 2024 11:00
@dimkl dimkl merged commit 38ee71e into main Feb 22, 2024
2 checks passed
@dimkl dimkl deleted the introduce-handshake branch February 22, 2024 12:32
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.

5 participants