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

presence detection callback #2048

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

James-Pickett
Copy link
Contributor

@James-Pickett James-Pickett commented Jan 14, 2025

This PR updates launcher to send a separate callbacks for presence detection requests. They function as follows:

  1. send a callback immediately when presence detection begins
  2. on an interval (30s currently) send a callback letting server side know we're still waiting on user
  3. send callback when presence detection is completed

It also updates all the components of presence detection to have a unified timeout that will close the prompt (at least on MacOS) after the time out.

Questions and/or follow on PR

  1. Should we send a callback on failure?
  2. Do we want to have specific callback url for presence detection (will need to work with k2 on that)
  3. Do we come up with some formal codes for presence detection results, ie

0 = success
1 = failure
2 = timeout

@James-Pickett James-Pickett changed the title make presence detection call back part of krypto wrap presence detection callback Jan 14, 2025
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
@@ -222,6 +226,14 @@ func (e *kryptoEcMiddleware) Wrap(next http.Handler) http.Handler {
// so just add the session id manually
context.WithValue(callbackReq.Context(), multislogger.KolideSessionIdKey, kolideSessionId[0]),
)

// here we want to create an additional callback for presence detection, but it's only going to be valid if we have the session (saml) id
presenceDetectionCallbackReq := callbackReq.Clone(context.WithoutCancel(callbackReq.Context()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the rails side would be easier if we used a different URL. Probably okay to try either way

@James-Pickett James-Pickett marked this pull request as ready for review January 21, 2025 21:40
Copy link
Contributor

@RebeccaMahany RebeccaMahany left a comment

Choose a reason for hiding this comment

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

Read through it but I think I need a little bit more time to absorb it -- left a couple small comments in the meantime, will come back to it tomorrow AM. Overall LGTM!

ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
Copy link
Contributor

@RebeccaMahany RebeccaMahany left a comment

Choose a reason for hiding this comment

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

Okay, I took another look at it today. I like this approach and I think it makes sense!

ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Show resolved Hide resolved
Copy link
Contributor

@RebeccaMahany RebeccaMahany left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

ee/localserver/krypto-ec-middleware-helpers.go Outdated Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Show resolved Hide resolved
ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved
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