Client + Server: Refactor adding a server by first adding then auth

Otherwise we go to the main state after auth is done and the server is
not yet added

5 files changed, 52 insertions, 51 deletions

diff --git a/CHANGES.md b/CHANGES.md
index aaaf0ea..f7c0e2c 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,5 +1,6 @@
 # Not released
 * Expose default gateway in profile settings too. For clients, use the default gateway set on the config object, this is maybe only useful for suggesting some profiles in the client profile chooser UI
+* Add a server internally before authorizing and remove it again if authorization has failed. This makes sure the internet state is always up-to-date with what is happening. This also allows us to move to the main state when authorization is done as previously it could be the case where authorization was done but the server was not added yet
 
 # 1.99.1 (2024-03-11)
 * Disable type annotation for global eduVPN class as it gave a `SyntaxError` on some Python versions. See https://bugs.python.org/issue34939
diff --git a/client/client.go b/client/client.go
index 127c790..7cf1e8b 100644
--- a/client/client.go
+++ b/client/client.go
@@ -338,17 +338,17 @@ func (c *Client) AddServer(ck *cookie.Cookie, identifier string, _type srvtypes.
 
 	switch _type {
 	case srvtypes.TypeInstituteAccess:
-		_, err = c.Servers.AddInstitute(ck.Context(), c.cfg.Discovery(), identifier, ni)
+		err = c.Servers.AddInstitute(ck.Context(), c.cfg.Discovery(), identifier, ni)
 		if err != nil {
 			return i18nerr.Wrapf(err, "The institute access server with URL: '%s' could not be added", identifier)
 		}
 	case srvtypes.TypeSecureInternet:
-		_, err = c.Servers.AddSecure(ck.Context(), c.cfg.Discovery(), identifier, ni)
+		err = c.Servers.AddSecure(ck.Context(), c.cfg.Discovery(), identifier, ni)
 		if err != nil {
 			return i18nerr.Wrapf(err, "The secure internet server with organisation ID: '%s' could not be added", identifier)
 		}
 	case srvtypes.TypeCustom:
-		_, err = c.Servers.AddCustom(ck.Context(), identifier, ni)
+		err = c.Servers.AddCustom(ck.Context(), identifier, ni)
 		if err != nil {
 			return i18nerr.Wrapf(err, "The custom server with URL: '%s' could not be added", identifier)
 		}
diff --git a/internal/server/custom.go b/internal/server/custom.go
index 10e9a28..d2679cf 100644
--- a/internal/server/custom.go
+++ b/internal/server/custom.go
@@ -2,7 +2,6 @@ package server
 
 import (
 	"context"
-	"time"
 
 	"github.com/eduvpn/eduvpn-common/internal/api"
 	"github.com/eduvpn/eduvpn-common/internal/config/v2"
@@ -14,7 +13,7 @@ import (
 // `ctx` is the context used for cancellation
 // `id` is the identifier of the server, the base URL
 // `na` specifies whether or not we want to add the server without doing authorization now
-func (s *Servers) AddCustom(ctx context.Context, id string, na bool) (*Server, error) {
+func (s *Servers) AddCustom(ctx context.Context, id string, na bool) error {
 	sd := api.ServerData{
 		ID:         id,
 		Type:       server.TypeCustom,
@@ -22,25 +21,24 @@ func (s *Servers) AddCustom(ctx context.Context, id string, na bool) (*Server, e
 		BaseAuthWK: id,
 	}
 
-	var a *api.API
-	var err error
-	if !na {
-		// Authorize by creating the API object
-		a, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	err = s.config.AddServer(id, server.TypeCustom, v2.Server{LastAuthorizeTime: time.Now()})
+	err := s.config.AddServer(id, server.TypeCustom, v2.Server{})
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	cust := s.NewServer(id, server.TypeCustom, a)
-	// Return the server with the API
+	// no authorization should be triggered, return
+	if na {
+		return nil
+	}
 
-	return &cust, nil
+	// Authorize by creating the API object
+	_, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
+	if err != nil {
+		// authorization has failed, remove the server again
+		s.config.RemoveServer(id, server.TypeCustom)
+		return err
+	}
+	return nil
 }
 
 // GetCustom gets a custom server
diff --git a/internal/server/institute.go b/internal/server/institute.go
index aa032c7..195c2ef 100644
--- a/internal/server/institute.go
+++ b/internal/server/institute.go
@@ -2,7 +2,6 @@ package server
 
 import (
 	"context"
-	"time"
 
 	"github.com/eduvpn/eduvpn-common/internal/api"
 	"github.com/eduvpn/eduvpn-common/internal/config/v2"
@@ -16,11 +15,11 @@ import (
 // `disco` are the discovery servers
 // `id` is the identifier for the server, the base url
 // `na` is true when authorization should not be triggered
-func (s *Servers) AddInstitute(ctx context.Context, disco *discovery.Discovery, id string, na bool) (*Server, error) {
+func (s *Servers) AddInstitute(ctx context.Context, disco *discovery.Discovery, id string, na bool) error {
 	// This is basically done to double check if the server is part of the institute access section of disco
 	dsrv, err := disco.ServerByURL(id, "institute_access")
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	sd := api.ServerData{
@@ -30,22 +29,24 @@ func (s *Servers) AddInstitute(ctx context.Context, disco *discovery.Discovery,
 		BaseAuthWK: dsrv.BaseURL,
 	}
 
-	var a *api.API
-	if !na {
-		// Authorize by creating the API object
-		a, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
-		if err != nil {
-			return nil, err
-		}
+	err = s.config.AddServer(dsrv.BaseURL, server.TypeInstituteAccess, v2.Server{})
+	if err != nil {
+		return err
 	}
 
-	err = s.config.AddServer(dsrv.BaseURL, server.TypeInstituteAccess, v2.Server{LastAuthorizeTime: time.Now()})
-	if err != nil {
-		return nil, err
+	// no authorization should be triggered, return
+	if na {
+		return nil
 	}
 
-	inst := s.NewServer(dsrv.BaseURL, server.TypeInstituteAccess, a)
-	return &inst, nil
+	// Authorize by creating the API object
+	_, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
+	if err != nil {
+		// authorization has failed, remove the server again
+		s.config.RemoveServer(dsrv.BaseURL, server.TypeInstituteAccess)
+		return err
+	}
+	return nil
 }
 
 // GetInstitute gets an institute access server
diff --git a/internal/server/secureinternet.go b/internal/server/secureinternet.go
index 0d06a55..3377650 100644
--- a/internal/server/secureinternet.go
+++ b/internal/server/secureinternet.go
@@ -3,7 +3,6 @@ package server
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/eduvpn/eduvpn-common/internal/api"
 	"github.com/eduvpn/eduvpn-common/internal/config/v2"
@@ -18,9 +17,9 @@ import (
 // `disco` are the discovery servers
 // `orgID` is the organiztaion ID
 // `na` specifies whether or not authorization should be triggered when adding
-func (s *Servers) AddSecure(ctx context.Context, disco *discovery.Discovery, orgID string, na bool) (*Server, error) {
+func (s *Servers) AddSecure(ctx context.Context, disco *discovery.Discovery, orgID string, na bool) error {
 	if s.config.HasSecureInternet() {
-		return nil, errors.New("a secure internet server already exists")
+		return errors.New("a secure internet server already exists")
 	}
 	dorg, dsrv, err := disco.SecureHomeArgs(orgID)
 	if err != nil {
@@ -28,7 +27,7 @@ func (s *Servers) AddSecure(ctx context.Context, disco *discovery.Discovery, org
 		// Note that in the docs it states that it only should happen when the Org ID doesn't exist
 		// However, this is nice as well because it also catches the error where the SecureInternetHome server is not found
 		disco.MarkOrganizationsExpired()
-		return nil, err
+		return err
 	}
 
 	sd := api.ServerData{
@@ -41,22 +40,24 @@ func (s *Servers) AddSecure(ctx context.Context, disco *discovery.Discovery, org
 		},
 	}
 
-	var a *api.API
-	if !na {
-		// Authorize by creating the API object
-		a, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
-		if err != nil {
-			return nil, err
-		}
+	err = s.config.AddServer(orgID, server.TypeSecureInternet, v2.Server{CountryCode: dsrv.CountryCode})
+	if err != nil {
+		return err
 	}
 
-	err = s.config.AddServer(orgID, server.TypeSecureInternet, v2.Server{CountryCode: dsrv.CountryCode, LastAuthorizeTime: time.Now()})
-	if err != nil {
-		return nil, err
+	// no authorization should be triggered, return
+	if na {
+		return nil
 	}
 
-	sec := s.NewServer(orgID, server.TypeSecureInternet, a)
-	return &sec, nil
+	// Authorize by creating the API object
+	_, err = api.NewAPI(ctx, s.clientID, sd, s.cb, nil)
+	if err != nil {
+		// authorization has failed, remove the server again
+		s.config.RemoveServer(orgID, server.TypeSecureInternet)
+		return err
+	}
+	return nil
 }
 
 // GetSecure gets a secure internet server