Repository: eduvpn/eduvpn-common (language: python)

Issue:
Discovery: JSON unmarshalling does not overwrite all old values
The problem: https://go.dev/play/p/wkqry77oi7_d

We need to make sure we marshal to a completely new structure instead
Discovery: JSON unmarshalling does not overwrite all old values
The problem: https://go.dev/play/p/wkqry77oi7_d

We need to make sure we marshal to a completely new structure instead


Discussion / hints:

Fix patch:
diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go
index 3994abad..fe72e7c4 100644
--- a/internal/discovery/discovery.go
+++ b/internal/discovery/discovery.go
@@ -292,8 +292,10 @@ func (discovery *Discovery) Organizations(ctx context.Context) (*Organizations,
 		return &discovery.OrganizationList, false, nil
 	}
 	file := "organization_list.json"
-	err := discovery.file(ctx, file, discovery.OrganizationList.Version, &discovery.OrganizationList)
+	var jsonDecode Organizations
+	err := discovery.file(ctx, file, discovery.OrganizationList.Version, &jsonDecode)
 	if err != nil {
+		log.Logger.Warningf("failed to get fresh organizations: %v", err)
 		// Return previous with an error
 		orgs, perr := discovery.previousOrganizations()
 		if perr != nil {
@@ -301,6 +303,11 @@ func (discovery *Discovery) Organizations(ctx context.Context) (*Organizations,
 		}
 		return orgs, false, err
 	}
+	if len(jsonDecode.List) == 0 {
+		log.Logger.Warningf("fresh organization list is empty")
+	} else {
+		discovery.OrganizationList = jsonDecode
+	}
 	discovery.OrganizationList.Timestamp = time.Now()
 	return &discovery.OrganizationList, true, nil
 }
@@ -313,8 +320,10 @@ func (discovery *Discovery) Servers(ctx context.Context) (*Servers, bool, error)
 		return &discovery.ServerList, false, nil
 	}
 	file := "server_list.json"
-	err := discovery.file(ctx, file, discovery.ServerList.Version, &discovery.ServerList)
+	var jsonDecode Servers
+	err := discovery.file(ctx, file, discovery.ServerList.Version, &jsonDecode)
 	if err != nil {
+		log.Logger.Warningf("failed to get fresh servers: %v", err)
 		// Return previous with an error
 		srvs, perr := discovery.previousServers()
 		if perr != nil {
@@ -322,7 +331,11 @@ func (discovery *Discovery) Servers(ctx context.Context) (*Servers, bool, error)
 		}
 		return srvs, false, err
 	}
-	// Update servers timestamp
+	if len(jsonDecode.List) == 0 {
+		log.Logger.Warningf("fresh server list is empty")
+	} else {
+		discovery.ServerList = jsonDecode
+	}
 	discovery.ServerList.Timestamp = time.Now()
 	return &discovery.ServerList, true, nil
 }


Test patch:
diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go
index 42191be2..1101bb28 100644
--- a/internal/discovery/discovery_test.go
+++ b/internal/discovery/discovery_test.go
@@ -23,7 +23,7 @@ func TestServers(t *testing.T) {
 	}
 	d := &Discovery{httpClient: c}
 	// get servers
-	s1, fresh, err := d.Servers(context.Background())
+	_, fresh, err := d.Servers(context.Background())
 	if !fresh {
 		t.Fatalf("Did not obtain the server list fresh")
 	}
@@ -31,6 +31,36 @@ func TestServers(t *testing.T) {
 		t.Fatalf("Failed getting servers: %v", err)
 	}
 
+	// force expired
+	d.ServerList.Timestamp = time.Now().Add(-1 * time.Hour)
+
+	// override the last server with a keyword list
+	// and make sure a new fetch doesn't copy over the keyword list
+	// to the last entry
+	d.ServerList.List[len(d.ServerList.List)-1] = Server{
+		Server: discotypes.Server{
+			BaseURL: "https://example.org/",
+			DisplayName: map[string]string{
+				"en": "example",
+			},
+			Type: "institute_access",
+		},
+		KeywordList: map[string]string{
+			"en": "test bla",
+		},
+		SupportContact: []string{"mailto:test@example.org"},
+	}
+	s1, fresh, err := d.Servers(context.Background())
+	if !fresh {
+		t.Fatalf("Did not obtain the server list fresh after inserting a mock entry and faking expiry")
+	}
+	if err != nil {
+		t.Fatalf("Failed getting servers after inserting a mock entry: %v", err)
+	}
+	if kws := s1.List[len(s1.List)-1].KeywordList; kws != nil {
+		t.Fatalf("KeywordList is not nil when getting a fresh server list after inserting a mock entry: %v", kws)
+	}
+
 	// Shutdown the server
 	s.Close()
 	// Test if we get the same cached copy
@@ -75,7 +105,7 @@ func TestOrganizations(t *testing.T) {
 	}
 	d := &Discovery{httpClient: c}
 	// get servers
-	s1, fresh, err := d.Organizations(context.Background())
+	_, fresh, err := d.Organizations(context.Background())
 	if !fresh {
 		t.Fatalf("The organization list was not obtained fresh")
 	}
@@ -86,6 +116,35 @@ func TestOrganizations(t *testing.T) {
 		t.Fatalf("Did not get a fresh organization list")
 	}
 
+	// force expired
+	d.OrganizationList.Timestamp = time.Now().Add(-4 * time.Hour)
+
+	// override the last organization with a keyword list
+	// and make sure a new fetch doesn't copy over the keyword list
+	// to the last entry
+	d.OrganizationList.List[len(d.OrganizationList.List)-1] = Organization{
+		Organization: discotypes.Organization{
+			DisplayName: map[string]string{
+				"en": "example",
+			},
+			OrgID: "example_orgid",
+		},
+		SecureInternetHome: "example.org",
+		KeywordList: map[string]string{
+			"en": "test bla",
+		},
+	}
+	s1, fresh, err := d.Organizations(context.Background())
+	if !fresh {
+		t.Fatalf("Did not obtain the organization list fresh after inserting a mock entry and faking expiry")
+	}
+	if err != nil {
+		t.Fatalf("Failed getting organizations after inserting a mock entry: %v", err)
+	}
+	if kws := s1.List[len(s1.List)-1].KeywordList; kws != nil {
+		t.Fatalf("KeywordList is not nil when getting a fresh organization list after inserting a mock entry: %v", kws)
+	}
+
 	// Shutdown the server
 	s.Close()
 	// Test if we get the same cached copy