fix(server/authn): Re-order auth flow to prevent side effects on handleCookieAuth

[gregfurman]

Description

Currently, any HTTP request to a route that accepts cookie-based auth creates a new entry in the UserSession table, even when the request is ultimately authenticated via a different strategy (e.g. bearer auth using HATCHET_CLIENT_API_TOKEN).

This happens because handleCookieAuth saves an unauthenticated session as a side effect before falling through to the next auth strategy in the chain. For programmatic clients making many bearer-auth requests, this produces an unauthenticated UserSession row per request that is never referenced again and never cleaned up.

Hence, we should only create new UserSession entries when the request is actually attempting cookie-based auth.

Partial fix for #3913. See original issue for reproduction steps.

Type of change

• Bug fix (non-breaking change which fixes an issue)

What's Changed

• When multiple authentication types are possible, ensure cookieAuth occurs last.

[vercel]

@gregfurman is attempting to deploy a commit to the Hatchet Team on Vercel.

A member of the Team first needs to authorize it.

[gregfurman]

❓ Should we be including a migration to clean-up orphaned session entries (i.e those with userId = NULL) in the UserSession table?

[abelanger5]

Are we sure this is correct? There are some endpoints where we would like to call Set-Cookie, for example when beginning an oauth flow, we need to store the oauth state parameter.

It's quite possible we don't hit this case on any such endpoints, in which case I wonder why we call SaveUnauthenticated later on (which we do a few lines later). Though it's possible that the error block is there so that we can invalidate a cookie which can no longer be decrypted.

[gregfurman]

Are we sure this is correct?

Not entirely 🫠 Since the bug here is that we shouldn't be creating a new UserSession entry when there is no session to be persisted -- which will always occur regardless of the auth type since we assume cookie auth whose fallback has the aforementioned side-effect of a new row being added.

I could rejig the logic here to instead start off assuming bearer auth (which has no side-effects) i.e from cookie -> bearer -> custom to bearer -> custom -> cookie

It's quite possible we don't hit this case on any such endpoints, in which case I wonder why we call SaveUnauthenticated later on (which we do a few lines later).

At a glance, I couldn't really figure out how to induce a fallback to SaveUnauthenticated which makes me think this handler logic deserves a revisit.

For this PR, let me not make any changes to the individual auth handler logic and instead try the aforementioned handler re-ordering. Wdyt?

[abelanger5]

❓ Should we be including a migration to clean-up orphaned session entries (i.e those with userId = NULL) in the UserSession table?

I think a cron job to clean up those entries is not unreasonable, though note that not every session which has userId = NULL should necessarily be cleaned up (again for the purposes of the oauth flow). So it seems like we should clean up when:

1. The session has expired (assuming we have no mechanism to refresh a session, which I don't think we do), or
2. The session has userId = NULL and was created before a certain time, like older than 24 hours ago

[gregfurman]

I think a cron job to clean up those entries is not unreasonable, though note that not every session which has userId = NULL should necessarily be cleaned up (again for the purposes of the oauth flow).

Ah yes. I omitted the expiredAt constraint as well. Sessions that have expired are already cleaned up on logout due to our Save logic:

hatchet/pkg/auth/cookie/sessionstore.go

Lines 218 to 226 in 26a44c2

	if session.Options.MaxAge < 0 {
	if session.ID != "" {
	sessionID, parseErr := uuid.Parse(session.ID)
	if parseErr == nil {
	if _, delErr := repo.Delete(r.Context(), sessionID); delErr != nil && !errors.Is(delErr, pgx.ErrNoRows) {
	store.l.Error().Err(delErr).Msg("user session delete failed during logout; clearing browser cookie anyway")
	}
	}
	}

Granted, there are probably some edge-cases here like a user never logging back in after their session expired which would probably just leave the expired session in the table. Since there isn't really a way of determining whether a session entry was incorrectly included on a token-authenticated request, perhaps a cron makes sense.

Do we have a preference on whether to manage cleanups in the Go code or would a pg_cron be better suited? Suppose this could be included in the DataRetention cleanup as well?

Also, I re-worked the logic here based on #3923 (comment) -- lmk if we're happy with the approach!

[abelanger5]

Do we have a preference on whether to manage cleanups in the Go code or would a pg_cron be better suited? Suppose this could be included in the DataRetention cleanup as well?

Definitely would prefer to manage this in Go code, we don't want to depend on postgres extensions if we can avoid them. I think a simple go-cron on the API layer would be fine. I don't think we have this on any of the API surface yet, only on the engine, but those should be a good reference.