Files
netbird/client/server/login_account_test.go
T
Zoltan Papp fe0e9042ad [client] Drop the pending login flow when a login switches the profile (#7882)
* [client] Force interactive login when extending the auth session

A session extend must be answered from the account the peer is registered
under. With a silent PKCE flow (DisablePromptLogin or max_age=0) the IdP
answers from whatever session it already holds, which need not be the
peer's account when several are signed in; the token then fails the
user match in ExtendAuthSession with no way to pick another account.

Mark the PKCE flow request as a session extend so the management server
can force prompt=login for it, overriding the configured silent flow.

* [client] Reduce cognitive complexity of Server.Login

Login sat at cognitive complexity 27, over the 25 the linter allows.

Extract the interactive SSO branch into startSSOLogin, and split the
nested in-flight-flow reuse check out of it into reuseOAuthFlow, which
flattens the original if/else into early returns: it returns the cached
auth info when the previous flow targets the same client and still has
more than 90s left, otherwise cancels the stale wait and returns nil so
the caller requests a fresh flow.

The helpers take the contextState through a small statusSetter
interface, since internal.contextState is unexported and re-deriving it
with CtxGetState inside the helper would resolve against callerCtx
rather than rootCtx.

No behavior change: same ordering of state transitions, same mutex scope
around the oauthAuthFlow write, same error paths. Login is now at 21.

* [client] Respect DisablePromptLogin when extending the auth session

Forcing prompt=login on a session extend overrode DisablePromptLogin, which
is set for IdPs that break on it: Authentik triggers a double authentication
and social logins fail outright. Overriding it there trades a recoverable
extend for a login that cannot complete at all.

Keep the LoginFlag override, which only replaces max_age=0 or none with
prompt=login so the IdP honours login_hint, and leave DisablePromptLogin as
configured. Those deployments keep the silent flow, and with several accounts
signed in an extend answered from the wrong one still fails the user match.

* [client] Guard the shared OAuth flow state with the server mutex

reuseOAuthFlow read flow, expiresAt, waitCancel and info without holding
s.mutex, while startSSOLogin and WaitSSOLogin write them under it. Reading the
fields one at a time could also answer with auth info from a flow that was
already replaced, or cancel a wait that no longer belongs to the flow just
judged stale. Take one snapshot under the lock and decide from it.

WaitSSOLogin read oauthAuthFlow.flow twice outside the lock; both now use a
value snapshotted in the critical section that already installs actCancel.

Its stale waitCancel was read and called in a separate section from the one
installing the new one, so two racing calls could read the same predecessor and
leave one wait uncancelled. Swap the two in a single critical section. Both
cancels run after unlocking: the displaced wait takes s.mutex as it unwinds.

* [client] Verify the SSO login came back for the hinted account

login_hint is a suggestion the IdP may ignore: with a silent flow configured
(DisablePromptLogin or max_age=0) and a live IdP session for another account,
the login completes with that account's token. On a registered peer the
management server rejects it as a user mismatch, but on a fresh profile the
peer silently registers under the wrong account and the profile is then bound
to it — every later login follows the stored hint straight back.

After the token exchange, compare the ID token's email against the hint the
flow was sent with. On a mismatch, do not log in to management with the token;
run one more round asking the IdP to re-decide the account (prompt=login, via
ForceAccountPrompt — DisablePromptLogin still wins there). If the prompted
round also comes back different, proceed with a warning: the address may
legitimately have changed, and refusing forever would lock the user out of the
profile while the management server still rejects a token that does not own
the peer. A token or profile with no email to compare is not judged.

The retry differs per platform because of who opens the browser:

- CLI (netbird login foreground) and Android run the whole flow in one
  process, so the mismatch retries automatically: the browser reopens with
  the account prompt within the same login attempt.
- On desktop the login is split between the daemon and the GUI: Login hands
  the authorize URL to the GUI, WaitSSOLogin blocks for the token, and only
  the GUI can open a browser. A new URL cannot be handed out from inside
  WaitSSOLogin (its response has no field for one, kept that way to avoid a
  proto change), so the daemon arms forceAccountPrompt, fails the round with
  "connect again to choose the account", and builds the next Login's flow
  with the prompt — the user's next connect is the retry.

The flag and the flow annotations live in daemon memory only; SwitchProfile
drops them so the previous profile's hint cannot judge the next profile's
token. The device code flow has no prompt parameter (RFC 8628), so a prompted
round there runs as-is and a repeated mismatch is let through with the
warning rather than looping.

* [client] Address review comments on PKCE session extend flow

Fail the PKCE authorization flow test on request error instead of
continuing into a nil dereference, and make the godoc comments on the
touched exported symbols identifier-leading full sentences.

* [client] Match accounts only on the email claim of the ID token

The name-claim fallback in the ID token parsing is kept for the login
hint and display, but account matching now only considers a value that
came from the email claim, so a token without one no longer produces a
false account mismatch.

* [client] Drop the pending session extend on a profile switch

The profile-switch cleanup dropped the pending login flow and the
account-prompt flag, but left extendAuthSessionFlow untouched. Its device
code was issued by the previous profile's IdP client, so a
WaitExtendAuthSession still parked on the browser leg would submit the
resulting token against the new profile's engine.

* [client] Judge the SSO account against the flow that produced the token

WaitSSOLogin snapshotted the flow on entry but re-read the info, hint and
accountPrompted from the live s.oauthAuthFlow afterwards, in separate
critical sections. WaitToken blocks for the whole browser leg, so a
concurrent Login or RequestJWTAuth could replace the flow meanwhile and
the mismatch check would compare this wait's token against another flow's
account: either arming the prompt spuriously or letting a wrong-account
token through against an unrelated profile's hint. Take all of it in the
entry snapshot.

* [client] Keep the forced account prompt from being lost to flow reuse

startSSOLogin consumed forceAccountPrompt and applied the prompt to the
freshly built flow, but reuseOAuthFlow could then answer from a cached
flow for the same client — one built without prompt=login, e.g. by
RequestJWTAuth. The user got the same silent authorization URL that
produced the mismatch, with the flag already spent, so no later round
asked either. Rule reuse out when the prompt is forced, while still
cancelling the predecessor's wait.

RequestJWTAuth also wrote the flow fields one by one, leaving the previous
login's hint and accountPrompted behind for WaitSSOLogin to judge a later
token against. Both sites now replace the whole record.

* [client] Consume the forced account prompt after the retry

forceAccountPrompt was never cleared, so a flow that outlived the retry it
was armed for kept sending prompt=login on every later authorization
request and re-authenticated the user each time. RequestAuthInfo now takes
the flag as it builds the request.

* [client] Cancel the caller context in the SSO login tests

WaitSSOLogin parks a goroutine on the caller's context for the whole
browser leg. The tests passed context.Background(), which never cancels,
so each left one goroutine behind for the lifetime of the test binary.

* [client] Cancel the wait displaced by an OAuth flow replacement

Replacing the shared record with a whole struct value dropped the previous
flow's waitCancel, so an SSO browser wait still parked on it lost its
cancel: nothing could preempt it, and it could go on to run attemptLogin
or mutate the record behind the new flow. Both replacement sites now take
the displaced cancel over in the same critical section, via a shared
replaceOAuthFlow, and invoke it after the unlock.

* [client] Guard OAuth flow mutations by the flow that owns the wait

* [client] Arm the account prompt only from the wait that owns the flow

* [client] Drop the pending login flow when a login switches the profile

SwitchProfile cancels the pending OAuth wait and clears the flow record,
the account-prompt flag and the pending session extend, because they
describe the previous profile's login. A Login or Up that carries a
ProfileName switches the profile through switchProfileIfNeeded without
that cleanup, so reuseOAuthFlow could hand the new profile the previous
profile's flow: the same IdP client ID, the previous account's
login_hint in the URL, and a record whose hint WaitSSOLogin would judge
the new profile's token against. That either fails the login with a
spurious account mismatch or lets a fresh peer register under the other
account.

switchProfileIfNeeded now reports whether it switched, and its callers
run the same cleanup on a switch. A login on the same profile keeps the
pending flow, so a second client can still join it.

* [client] Clear the JWT cache when a login switches the profile

The daemon keeps the user's JWT in a cache for SSH logins. When the user
switches to another profile, this token belongs to the old profile, so it
must not be used for the new one.

SwitchProfile already cleared the cache. A login or up request can also
switch the profile, but it did not clear the cache. After such a switch,
SSH could still use the old profile's token, and a sign-in still running
for the old profile could save its token into the new profile's cache.

Clear the cache in dropPendingAuthFlows, so every profile switch does it.

* [client] Bump the JWT cache generation with the config swap in Login

RequestJWTAuth snapshots s.config and the cache generation under one
s.mutex section and relies on the two flipping together. Login cleared
the cache right after the profile switch but replaced s.config only
later, after getConfig, so a JWT flow started in between carried the
previous profile's config with the new generation, and its token landed
in the cache the new profile then served.

The swap now bumps the generation under the same lock. The early drop
stays: it covers a login that fails after the switch, where a retry no
longer sees a profile change.
2026-10-09 12:56:41 +02:00

333 lines
12 KiB
Go

package server
import (
"context"
"errors"
"path/filepath"
"testing"
"time"
"github.com/stretchr/testify/require"
"github.com/netbirdio/netbird/client/internal"
"github.com/netbirdio/netbird/client/internal/auth"
"github.com/netbirdio/netbird/client/internal/ipcauth"
"github.com/netbirdio/netbird/client/internal/profilemanager"
"github.com/netbirdio/netbird/client/mdm"
"github.com/netbirdio/netbird/client/proto"
)
type jwtStoringMDMFetcher struct {
cache *jwtCache
caller ipcauth.Identity
generation uint64
}
func (f *jwtStoringMDMFetcher) Fetch() map[string]any {
f.generation = f.cache.currentGeneration()
f.cache.store("previous-profile-token", f.caller, time.Minute, f.generation)
return nil
}
type stubOAuthFlow struct {
token auth.TokenInfo
onWait func()
}
func (f *stubOAuthFlow) RequestAuthInfo(context.Context) (auth.AuthFlowInfo, error) {
return auth.AuthFlowInfo{}, nil
}
func (f *stubOAuthFlow) WaitToken(context.Context, auth.AuthFlowInfo) (auth.TokenInfo, error) {
if f.onWait != nil {
f.onWait()
}
return f.token, nil
}
func (f *stubOAuthFlow) GetClientID(context.Context) string {
return "stub-client"
}
func TestWaitSSOLogin_WrongAccountArmsPromptAndFails(t *testing.T) {
s := newSSOTestServer(t, "user@example.com", false, "other@example.com")
attempts := 0
s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) {
attempts++
return "", nil
}
resp, err := s.WaitSSOLogin(callerCtx(t), &proto.WaitSSOLoginRequest{UserCode: "code"})
require.Error(t, err)
require.Nil(t, resp)
require.Equal(t, 0, attempts, "the wrong account's token reached the management login")
require.True(t, s.forceAccountPrompt, "the next login was not armed to ask for the account")
require.Nil(t, s.oauthAuthFlow.flow, "the mismatched flow stayed cached for reuse")
status, stateErr := internal.CtxGetState(s.rootCtx).Status()
require.NoError(t, stateErr)
require.Equal(t, internal.StatusNeedsLogin, status, "the mismatch must stay retryable")
}
func TestWaitSSOLogin_WrongAccountAfterPromptProceeds(t *testing.T) {
s := newSSOTestServer(t, "user@example.com", true, "other@example.com")
attempts := 0
s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) {
attempts++
return "", nil
}
resp, err := s.WaitSSOLogin(callerCtx(t), &proto.WaitSSOLoginRequest{UserCode: "code"})
require.NoError(t, err, "a prompted round must not error again on a mismatch")
require.NotNil(t, resp)
require.Equal(t, "other@example.com", resp.Email)
require.Equal(t, 1, attempts)
require.False(t, s.forceAccountPrompt)
}
func TestWaitSSOLogin_MatchingAccountProceeds(t *testing.T) {
s := newSSOTestServer(t, "user@example.com", false, "User@Example.com")
attempts := 0
s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) {
attempts++
return "", nil
}
resp, err := s.WaitSSOLogin(callerCtx(t), &proto.WaitSSOLoginRequest{UserCode: "code"})
require.NoError(t, err)
require.NotNil(t, resp)
require.Equal(t, 1, attempts)
require.False(t, s.forceAccountPrompt)
}
func TestWaitSSOLogin_NoHintIsNotJudged(t *testing.T) {
s := newSSOTestServer(t, "", false, "whoever@example.com")
attempts := 0
s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) {
attempts++
return "", nil
}
_, err := s.WaitSSOLogin(callerCtx(t), &proto.WaitSSOLoginRequest{UserCode: "code"})
require.NoError(t, err)
require.Equal(t, 1, attempts)
require.False(t, s.forceAccountPrompt)
}
func TestSwitchProfile_DropsAccountPromptAndPendingFlow(t *testing.T) {
s, ctx, _, _, _ := setupServerWithProfile(t)
s.forceAccountPrompt = true
cancelled := false
s.oauthAuthFlow = oauthAuthFlow{
flow: &stubOAuthFlow{},
hint: "user@example.com",
waitCancel: func() { cancelled = true },
}
extendCancelled := false
s.extendAuthSessionFlow.Set(&stubOAuthFlow{}, auth.AuthFlowInfo{DeviceCode: "device"})
s.extendAuthSessionFlow.SetWaitCancel(func() { extendCancelled = true })
_, err := s.SwitchProfile(ctx, nil)
require.NoError(t, err)
require.False(t, s.forceAccountPrompt, "the prompt flag leaked across a profile switch")
require.Nil(t, s.oauthAuthFlow.flow, "the previous profile's flow leaked across a profile switch")
require.Empty(t, s.oauthAuthFlow.hint)
require.True(t, cancelled, "the pending wait was not cancelled")
require.True(t, extendCancelled, "the pending extend wait was not cancelled")
_, _, pending := s.extendAuthSessionFlow.Get()
require.False(t, pending, "the previous profile's extend flow leaked across a profile switch")
}
func TestLogin_ProfileSwitchDropsAccountPromptAndPendingFlow(t *testing.T) {
s, _, _, username, cfgPath := setupServerWithProfile(t)
s.rootCtx = internal.CtxInitState(context.Background())
s.isLoginRequiredFn = func(context.Context) (bool, error) {
return true, nil
}
other := "other-profile"
otherPath := filepath.Join(filepath.Dir(cfgPath), other+".json")
_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
ConfigPath: otherPath,
ManagementURL: "https://api.netbird.io:443",
})
require.NoError(t, err)
breakProfilePrivateKey(t, otherPath)
s.forceAccountPrompt = true
cancelled := false
s.oauthAuthFlow = oauthAuthFlow{
flow: &stubOAuthFlow{},
hint: "user@example.com",
waitCancel: func() { cancelled = true },
}
extendCancelled := false
s.extendAuthSessionFlow.Set(&stubOAuthFlow{}, auth.AuthFlowInfo{DeviceCode: "device"})
s.extendAuthSessionFlow.SetWaitCancel(func() { extendCancelled = true })
generation := s.jwtCache.currentGeneration()
_, err = s.Login(userCtx(), &proto.LoginRequest{ProfileName: &other, Username: &username})
require.Error(t, err, "the broken key must stop the login before a flow is built")
active, err := s.profileManager.GetActiveProfileState()
require.NoError(t, err)
require.Equal(t, profilemanager.ID(other), active.ID, "the login did not switch the profile")
require.False(t, s.forceAccountPrompt, "the prompt flag leaked across a login-driven profile switch")
require.Nil(t, s.oauthAuthFlow.flow, "the previous profile's flow leaked across a login-driven profile switch")
require.Empty(t, s.oauthAuthFlow.hint)
require.True(t, cancelled, "the pending wait was not cancelled")
require.True(t, extendCancelled, "the pending extend wait was not cancelled")
_, _, pending := s.extendAuthSessionFlow.Get()
require.False(t, pending, "the previous profile's extend flow leaked across a login-driven profile switch")
require.Greater(t, s.jwtCache.currentGeneration(), generation, "the previous profile's JWT cache survived a login-driven profile switch")
}
func TestLogin_ProfileSwitchRejectsJWTObtainedUnderPreviousConfig(t *testing.T) {
s, _, _, username, cfgPath := setupServerWithProfile(t)
s.rootCtx = internal.CtxInitState(context.Background())
s.isLoginRequiredFn = func(context.Context) (bool, error) {
return false, errors.New("stop once the config is swapped")
}
other := "other-profile"
_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
ConfigPath: filepath.Join(filepath.Dir(cfgPath), other+".json"),
ManagementURL: "https://api.netbird.io:443",
})
require.NoError(t, err)
// getConfig loads the MDM policy right before Login swaps s.config, so the
// fetcher runs where a RequestJWTAuth racing the login would land: after the
// switch dropped the previous profile's state, while s.config still belongs
// to the previous profile. It caches a token under the generation current at
// that point, as WaitJWTToken would.
generation := s.jwtCache.currentGeneration()
fetcher := &jwtStoringMDMFetcher{cache: s.jwtCache, caller: unprivilegedIdentity()}
s.mdmLoader = mdm.NewLoader(fetcher)
_, err = s.Login(userCtx(), &proto.LoginRequest{ProfileName: &other, Username: &username})
require.Error(t, err)
require.Greater(t, fetcher.generation, generation, "the token was not cached after the switch dropped the previous profile's state")
_, found := s.jwtCache.get(fetcher.caller)
require.False(t, found, "a JWT obtained under the previous profile's config survived the login-driven switch")
}
func TestLogin_SameProfileKeepsPendingFlow(t *testing.T) {
s, _, profName, username, cfgPath := setupServerWithProfile(t)
s.rootCtx = internal.CtxInitState(context.Background())
s.isLoginRequiredFn = func(context.Context) (bool, error) {
return true, nil
}
breakProfilePrivateKey(t, cfgPath)
cancelled := false
flow := &stubOAuthFlow{}
s.oauthAuthFlow = oauthAuthFlow{
flow: flow,
hint: "user@example.com",
waitCancel: func() { cancelled = true },
}
_, err := s.Login(userCtx(), &proto.LoginRequest{ProfileName: &profName, Username: &username})
require.Error(t, err, "the broken key must stop the login before a flow is built")
require.Equal(t, flow, s.oauthAuthFlow.flow, "a login on the same profile dropped the flow a second client could join")
require.Equal(t, "user@example.com", s.oauthAuthFlow.hint)
require.False(t, cancelled, "a login on the same profile cancelled the pending wait")
}
func TestWaitSSOLogin_JudgesTheFlowThatProducedTheToken(t *testing.T) {
s := newSSOTestServer(t, "user@example.com", false, "user@example.com")
attempts := 0
s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) {
attempts++
return "", nil
}
flow := s.oauthAuthFlow.flow.(*stubOAuthFlow)
flow.onWait = func() {
s.mutex.Lock()
defer s.mutex.Unlock()
s.oauthAuthFlow.hint = "someone-else@example.com"
}
resp, err := s.WaitSSOLogin(callerCtx(t), &proto.WaitSSOLoginRequest{UserCode: "code"})
require.NoError(t, err, "a flow replaced mid-wait must not decide this wait's verdict")
require.NotNil(t, resp)
require.Equal(t, 1, attempts)
require.False(t, s.forceAccountPrompt, "the prompt was armed off another flow's hint")
}
func TestReuseOAuthFlow_ForcedPromptRefusesTheCachedFlow(t *testing.T) {
s := New(internal.CtxInitState(context.Background()), "console", "", false, false, false, false)
cancelled := false
s.oauthAuthFlow = oauthAuthFlow{
flow: &stubOAuthFlow{},
info: auth.AuthFlowInfo{UserCode: "code"},
expiresAt: time.Now().Add(time.Hour),
waitCancel: func() { cancelled = true },
}
state := internal.CtxGetState(s.rootCtx)
resp := s.reuseOAuthFlow(context.Background(), &stubOAuthFlow{}, state, true)
require.Nil(t, resp, "a forced account prompt reused the flow that skipped it")
require.True(t, cancelled, "the predecessor wait was orphaned")
resp = s.reuseOAuthFlow(context.Background(), &stubOAuthFlow{}, state, false)
require.NotNil(t, resp, "an unforced login stopped reusing a live flow")
require.Equal(t, "code", resp.UserCode)
}
func TestReplaceOAuthFlow_CancelsTheDisplacedWait(t *testing.T) {
s := New(internal.CtxInitState(context.Background()), "console", "", false, false, false, false)
cancelled := false
s.oauthAuthFlow = oauthAuthFlow{
flow: &stubOAuthFlow{},
info: auth.AuthFlowInfo{UserCode: "code"},
expiresAt: time.Now().Add(time.Hour),
hint: "user@example.com",
accountPrompted: true,
waitCancel: func() { cancelled = true },
}
next := &stubOAuthFlow{}
s.replaceOAuthFlow(oauthAuthFlow{flow: next, info: auth.AuthFlowInfo{UserCode: "next"}})
require.True(t, cancelled, "the displaced wait was left without an owner")
require.Equal(t, next, s.oauthAuthFlow.flow)
require.Equal(t, "next", s.oauthAuthFlow.info.UserCode)
require.Empty(t, s.oauthAuthFlow.hint, "the previous flow's hint survived the replacement")
require.False(t, s.oauthAuthFlow.accountPrompted)
require.Nil(t, s.oauthAuthFlow.waitCancel, "the consumed cancel stayed on the record")
}
func newSSOTestServer(t *testing.T, hint string, accountPrompted bool, tokenEmail string) *Server {
t.Helper()
s := New(internal.CtxInitState(context.Background()), "console", "", false, false, false, false)
s.oauthAuthFlow = oauthAuthFlow{
flow: &stubOAuthFlow{token: auth.TokenInfo{Email: tokenEmail, EmailClaim: tokenEmail}},
info: auth.AuthFlowInfo{UserCode: "code"},
expiresAt: time.Now().Add(time.Minute),
hint: hint,
accountPrompted: accountPrompted,
}
return s
}
// callerCtx is the gRPC caller's context. WaitSSOLogin parks a goroutine on it
// for the whole browser leg, so a test that never cancels leaks one.
func callerCtx(t *testing.T) context.Context {
t.Helper()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
return ctx
}