Refactor: Use constants for state callbacks instead of strings

11 files changed, 77 insertions, 56 deletions

diff --git a/cmd/cli/main.go b/cmd/cli/main.go
index 3c262a0..80f4baf 100644
--- a/cmd/cli/main.go
+++ b/cmd/cli/main.go
@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/jwijenbergh/eduvpn-common"
+	"github.com/jwijenbergh/eduvpn-common/internal/fsm"
 )
 
 type ServerTypes int8
@@ -86,12 +87,13 @@ func sendProfile(state *eduvpn.VPNState, data string) {
 // If OAuth is started we open the browser with the Auth URL
 // If we ask for a profile, we send the profile using command line input
 // Note that this has an additional argument, the vpn state which was wrapped into this callback function below
-func stateCallback(state *eduvpn.VPNState, oldState string, newState string, data string) {
-	if newState == "OAuth_Started" {
+func stateCallback(state *eduvpn.VPNState, oldState eduvpn.VPNStateID, newState eduvpn.VPNStateID, data string) {
+	// TODO: Remove internal usage of fsm
+	if newState == fsm.OAUTH_STARTED {
 		openBrowser(data)
 	}
 
-	if newState == "Ask_Profile" {
+	if newState == fsm.ASK_PROFILE {
 		sendProfile(state, data)
 	}
 }
@@ -162,7 +164,7 @@ func storeSecureInternetConfig(state *eduvpn.VPNState, url string, directory str
 func getSecureInternetAll(homeURL string) {
 	state := &eduvpn.VPNState{}
 
-	state.Register("org.eduvpn.app.linux", "configs", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configs", func(old eduvpn.VPNStateID, new eduvpn.VPNStateID, data string) {
 		stateCallback(state, old, new, data)
 	}, true)
 
@@ -203,7 +205,7 @@ func getSecureInternetAll(homeURL string) {
 func printConfig(url string, serverType ServerTypes) {
 	state := &eduvpn.VPNState{}
 
-	state.Register("org.eduvpn.app.linux", "configs", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configs", func(old eduvpn.VPNStateID, new eduvpn.VPNStateID, data string) {
 		stateCallback(state, old, new, data)
 	}, true)
 
diff --git a/exports/exports.go b/exports/exports.go
index ce8faa2..5e4aab3 100644
--- a/exports/exports.go
+++ b/exports/exports.go
@@ -3,10 +3,10 @@ package main
 /*
 #include <stdlib.h>
 
-typedef void (*PythonCB)(const char* name, const char* oldstate, const char* newstate, const char* data);
+typedef void (*PythonCB)(const char* name, int oldstate, int newstate, const char* data);
 
 __attribute__((weak))
-void call_callback(PythonCB callback, const char *name, const char* oldstate, const char* newstate, const char* data)
+void call_callback(PythonCB callback, const char *name, int oldstate, int newstate, const char* data)
 {
     callback(name, oldstate, newstate, data);
 }
@@ -26,19 +26,17 @@ var P_StateCallbacks map[string]C.PythonCB
 
 var VPNStates map[string]*eduvpn.VPNState
 
-func StateCallback(name string, old_state string, new_state string, data string) {
+func StateCallback(name string, old_state eduvpn.VPNStateID, new_state eduvpn.VPNStateID, data string) {
 	P_StateCallback, exists := P_StateCallbacks[name]
 	if !exists || P_StateCallback == nil {
 		return
 	}
 	name_c := C.CString(name)
-	oldState_c := C.CString(old_state)
-	newState_c := C.CString(new_state)
+	oldState_c := C.int(old_state)
+	newState_c := C.int(new_state)
 	data_c := C.CString(data)
 	C.call_callback(P_StateCallback, name_c, oldState_c, newState_c, data_c)
 	C.free(unsafe.Pointer(name_c))
-	C.free(unsafe.Pointer(oldState_c))
-	C.free(unsafe.Pointer(newState_c))
 	C.free(unsafe.Pointer(data_c))
 }
 
@@ -67,7 +65,7 @@ func Register(name *C.char, config_directory *C.char, stateCallback C.PythonCB,
 	}
 	VPNStates[nameStr] = state
 	P_StateCallbacks[nameStr] = stateCallback
-	registerErr := state.Register(nameStr, C.GoString(config_directory), func(old string, new string, data string) {
+	registerErr := state.Register(nameStr, C.GoString(config_directory), func(old eduvpn.VPNStateID, new eduvpn.VPNStateID, data string) {
 		StateCallback(nameStr, old, new, data)
 	}, debug != 0)
 
diff --git a/internal/fsm/fsm.go b/internal/fsm/fsm.go
index 5a88885..84c39d5 100644
--- a/internal/fsm/fsm.go
+++ b/internal/fsm/fsm.go
@@ -124,12 +124,12 @@ type FSM struct {
 
 	// Info to be passed from the parent state
 	Name          string
-	StateCallback func(string, string, string)
+	StateCallback func(FSMStateID, FSMStateID, string)
 	Directory     string
 	Debug         bool
 }
 
-func (fsm *FSM) Init(name string, callback func(string, string, string), directory string, debug bool) {
+func (fsm *FSM) Init(name string, callback func(FSMStateID, FSMStateID, string), directory string, debug bool) {
 	fsm.States = FSMStates{
 		DEREGISTERED:   FSMState{Transitions: []FSMTransition{{NO_SERVER, "Client registers"}}},
 		NO_SERVER:      FSMState{Transitions: []FSMTransition{{CHOSEN_SERVER, "User chooses a server"}, {SEARCH_SERVER, "The user is trying to choose a Server in the UI"}, {CONNECTED, "The user is already connected"}, {ASK_LOCATION, "Change the location in the main screen"}}},
@@ -141,7 +141,7 @@ func (fsm *FSM) Init(name string, callback func(string, string, string), directo
 		AUTHORIZED:     FSMState{Transitions: []FSMTransition{{OAUTH_STARTED, "Re-authorize with OAuth"}, {REQUEST_CONFIG, "Client requests a config"}}},
 		REQUEST_CONFIG: FSMState{Transitions: []FSMTransition{{ASK_PROFILE, "Multiple profiles found and no profile chosen"}, {HAS_CONFIG, "Only one profile or profile already chosen"}, {NO_SERVER, "Cancel or Error"}, {OAUTH_STARTED, "Re-authorize"}}},
 		ASK_PROFILE:    FSMState{Transitions: []FSMTransition{{HAS_CONFIG, "User chooses profile"}, {NO_SERVER, "Cancel or Error"}, {SEARCH_SERVER, "Cancel or Error"}}},
-		HAS_CONFIG:     FSMState{Transitions: []FSMTransition{{CONNECTING, "OS reports it is trying to connect"}, {REQUEST_CONFIG, "User chooses a new profile"}, {NO_SERVER, "User wants to choose a new server"}, {OAUTH_STARTED, "Re-authorize with OAuth"}}, BackState: NO_SERVER},
+		HAS_CONFIG:     FSMState{Transitions: []FSMTransition{{CONNECTING, "OS reports it is trying to connect"}, {REQUEST_CONFIG, "User reconnects"}, {NO_SERVER, "User wants to choose a new server"}, {OAUTH_STARTED, "Re-authorize with OAuth"}}, BackState: NO_SERVER},
 		CONNECTING:     FSMState{Transitions: []FSMTransition{{HAS_CONFIG, "Cancel or Error"}, {CONNECTED, "Done connecting"}}},
 		CONNECTED:      FSMState{Transitions: []FSMTransition{{HAS_CONFIG, "OS reports disconnected"}}},
 	}
@@ -202,9 +202,9 @@ func (fsm *FSM) GoTransitionWithData(newState FSMStateID, data string, backgroun
 		}
 
 		if background {
-			go fsm.StateCallback(oldState.String(), newState.String(), data)
+			go fsm.StateCallback(oldState, newState, data)
 		} else {
-			fsm.StateCallback(oldState.String(), newState.String(), data)
+			fsm.StateCallback(oldState, newState, data)
 		}
 	}
 
diff --git a/state.go b/state.go
index 594df8d..45dd614 100644
--- a/state.go
+++ b/state.go
@@ -13,6 +13,8 @@ import (
 	"github.com/jwijenbergh/eduvpn-common/internal/util"
 )
 
+type VPNStateID = fsm.FSMStateID
+
 type VPNState struct {
 	// The chosen server
 	Servers server.Servers `json:"servers"`
@@ -43,7 +45,7 @@ func (state *VPNState) GetSavedServers() string {
 	return serversJSON
 }
 
-func (state *VPNState) Register(name string, directory string, stateCallback func(string, string, string), debug bool) error {
+func (state *VPNState) Register(name string, directory string, stateCallback func(VPNStateID, VPNStateID, string), debug bool) error {
 	errorMessage := "failed to register with the GO library"
 	if !state.FSM.InState(fsm.DEREGISTERED) {
 		return &types.WrappedErrorMessage{Message: errorMessage, Err: fsm.DeregisteredError{}.CustomError()}
@@ -406,7 +408,7 @@ func (state *VPNState) RenewSession() error {
 	}
 
 	oauthStructure := currentServer.GetOAuth()
-	oauthStructure.Token = oauth.OAuthToken{Access: "",Refresh: "",Type: "",Expires: 0,ExpiredTimestamp: util.GetCurrentTime()}
+	oauthStructure.Token = oauth.OAuthToken{Access: "", Refresh: "", Type: "", Expires: 0, ExpiredTimestamp: util.GetCurrentTime()}
 	// Make sure the FSM is initialized
 	oauthStructure.FSM = &state.FSM
 
@@ -422,7 +424,6 @@ func (state *VPNState) RenewSession() error {
 	return nil
 }
 
-
 func (state *VPNState) ShouldRenewButton() bool {
 	if !state.FSM.InState(fsm.CONNECTED) {
 		return false
diff --git a/state_test.go b/state_test.go
index c42b314..082a93c 100644
--- a/state_test.go
+++ b/state_test.go
@@ -57,8 +57,8 @@ func loginOAuthSelenium(t *testing.T, url string, state *VPNState) {
 	}
 }
 
-func stateCallback(t *testing.T, oldState string, newState string, data string, state *VPNState) {
-	if newState == "OAuth_Started" {
+func stateCallback(t *testing.T, oldState VPNStateID, newState VPNStateID, data string, state *VPNState) {
+	if newState == fsm.OAUTH_STARTED {
 		loginOAuthSelenium(t, data, state)
 	}
 }
@@ -68,7 +68,7 @@ func Test_server(t *testing.T) {
 	state := &VPNState{}
 	ensureLocalWellKnown()
 
-	state.Register("org.eduvpn.app.linux", "configstest", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configstest", func(old VPNStateID, new VPNStateID, data string) {
 		stateCallback(t, old, new, data, state)
 	}, false)
 
@@ -84,8 +84,8 @@ func test_connect_oauth_parameter(t *testing.T, parameters httpw.URLParameters,
 	state := &VPNState{}
 	configDirectory := "test_oauth_parameters"
 
-	state.Register("org.eduvpn.app.linux", configDirectory, func(oldState string, newState string, data string) {
-		if newState == "OAuth_Started" {
+	state.Register("org.eduvpn.app.linux", configDirectory, func(oldState VPNStateID, newState VPNStateID, data string) {
+		if newState == fsm.OAUTH_STARTED {
 			baseURL := "http://127.0.0.1:8000/callback"
 			url, err := httpw.HTTPConstructURL(baseURL, parameters)
 			if err != nil {
@@ -153,7 +153,7 @@ func Test_token_expired(t *testing.T) {
 	// Get a vpn state
 	state := &VPNState{}
 
-	state.Register("org.eduvpn.app.linux", "configsexpired", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configsexpired", func(old VPNStateID, new VPNStateID, data string) {
 		stateCallback(t, old, new, data, state)
 	}, false)
 
@@ -201,7 +201,7 @@ func Test_token_invalid(t *testing.T) {
 
 	ensureLocalWellKnown()
 
-	state.Register("org.eduvpn.app.linux", "configsinvalid", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configsinvalid", func(old VPNStateID, new VPNStateID, data string) {
 		stateCallback(t, old, new, data, state)
 	}, false)
 
@@ -251,7 +251,7 @@ func Test_invalid_profile_corrected(t *testing.T) {
 
 	ensureLocalWellKnown()
 
-	state.Register("org.eduvpn.app.linux", "configscancelprofile", func(old string, new string, data string) {
+	state.Register("org.eduvpn.app.linux", "configscancelprofile", func(old VPNStateID, new VPNStateID, data string) {
 		stateCallback(t, old, new, data, state)
 	}, false)
 
diff --git a/wrappers/python/main.py b/wrappers/python/main.py
index 58e8b95..1ab29cc 100644
--- a/wrappers/python/main.py
+++ b/wrappers/python/main.py
@@ -1,4 +1,5 @@
 import eduvpn_common.main as eduvpn
+from eduvpn_common.state import State, StateType
 import webbrowser
 import json
 import sys
@@ -27,22 +28,20 @@ def ask_profile_input(total: int) -> int:
 def setup_callbacks(_eduvpn: eduvpn.EduVPN) -> None:
     # The callback that starst OAuth
     # It needs to open the URL in the web browser
-    @_eduvpn.event.on("OAuth_Started", eduvpn.StateType.Enter)
+    @_eduvpn.event.on(State.OAUTH_STARTED, StateType.Enter)
     def oauth_initialized(old_state: str, url: str) -> None:
         print(f"Got OAuth URL {url}, old state: {old_state}")
         webbrowser.open(url)
 
-    @_eduvpn.event.on("Ask_Location", eduvpn.StateType.Enter)
+    @_eduvpn.event.on(State.ASK_LOCATION, StateType.Enter)
     def ask_location(old_state: str, locations: str):
         print("Locations: ", locations)
         _eduvpn.set_secure_location("NL")
 
     # The callback which asks the user for a profile
-    @_eduvpn.event.on("Ask_Profile", eduvpn.StateType.Enter)
+    @_eduvpn.event.on(State.ASK_PROFILE, StateType.Enter)
     def ask_profile(old_state: str, profiles: str):
-        print(
-            "Multiple profiles found, you need to select a profile, old state: {old_state}"
-        )
+        print("Multiple profiles found, you need to select a profile:")
 
         # Parse the profiles as JSON
         data = json.loads(profiles)
@@ -94,6 +93,7 @@ if __name__ == "__main__":
 
     # Set the internal FSM state to connected
     try:
+        _eduvpn.set_connecting()
         _eduvpn.set_connected()
     except Exception as e:
         print("Failed to set connected:", e)
diff --git a/wrappers/python/src/__init__.py b/wrappers/python/src/__init__.py
index be06525..c0b6679 100644
--- a/wrappers/python/src/__init__.py
+++ b/wrappers/python/src/__init__.py
@@ -43,7 +43,7 @@ class DataError(Structure):
     _fields_ = [("data", c_void_p), ("error", c_void_p)]
 
 
-VPNStateChange = CFUNCTYPE(None, c_char_p, c_char_p, c_char_p, c_char_p)
+VPNStateChange = CFUNCTYPE(None, c_char_p, c_int, c_int, c_char_p)
 
 # Exposed functions
 # We have to use c_void_p instead of c_char_p to free it properly
diff --git a/wrappers/python/src/event.py b/wrappers/python/src/event.py
index 778ce5e..0803dee 100644
--- a/wrappers/python/src/event.py
+++ b/wrappers/python/src/event.py
@@ -1,19 +1,14 @@
 from . import VPNStateChange
 from enum import Enum
 from typing import Callable
-
-
-class StateType(Enum):
-    Enter = 1
-    Leave = 2
-    Wait = 3
+from .state import StateType
 
 
 EDUVPN_CALLBACK_PROPERTY = "_eduvpn_property_callback"
 
 # A state transition decorator for classes
 # To use this, make sure to register the class with `register_class_callbacks`
-def class_state_transition(state: str, state_type: StateType) -> Callable:
+def class_state_transition(state: int, state_type: StateType) -> Callable:
     def wrapper(func):
         setattr(func, EDUVPN_CALLBACK_PROPERTY, (state, state_type))
         return func
@@ -45,7 +40,7 @@ class EventHandler(object):
                 else:
                     self.remove_event(state, state_type, method)
 
-    def remove_event(self, state: str, state_type: StateType, func: Callable):
+    def remove_event(self, state: int, state_type: StateType, func: Callable):
         for key, values in self.handlers.copy().items():
             if key == (state, state_type):
                 values.remove(func)
@@ -54,13 +49,13 @@ class EventHandler(object):
                 else:
                     self.handlers[key] = values
 
-    def add_event(self, state: str, state_type: StateType, func: Callable):
+    def add_event(self, state: int, state_type: StateType, func: Callable):
         if (state, state_type) not in self.handlers:
             self.handlers[(state, state_type)] = []
         self.handlers[(state, state_type)].append(func)
 
     # A decorator for standalone functions
-    def on(self, state: str, state_type: StateType) -> Callable:
+    def on(self, state: int, state_type: StateType) -> Callable:
         def wrapped_f(func):
             self.add_event(state, state_type, func)
             return func
@@ -68,14 +63,14 @@ class EventHandler(object):
         return wrapped_f
 
     def run_state(
-        self, state: str, other_state: str, state_type: StateType, data: str
+        self, state: int, other_state: int, state_type: StateType, data: str
     ) -> None:
         if (state, state_type) not in self.handlers:
             return
         for func in self.handlers[(state, state_type)]:
             func(other_state, data)
 
-    def run(self, old_state: str, new_state: str, data: str) -> None:
+    def run(self, old_state: int, new_state: int, data: str) -> None:
         if old_state == new_state:
             return
 
diff --git a/wrappers/python/src/main.py b/wrappers/python/src/main.py
index ac44073..8440c7d 100644
--- a/wrappers/python/src/main.py
+++ b/wrappers/python/src/main.py
@@ -1,7 +1,8 @@
 from . import lib, VPNStateChange, encode_args, decode_res
 from typing import Optional, Tuple
 import threading
-from .event import StateType, EventHandler
+from .event import EventHandler
+from .state import State, StateType
 import json
 
 eduvpn_objects = {}
@@ -25,7 +26,7 @@ def state_callback(name, old_state, new_state, data):
     name = name.decode()
     if name not in eduvpn_objects:
         return
-    eduvpn_objects[name].callback(old_state.decode(), new_state.decode(), data.decode())
+    eduvpn_objects[name].callback(State(old_state), State(new_state), data.decode())
 
 
 class EduVPN(object):
@@ -41,13 +42,13 @@ class EduVPN(object):
         self.profile_event: Optional[threading.Event] = None
         self.location_event: Optional[threading.Event] = None
 
-        @self.event.on("Ask_Profile", StateType.Wait)
-        def wait_profile_event(old_state: str, profiles: str):
+        @self.event.on(State.ASK_PROFILE, StateType.Wait)
+        def wait_profile_event(old_state: int, profiles: str):
             if self.profile_event:
                 self.profile_event.wait()
 
-        @self.event.on("Ask_Location", StateType.Wait)
-        def wait_location_event(old_state: str, locations: str):
+        @self.event.on(State.ASK_LOCATION, StateType.Wait)
+        def wait_location_event(old_state: int, locations: str):
             if self.location_event:
                 self.location_event.wait()
 
@@ -172,7 +173,7 @@ class EduVPN(object):
     def event(self) -> EventHandler:
         return self.event_handler
 
-    def callback(self, old_state: str, new_state: str, data: str) -> None:
+    def callback(self, old_state: State, new_state: State, data: str) -> None:
         self.event.run(old_state, new_state, data)
 
     def set_profile(self, profile_id: str) -> None:
diff --git a/wrappers/python/src/state.py b/wrappers/python/src/state.py
new file mode 100644
index 0000000..cd5bd90
--- /dev/null
+++ b/wrappers/python/src/state.py
@@ -0,0 +1,23 @@
+from enum import IntEnum
+
+
+class StateType(IntEnum):
+    Enter = 1
+    Leave = 2
+    Wait = 3
+
+
+class State(IntEnum):
+    DEREGISTERED = 0
+    NO_SERVER = 1
+    ASK_LOCATION = 2
+    SEARCH_SERVER = 3
+    LOADING_SERVER = 4
+    CHOSEN_SERVER = 5
+    OAUTH_STARTED = 6
+    AUTHORIZED = 7
+    REQUEST_CONFIG = 8
+    ASK_PROFILE = 9
+    HAS_CONFIG = 10
+    CONNECTING = 11
+    CONNECTED = 12
diff --git a/wrappers/python/tests.py b/wrappers/python/tests.py
index cbec370..5cf5c25 100644
--- a/wrappers/python/tests.py
+++ b/wrappers/python/tests.py
@@ -2,6 +2,7 @@
 
 import unittest
 import eduvpn_common.main as eduvpn
+from eduvpn_common.state import State, StateType
 import webbrowser
 import sys
 import os
@@ -20,7 +21,7 @@ class ConfigTests(unittest.TestCase):
         # This can throw an exception
         _eduvpn.register()
 
-        @_eduvpn.event.on("OAuth_Started", eduvpn.StateType.Enter)
+        @_eduvpn.event.on(State.OAUTH_STARTED, StateType.Enter)
         def oauth_initialized(old_state, url):
             login_eduvpn(url)