diff --git a/client/server/login_outcome_test.go b/client/server/login_outcome_test.go index d3b1b7c52..7ebf04f92 100644 --- a/client/server/login_outcome_test.go +++ b/client/server/login_outcome_test.go @@ -8,8 +8,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "google.golang.org/grpc/codes" - gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/proto" @@ -27,9 +25,9 @@ func TestLogin_ManagementUnreachableIsReturnedInsteadOfDemandingSSO(t *testing.T unreachable := errors.New("create connection: dial context: context deadline exceeded") attempts := 0 - s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) { + s.isLoginRequiredFn = func(context.Context) (bool, error) { attempts++ - return internal.StatusLoginFailed, unreachable + return false, unreachable } resp, err := s.Login(userCtx(), &proto.LoginRequest{Username: &username}) @@ -55,15 +53,12 @@ func TestLogin_AuthRefusalStartsSSOFlow(t *testing.T) { s.rootCtx = internal.CtxInitState(context.Background()) breakProfilePrivateKey(t, cfgPath) - refused := gstatus.Error(codes.PermissionDenied, "peer is not registered") - s.loginAttemptFn = func(context.Context, string, string) (internal.StatusType, error) { - return internal.StatusNeedsLogin, refused + s.isLoginRequiredFn = func(context.Context) (bool, error) { + return true, nil } _, err := s.Login(userCtx(), &proto.LoginRequest{Username: &username}) require.Error(t, err) - require.NotErrorIs(t, err, refused, - "the refusal was handed back to the caller instead of starting the SSO flow") status, stateErr := internal.CtxGetState(s.rootCtx).Status() require.NoError(t, stateErr) @@ -71,6 +66,32 @@ func TestLogin_AuthRefusalStartsSSOFlow(t *testing.T) { "the SSO flow setup was never reached with the broken key") } +func TestLogin_SetupKeyStillRunsWhenPeerNeedsLogin(t *testing.T) { + s, _, _, username, _ := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + + s.isLoginRequiredFn = func(context.Context) (bool, error) { + return true, nil + } + + var keysTried []string + s.loginAttemptFn = func(_ context.Context, setupKey, _ string) (internal.StatusType, error) { + keysTried = append(keysTried, setupKey) + return "", nil + } + + setupKey := "A2C8E32F-AEB2-4B45-8FD3-8A0C1B2D3E4F" + resp, err := s.Login(userCtx(), &proto.LoginRequest{Username: &username, SetupKey: setupKey}) + require.NoError(t, err, "the probe's outcome leaked out as the login result") + require.NotNil(t, resp) + require.Equal(t, []string{setupKey}, keysTried, "the setup key never reached the login attempt") + require.Nil(t, s.oauthAuthFlow.flow, "a setup-key login started an SSO flow") + + status, err := internal.CtxGetState(s.rootCtx).Status() + require.NoError(t, err) + require.Equal(t, internal.StatusIdle, status) +} + // breakProfilePrivateKey replaces the profile's private key with an unparseable // one, which makes any attempt to build a Management client fail on the spot. func breakProfilePrivateKey(t *testing.T, cfgPath string) { diff --git a/client/server/server.go b/client/server/server.go index 892c9c5de..eb6a8f2bc 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -140,6 +140,8 @@ type Server struct { // it to drive the login outcomes that need a server on the other end; // production leaves it nil, and every login goes through loginAttempt. loginAttemptFn func(ctx context.Context, setupKey, jwtToken string) (internal.StatusType, error) + + isLoginRequiredFn func(ctx context.Context) (bool, error) } type oauthAuthFlow struct { @@ -384,6 +386,21 @@ func (s *Server) attemptLogin(ctx context.Context, setupKey, jwtToken string) (i return s.loginAttempt(ctx, setupKey, jwtToken) } +func (s *Server) isLoginRequired(ctx context.Context) (bool, error) { + if s.isLoginRequiredFn != nil { + return s.isLoginRequiredFn(ctx) + } + + authClient, err := auth.NewAuth(ctx, s.config.PrivateKey, s.config.ManagementURL, s.config) + if err != nil { + log.Errorf("failed to create auth client: %v", err) + return false, err + } + defer authClient.Close() + + return authClient.IsLoginRequired(ctx) +} + // loginAttempt attempts to login using the provided information. It returns // StatusNeedsLogin when Management refused the peer's credentials and // StatusLoginFailed for every other failure, so callers can tell an @@ -640,22 +657,22 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro s.config = config s.mutex.Unlock() - loginStatus, err := s.attemptLogin(ctx, "", "") - if err == nil { - state.Set(internal.StatusIdle) - return &proto.LoginResponse{}, nil - } - - // Only an authentication refusal means the peer has to (re-)authenticate. - // Any other failure leaves the login undecided: Management unreachable, a + // A probe that errors leaves the login undecided: Management unreachable, a // restart mid-request, an internal error. Those are returned for the caller // to retry, because turning them into an SSO prompt asks the user to solve // something that is not theirs to solve, and a browser login cannot succeed - // while Management is unreachable anyway. - if loginStatus != internal.StatusNeedsLogin { - state.Set(loginStatus) + // while Management is unreachable anyway. Only Management refusing the + // peer's key is a decision, and IsLoginRequired reports that as + // needsLogin=true rather than an error. + needsLogin, err := s.isLoginRequired(ctx) + if err != nil { + state.Set(internal.StatusLoginFailed) return nil, err } + if !needsLogin { + state.Set(internal.StatusIdle) + return &proto.LoginResponse{}, nil + } if msg.SetupKey == "" { hint := ""