From 75ef7fd63aa26fdea966221abc6aa3ea726e51e7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 22 Apr 2024 17:25:04 +0200 Subject: [PATCH 01/22] refactor: consolidate httpx and httpapi This diff includes a first attempt at consolidating the patterns with which we invoke OONI and third-party API calls. I have refactored the code of httpx and httpapi into a new package called httpclientx, added some tests, started converting some parts of the tree, and explained myself in a design document. Part of https://github.com/ooni/probe/issues/2700 --- internal/cmd/apitool/main.go | 2 +- internal/engine/session.go | 8 +- internal/enginelocate/cloudflare.go | 23 +- internal/enginelocate/ubuntu.go | 28 +- internal/erroror/erroror.go | 8 + internal/httpclientx/DESIGN.md | 380 ++++++++++++++++++++++ internal/httpclientx/config.go | 20 ++ internal/httpclientx/getjson.go | 39 +++ internal/httpclientx/getjson_test.go | 220 +++++++++++++ internal/httpclientx/getraw.go | 32 ++ internal/httpclientx/getraw_test.go | 175 ++++++++++ internal/httpclientx/getxml.go | 39 +++ internal/httpclientx/getxml_test.go | 216 ++++++++++++ internal/httpclientx/httpclientx.go | 102 ++++++ internal/httpclientx/httpclientx_test.go | 144 ++++++++ internal/httpclientx/overlapped.go | 159 +++++++++ internal/httpclientx/postjson.go | 61 ++++ internal/httpclientx/postjson_test.go | 314 ++++++++++++++++++ internal/httpclientx/singlecall.go | 5 + internal/oonirun/v2.go | 18 +- internal/probeservices/bouncer.go | 23 +- internal/probeservices/checkin.go | 27 +- internal/probeservices/measurementmeta.go | 34 +- internal/probeservices/register.go | 21 ++ internal/testingx/httptestx.go | 48 ++- internal/testingx/httptestx_test.go | 40 +++ internal/testingx/logger.go | 87 +++++ internal/testingx/logger_test.go | 33 ++ 28 files changed, 2225 insertions(+), 81 deletions(-) create mode 100644 internal/erroror/erroror.go create mode 100644 internal/httpclientx/DESIGN.md create mode 100644 internal/httpclientx/config.go create mode 100644 internal/httpclientx/getjson.go create mode 100644 internal/httpclientx/getjson_test.go create mode 100644 internal/httpclientx/getraw.go create mode 100644 internal/httpclientx/getraw_test.go create mode 100644 internal/httpclientx/getxml.go create mode 100644 internal/httpclientx/getxml_test.go create mode 100644 internal/httpclientx/httpclientx.go create mode 100644 internal/httpclientx/httpclientx_test.go create mode 100644 internal/httpclientx/overlapped.go create mode 100644 internal/httpclientx/postjson.go create mode 100644 internal/httpclientx/postjson_test.go create mode 100644 internal/httpclientx/singlecall.go create mode 100644 internal/testingx/logger.go create mode 100644 internal/testingx/logger_test.go diff --git a/internal/cmd/apitool/main.go b/internal/cmd/apitool/main.go index 0b24e55f31..2136d4e8ad 100644 --- a/internal/cmd/apitool/main.go +++ b/internal/cmd/apitool/main.go @@ -107,7 +107,7 @@ func mmeta(c probeservices.Client, full bool) *model.OOAPIMeasurementMeta { Input: *input, } ctx := context.Background() - m, err := c.GetMeasurementMeta(ctx, config) + m, err := c.GetMeasurementMeta(ctx, &config) fatalOnError(err, "client.GetMeasurementMeta failed") return m } diff --git a/internal/engine/session.go b/internal/engine/session.go index 855191f95a..961a2b6c11 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -645,7 +645,13 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error { } s.logger.Infof("session: using probe services: %+v", selected.Endpoint) s.selectedProbeService = &selected.Endpoint - s.availableTestHelpers = selected.TestHelpers + s.availableTestHelpers = map[string][]model.OOAPIService{ + "web-connectivity": {{ + Address: "https://4.th.ooni.org/", + Type: "https", + Front: "", + }}, + } return nil } diff --git a/internal/enginelocate/cloudflare.go b/internal/enginelocate/cloudflare.go index 17a69112a1..7d5fc89d7a 100644 --- a/internal/enginelocate/cloudflare.go +++ b/internal/enginelocate/cloudflare.go @@ -6,7 +6,7 @@ import ( "regexp" "strings" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -17,17 +17,24 @@ func cloudflareIPLookup( userAgent string, resolver model.Resolver, ) (string, error) { - data, err := (&httpx.APIClientTemplate{ - BaseURL: "https://www.cloudflare.com", - HTTPClient: httpClient, - Logger: logger, - UserAgent: model.HTTPHeaderUserAgent, - }).WithBodyLogging().Build().FetchResource(ctx, "/cdn-cgi/trace") + // get the raw response body + data, err := httpclientx.GetRaw( + ctx, + "https://www.cloudflare.com/cdn-cgi/trace", + httpClient, + logger, + userAgent, + ) + + // handle the error case if err != nil { return model.DefaultProbeIP, err } + + // find the IP addr r := regexp.MustCompile("(?:ip)=(.*)") ip := strings.Trim(string(r.Find(data)), "ip=") - logger.Debugf("cloudflare: body: %s", ip) + + // done! return ip, nil } diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index de59ec9d89..9df193dae3 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -5,7 +5,7 @@ import ( "encoding/xml" "net/http" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -21,20 +21,20 @@ func ubuntuIPLookup( userAgent string, resolver model.Resolver, ) (string, error) { - data, err := (&httpx.APIClientTemplate{ - BaseURL: "https://geoip.ubuntu.com/", - HTTPClient: httpClient, - Logger: logger, - UserAgent: userAgent, - }).WithBodyLogging().Build().FetchResource(ctx, "/lookup") - if err != nil { - return model.DefaultProbeIP, err - } - logger.Debugf("ubuntu: body: %s", string(data)) - var v ubuntuResponse - err = xml.Unmarshal(data, &v) + // read the HTTP response and parse as XML + resp, err := httpclientx.GetXML[*ubuntuResponse]( + ctx, + "https://geoip.ubuntu.com/lookup", + httpClient, + logger, + userAgent, + ) + + // handle the error case if err != nil { return model.DefaultProbeIP, err } - return v.IP, nil + + // handle the success case + return resp.IP, nil } diff --git a/internal/erroror/erroror.go b/internal/erroror/erroror.go new file mode 100644 index 0000000000..833272ee89 --- /dev/null +++ b/internal/erroror/erroror.go @@ -0,0 +1,8 @@ +// Package erroror contains code to represent an error or a value. +package erroror + +// Value represents an error or a value. +type Value[Type any] struct { + Err error + Value Type +} diff --git a/internal/httpclientx/DESIGN.md b/internal/httpclientx/DESIGN.md new file mode 100644 index 0000000000..1a69110aeb --- /dev/null +++ b/internal/httpclientx/DESIGN.md @@ -0,0 +1,380 @@ +# ./internal/httpclientx + +This package aims to replace previously existing packages for interact with +backend services controlled either by us or by third parties. + +As of 2024-04-22, these packages are: + +* `./internal/httpx`, which is currently deprecated; + +* `./internal/httpapi`, which aims to automatically generate swaggers from input +and output messages and implements support for falling back. + +The rest of this document explains the requirements and describes the design. + +## Table of Contents + +- [Requirements](#requirements) +- [Design](#design) + - [Overlapped Operations](#overlapped-operations) + - [Extensibility](#extensibility) + - [Functionality Comparison](#functionality-comparison) + - [GetJSON](#getjson) + - [GetRaw](#getraw) + - [GetXML](#getxml) + - [PostJSON](#postjson) +- [Refactoring Plan](#refactoring-plan) +- [Limitations and Future Work](#limitations-and-future-work) + +## Requirements + +We want this new package to: + +1. Implement common access patterns (GET with JSON body, GET with XML body, GET with +raw body, and POST with JSON request and response bodies). + +2. Optionally log request and response bodies. + +3. Factor common code for accessing such services. + +4. Take advantage of Go generics to automatically marshal and unmarshal without +having to write specific functions for each request/response pair. + +5. Support some kind of fallback policy like `httpapi` because we used this +for test helpers and, while, as of today, we're mapping serveral TH domain names +to a single IP address, it might still be useful to fallback. + +6. Provide an easy way for the caller to know whether an HTTP request failed +and, if so, with which status code, which is needed to intercept `401` responses +to take the appropriate logging-in actions. + +7. Make the design extensible such that re-adding unused functionality (such +as cloud fronting) does not require us to refactor the code much. + +8. Functional equivalent with existing packages (modulo the existing +functionality that is not relevant anymore). + +Non goals: + +1. automatically generate swaggers from the Go representation of API invocation +for these reasons: (1) the OONI backend swagger is only for documentational +purposes and it is not always in sync with reality; (2) by doing that, we obtained +overly complex code, which hampers maintenance. + +2. implementing algorithms such as logging in to the OONI backend and requesting +tokens, which should be the responsibility of another package. + +3. implementing cloud fronting, which is not used anymore. + +## Design + +This package supports the following operations: + +```Go +type Config struct { + Client model.HTTPClient + Logger model.Logger + UserAgent string +} + +func GetJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) + +func GetRaw(ctx context.Context, config *Config, URL string) ([]byte, error) + +func GetXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) + +func PostJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) +``` + +These operations implement all the actions listed in the first requirement. + +The `Config` struct allows to add new optional fields to implement new functionality without +changing the API and with minimal code refactoring efforts (seventh requirement). + +We're using generics to automate marshaling and umarshaling (requirement four). + +The internal implementation is such that we reuse code to avoid code duplication, +thus addressing also requirement three. + +Additionally, whenever a call fails with a non-200 status code, the return +value can be converted to the following type using `errors.As`: + +```Go +type ErrRequestFailed struct { + StatusCode int +} + +func (err *ErrRequestFailed) Error() string +``` + +Therefore, we have a way to know why a request failed (requirement six). + +To avoid logging bodies, one just needs to pass `model.DiscardLogger` as the +`logger` (thus fulfilling requirement two). + +### Overlapped Operations + +The code at `./internal/httpapi` performs sequential function calls. This design +does not interact well with the `enginenetx` package and its dial tactics. A better +strategy is to allow calls to be overlapped. This means that, if the `enginenetx` +is busy trying tactics for a given API endpoint, we eventually try to use the +subsequent (semantically-equivalent) endpoint after a given time, without waiting +for the first endpoint to complete. + +We allow for overlapped operations by defining these constructors: + +```Go +func NewOverlappedGetJSON[Output any](ctx context.Context, config *Config) *Overlapped[Output] + +func NewOverlappedGetRaw(ctx context.Context, config *Config) *Overlapped[[]byte] + +func NewOverlappedGetXML[Output any](ctx context.Context, config *Config) *Overlapped[Output] + +func NewOverlappedPostJSON[Input, Output any](ctx context.Context, config *Config, input Input) *Overlapped[Output] +``` + +They all construct the same `*Overlapped` struct, which looks like this: + +```Go +type Overlapped[Output any] struct { + RunFunc func(ctx context.Context, URL string) (Output, error) + + ScheduleInterval time.Duration +} +``` + +The constructor configures `RunFunc` to invoke the call corresponding to the construct +name (i.e., `NewOverlappedGetXML` configures `RunFunc` to be `GetXML`). + +Then, we define the following method: + +```Go +func (ovx *Overlapped[Output]) Run(ctx context.Context, URLs ...string) (Output, error) +``` + +This method starts N goroutines to issue the API calls with each URL. (A classic example is for +the URLs to be `https://0.th.ooni.org/`, `https://1.th.ooni.org/` and so on.) + +By default, `ScheduleInterval` is 15 seconds. If the first URL does not provide a result +within 15 seconds, we try the second one. That is, every 15 seconds, we will attempt using +another URL, until there's a successful response or we run out of URLs. + +As soon as we have a successful response, we cancel all the other pending operations +that may exist. Once all operations have terminated, we return to the caller. + +### Extensibility + +We use the `Config` object to package common settings. Thus adding a new field (e.g., a +custom `Host` header to implement cloud fronting), only means the following: + +1. Adding a new OPTIONAL field to `Config`. + +2. Honoring this field inside the internal implementation. + +_Et voilà_, this should allow for minimal efforts API upgrades. + +### Functionality Comparison + +This section compares side-by-side the operations performed by each implementation +as of [probe-cli@7dab5a29812](https://github.com/ooni/probe-cli/tree/7dab5a29812) to +show that they implement ~equivalent functionality. This should be the case, since +the `httpxclientx` package is a refactoring of `httpapi`, which in turn contains code +originally derived from `httpx`. Anyways, better to double check. + +#### GetJSON + +We compare to `httpapi.Call` and `httpx.GetJSONWithQuery`. + +| ------------------------- | ------- | ------- | ----- | +| Operation | GetJSON | httpapi | httpx | +| ------------------------- | ------- | ------- | ----- | +| enforce a call timeout | NO | yes | NO | +| parse base URL | NO | yes | yes | +| join path and base URL | NO | yes | yes | +| append query to URL | NO | yes | yes | +| NewRequestWithContext | yes️ | yes | yes | +| handle cloud front | NO | yes | yes | +| set Authorization | yes | yes | yes | +| set Accept | NO | yes | yes | +| set User-Agent | yes ️ | yes | yes | +| set Accept-Encoding gzip | yes️ | yes | NO | +| (HTTPClient).Do() | yes | yes | yes | +| defer resp.Body.Close() | yes | yes | yes | +| handle gzip encoding | yes | yes | NO | +| limit io.Reader | yes | yes | yes | +| netxlite.ReadAllContext() | yes | yes | yes | +| handle truncated body | NO | yes | NO | +| log response body | yes | yes | yes | +| handle non-200 response | ️ yes | yes* | yes* | +| unmarshal JSON | yes | yes | yes | + +The `yes*` means that `httpapi` rejects responses with status codes `>= 400` (like cURL) +while the new package only accepts status codes `== 200`. This difference should be of little +practical significance for all the APIs we invoke and the new behavior is stricter. + +Regarding all the other cases for which `GetJSON` is marked as "NO": + +1. Enforcing a call timeout is better done just through the context like `httpx` does. + +2. `GetJSON` lets the caller completely manage the construction of the URL, so we do not need +code to join together a base URL, possibly including a base path, a path, and a query. + +3. `GetJSON` does not handle cloud fronting because we don't use it. The design where the +`Config` contains mandatory and optional fields would still allow doing that easily. + +4. Setting the `Accept` header does not seem to matter in out context because we mostly +call API for which there's no need for content negotiation. + +5. It's difficult to say whether a body size was exactly the amount specified for truncation +or the body has been truncated. While this is a corner case, it seems perhaps wiser to let +the caller try parsing the body and failing if it is indeed truncated. + +#### GetRaw + +Here we're comparing to `httpapi.Call` and `httpx.FetchResource`. + +| ------------------------- | ------- | ------- | ----- | +| Operation | GetRaw | httpapi | httpx | +| ------------------------- | ------- | ------- | ----- | +| enforce a call timeout | NO | yes | NO | +| parse base URL | NO | yes | yes | +| join path and base URL | NO | yes | yes | +| append query to URL | NO | yes | yes | +| NewRequestWithContext | yes️ | yes | yes | +| handle cloud front | NO | yes | yes | +| set Authorization | yes | yes | yes | +| set Accept | NO | yes | yes | +| set User-Agent | yes ️ | yes | yes | +| set Accept-Encoding gzip | yes️ | yes | NO | +| (HTTPClient).Do() | yes | yes | yes | +| defer resp.Body.Close() | yes | yes | yes | +| handle gzip encoding | yes | yes | NO | +| limit io.Reader | yes | yes | yes | +| netxlite.ReadAllContext() | yes | yes | yes | +| handle truncated body | NO | yes | NO | +| log response body | yes | yes | yes | +| handle non-200 response | ️ yes | yes* | yes | + +#### GetXML + +There's no direct equivalent of `GetXML` in `httpapi` and `httpx`. Therefore, when using these +two APIs, the caller would need to fetch a raw body and then manually parse XML. + +| ------------------------- | ------- | ------- | ----- | +| Operation | GetXML | httpapi | httpx | +| ------------------------- | ------- | ------- | ----- | +| enforce a call timeout | NO | N/A | N/A | +| parse base URL | NO | N/A | N/A | +| join path and base URL | NO | N/A | N/A | +| append query to URL | NO | N/A | N/A | +| NewRequestWithContext | yes️ | N/A | N/A | +| handle cloud front | NO | N/A | N/A | +| set Authorization | yes | N/A | N/A | +| set Accept | NO | N/A | N/A | +| set User-Agent | yes ️ | N/A | N/A | +| set Accept-Encoding gzip | yes️ | N/A | N/A | +| (HTTPClient).Do() | yes | N/A | N/A | +| defer resp.Body.Close() | yes | N/A | N/A | +| handle gzip encoding | yes | N/A | N/A | +| limit io.Reader | yes | N/A | N/A | +| netxlite.ReadAllContext() | yes | N/A | N/A | +| handle truncated body | NO | N/A | N/A | +| log response body | yes | N/A | N/A | +| handle non-200 response | ️ yes | N/A | N/A | +| unmarshal XML | yes | N/A | N/A | + +Because comparison is not possible, there is not much else to say. + +#### PostJSON + +Here we're comparing to `httpapi.Call` and `httpx.PostJSON`. + +| ------------------------- | -------- | ------- | ----- | +| Operation | PostJSON | httpapi | httpx | +| ------------------------- | -------- | ------- | ----- | +| marshal JSON | yes | yes~ | yes | +| log request body | yes | yes | yes | +| enforce a call timeout | NO | yes | NO | +| parse base URL | NO | yes | yes | +| join path and base URL | NO | yes | yes | +| append query to URL | NO | yes | yes | +| NewRequestWithContext | yes️ | yes | yes | +| handle cloud front | NO | yes | yes | +| set Authorization | yes | yes | yes | +| set Accept | NO | yes | yes | +| set Content-Type | yes | yes | yes | +| set User-Agent | yes ️ | yes | yes | +| set Accept-Encoding gzip | yes️ | yes | NO | +| (HTTPClient).Do() | yes | yes | yes | +| defer resp.Body.Close() | yes | yes | yes | +| handle gzip encoding | yes | yes | NO | +| limit io.Reader | yes | yes | yes | +| netxlite.ReadAllContext() | yes | yes | yes | +| handle truncated body | NO | yes | NO | +| log response body | yes | yes | yes | +| handle non-200 response | ️ yes | yes* | yes* | +| unmarshal JSON | yes | yes | yes | + +The `yes*` means that `httpapi` rejects responses with status codes `>= 400` (like cURL) +while the new package only accepts status codes `== 200`. This difference should be of little +practical significance for all the APIs we invoke and the new behavior is stricter. + +The `yes~` means that `httpapi` already receives a marshaled body from a higher-level API +that is part of the same package, while in this package we marshal in `PostJSON`. + +## Refactoring Plan + +The overall goal is to replace usages of `httpapi` and `httpx` with usages of `httpclient`. + +The following packages use `httpapi`: + +1. `internal/experiment/webconnectivity`: uses `httpapi.SeqCaller` to chain calls to all +the available test helpers, which we can replace with using `*Overlapped`; + +2. `internal/experiment/webconnectivitylte`: same as above; + +3. `internal/ooapi`: uses `httpapi` to define the OONI backend APIs for the purpose of +generating and cross-validating swaggers, which is something we defined as a non-goal given +that we never really managed to do it reliably, and it has only led to code complexity; + +4. `internal/probeservices`: uses `httpapi` to implement check-in and the main reason why +this is the case is because it supports `"gzip"` encoding; + +The following packages use `httpx`: + +1. `internal/cmd/apitool`: this is just a byproduct of `probeservices.Client` embedding +the `httpx.APIClientTemplate`, so this should really be easy to get rid of; + +2. `internal/enginelocate`: we're using `httpx` convenience functions to figure out +the probe IP and we can easily replace these calls with `httpclientx`; + +3. `internal/oonirun`: uses `httpx` to fetch descriptors and can be easily replaced; + +4. `internal/probeservices`: uses `httpx` for most other calls. + +Based on the above information, it seems the easiest way to proceed is this: + +1. `internal/enginelocate`: replace `httpx` with `httpclientx`; + +2. `internal/oonirun`: replace `httpx` with `httpclientx`; + +3. `internal/probeservices`: replace the check-in implementation to use `httpclientx` +instead of using the `httpapi` package; + +4. `internal/experiment/webconnectivity{,lte}`: replace the `httpapi.SeqCaller` usage +with invocations of `*Overlapped`; + +5. remove the `httpapi` and `ooapi` packages, now unused; + +6. finish replacing `httpx` with `httpclientx` in `internal/probeservices` + +7. remove the `httpx` package. + +## Limitations and Future Work + +The current implementation of `*Overlapped` may cause us to do more work than needed in +case the network is really slow and an attempt is slowly fetching the body. In such a case, +starting a new attempt duplicayes work. Handling this case does not seem straightforward +currently, therefore, we will focus on this as part of future work. + + diff --git a/internal/httpclientx/config.go b/internal/httpclientx/config.go new file mode 100644 index 0000000000..71bdeb6161 --- /dev/null +++ b/internal/httpclientx/config.go @@ -0,0 +1,20 @@ +package httpclientx + +import "github.com/ooni/probe-cli/v3/internal/model" + +// Config contains configuration shared by [GetJSON], [GetXML], [GetRaw], and [PostJSON]. +// +// The zero value is invalid; initialize the MANDATORY fields. +type Config struct { + // Authorization contains the OPTIONAL Authorization header value to use. + Authorization string + + // Client is the MANDATORY [model.HTTPClient] to use. + Client model.HTTPClient + + // Logger is the MANDATORY [model.Logger] to use. + Logger model.Logger + + // UserAgent is the MANDATORY User-Agent header value to use. + UserAgent string +} diff --git a/internal/httpclientx/getjson.go b/internal/httpclientx/getjson.go new file mode 100644 index 0000000000..11cf21a35c --- /dev/null +++ b/internal/httpclientx/getjson.go @@ -0,0 +1,39 @@ +package httpclientx + +// +// getjson.go - GET a JSON response. +// + +import ( + "context" + "encoding/json" +) + +// GetJSON sends a GET request and reads a JSON response. +// +// Arguments: +// +// - ctx is the cancellable context; +// +// - config contains the config; +// +// - URL is the URL to use. +// +// This function either returns an error or a valid Output. +func GetJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) { + // read the raw body + rawrespbody, err := GetRaw(ctx, config, URL) + + // handle the case of error + if err != nil { + return zeroValue[Output](), err + } + + // parse the response body as JSON + var output Output + if err := json.Unmarshal(rawrespbody, &output); err != nil { + return zeroValue[Output](), err + } + + return output, nil +} diff --git a/internal/httpclientx/getjson_test.go b/internal/httpclientx/getjson_test.go new file mode 100644 index 0000000000..61371d9289 --- /dev/null +++ b/internal/httpclientx/getjson_test.go @@ -0,0 +1,220 @@ +package httpclientx + +import ( + "context" + "errors" + "net/http" + "strings" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +type apiResponse struct { + Age int + Name string +} + +func TestGetJSON(t *testing.T) { + t.Run("when GetRaw fails", func(t *testing.T) { + // create a server that RST connections + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // make sure the response is nil. + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("when JSON parsing fails", func(t *testing.T) { + // create a server that returns an invalid JSON type + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("[]")) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err.Error() != "json: cannot unmarshal array into Go value of type httpclientx.apiResponse" { + t.Fatal("unexpected error", err) + } + + // make sure the response is nil. + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("on success", func(t *testing.T) { + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"Name": "simone", "Age": 41}`)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK + expect := &apiResponse{Name: "simone", Age: 41} + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + }) +} + +// This test ensures that GetJSON sets correct HTTP headers +func TestGetJSONHeadersOkay(t *testing.T) { + var ( + gotheaders http.Header + gotmu sync.Mutex + ) + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // save the headers + gotmu.Lock() + gotheaders = r.Header + gotmu.Unlock() + + // send a minimal 200 Ok response + w.WriteHeader(200) + w.Write([]byte(`{}`)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Authorization: "scribai", + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // send the request and receive the response + apiresp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + + // we do not expect to see an error here + if err != nil { + t.Fatal(err) + } + + // given the handler, we expect to see an empty structure here + if apiresp.Age != 0 || apiresp.Name != "" { + t.Fatal("expected empty response") + } + + // make sure there are no data races + defer gotmu.Unlock() + gotmu.Lock() + + // make sure we have sent the authorization header + if value := gotheaders.Get("Authorization"); value != "scribai" { + t.Fatal("unexpected Authorization value", value) + } + + // now make sure we have sent user-agent + if value := gotheaders.Get("User-Agent"); value != model.HTTPHeaderUserAgent { + t.Fatal("unexpected User-Agent value", value) + } + + // now make sure we have sent accept-encoding + if value := gotheaders.Get("Accept-Encoding"); value != "gzip" { + t.Fatal("unexpected Accept-Encoding value", value) + } +} + +// This test ensures GetJSON logs the response body at Debug level. +func TestGetJSONLoggingOkay(t *testing.T) { + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"Name": "simone", "Age": 41}`)) + })) + defer server.Close() + + // instantiate a logger that collects logs + logger := &testingx.Logger{} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: logger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK + expect := &apiResponse{Name: "simone", Age: 41} + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + + // collect and verify the debug lines + debuglines := logger.DebugLines() + t.Log(debuglines) + if len(debuglines) != 1 { + t.Fatal("expected to see a single debug line") + } + if !strings.Contains(debuglines[0], "raw response body:") { + t.Fatal("did not see raw response body log line") + } +} diff --git a/internal/httpclientx/getraw.go b/internal/httpclientx/getraw.go new file mode 100644 index 0000000000..95e8d5e6b5 --- /dev/null +++ b/internal/httpclientx/getraw.go @@ -0,0 +1,32 @@ +package httpclientx + +// +// getraw.go - GET a RAW response. +// + +import ( + "context" + "net/http" +) + +// GetRaw sends a GET request and reads a raw response. +// +// Arguments: +// +// - ctx is the cancellable context; +// +// - config is the config to use; +// +// - URL is the URL to use. +// +// This function either returns an error or a valid Output. +func GetRaw(ctx context.Context, config *Config, URL string) ([]byte, error) { + // construct the request to use + req, err := http.NewRequestWithContext(ctx, "GET", URL, nil) + if err != nil { + return nil, err + } + + // get raw response body + return do(ctx, req, config) +} diff --git a/internal/httpclientx/getraw_test.go b/internal/httpclientx/getraw_test.go new file mode 100644 index 0000000000..2b4be2ba2b --- /dev/null +++ b/internal/httpclientx/getraw_test.go @@ -0,0 +1,175 @@ +package httpclientx + +import ( + "context" + "net/http" + "strings" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +func TestGetRaw(t *testing.T) { + t.Run("when we cannot create a request", func(t *testing.T) { + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + rawrespbody, err := GetRaw( + context.Background(), + config, + "\t", // <- invalid URL that we cannot parse + ) + + t.Log(rawrespbody) + t.Log(err) + + if err.Error() != `parse "\t": net/url: invalid control character in URL` { + t.Fatal("unexpected error", err) + } + + if len(rawrespbody) != 0 { + t.Fatal("expected zero-length body") + } + }) + + t.Run("on success", func(t *testing.T) { + expected := []byte(`Bonsoir, Elliot`) + + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(expected) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + rawrespbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(rawrespbody) + t.Log(err) + + if err != nil { + t.Fatal("unexpected error", err) + } + + if diff := cmp.Diff(expected, rawrespbody); diff != "" { + t.Fatal(diff) + } + }) +} + +// This test ensures that GetRaw sets correct HTTP headers +func TestGetRawHeadersOkay(t *testing.T) { + var ( + gotheaders http.Header + gotmu sync.Mutex + ) + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // save the headers + gotmu.Lock() + gotheaders = r.Header + gotmu.Unlock() + + // send a minimal 200 Ok response + w.WriteHeader(200) + w.Write([]byte(``)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Authorization: "scribai", + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // send the request and receive the response + rawresp, err := GetRaw(context.Background(), config, server.URL) + + // we do not expect to see an error here + if err != nil { + t.Fatal(err) + } + + // make sure the raw response is exactly what we expect to receive + if diff := cmp.Diff([]byte(``), rawresp); diff != "" { + t.Fatal("unexpected raw response") + } + + // make sure there are no data races + defer gotmu.Unlock() + gotmu.Lock() + + // make sure we have sent the authorization header + if value := gotheaders.Get("Authorization"); value != "scribai" { + t.Fatal("unexpected Authorization value", value) + } + + // now make sure we have sent user-agent + if value := gotheaders.Get("User-Agent"); value != model.HTTPHeaderUserAgent { + t.Fatal("unexpected User-Agent value", value) + } + + // now make sure we have sent accept-encoding + if value := gotheaders.Get("Accept-Encoding"); value != "gzip" { + t.Fatal("unexpected Accept-Encoding value", value) + } +} + +// This test ensures GetRaw logs the response body at Debug level. +func TestGetRawLoggingOkay(t *testing.T) { + expected := []byte(`Bonsoir, Elliot`) + + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(expected) + })) + defer server.Close() + + // instantiate a logger that collects logs + logger := &testingx.Logger{} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: logger, + UserAgent: model.HTTPHeaderUserAgent, + } + + rawrespbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(rawrespbody) + t.Log(err) + + if err != nil { + t.Fatal("unexpected error", err) + } + + if diff := cmp.Diff(expected, rawrespbody); diff != "" { + t.Fatal(diff) + } + + // collect and verify the debug lines + debuglines := logger.DebugLines() + t.Log(debuglines) + if len(debuglines) != 1 { + t.Fatal("expected to see a single debug line") + } + if !strings.Contains(debuglines[0], "raw response body:") { + t.Fatal("did not see raw response body log line") + } +} diff --git a/internal/httpclientx/getxml.go b/internal/httpclientx/getxml.go new file mode 100644 index 0000000000..d5c01c5cf5 --- /dev/null +++ b/internal/httpclientx/getxml.go @@ -0,0 +1,39 @@ +package httpclientx + +// +// getxml.go - GET an XML response. +// + +import ( + "context" + "encoding/xml" +) + +// GetXML sends a GET request and reads an XML response. +// +// Arguments: +// +// - ctx is the cancellable context; +// +// - config is the config to use; +// +// - URL is the URL to use. +// +// This function either returns an error or a valid Output. +func GetXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) { + // read the raw body + rawrespbody, err := GetRaw(ctx, config, URL) + + // handle the case of error + if err != nil { + return zeroValue[Output](), err + } + + // parse the response body as JSON + var output Output + if err := xml.Unmarshal(rawrespbody, &output); err != nil { + return zeroValue[Output](), err + } + + return output, nil +} diff --git a/internal/httpclientx/getxml_test.go b/internal/httpclientx/getxml_test.go new file mode 100644 index 0000000000..96e9262a0a --- /dev/null +++ b/internal/httpclientx/getxml_test.go @@ -0,0 +1,216 @@ +package httpclientx + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +func TestGetXML(t *testing.T) { + t.Run("when GetRaw fails", func(t *testing.T) { + // create a server that RST connections + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // make sure the response is nil. + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("when XML parsing fails", func(t *testing.T) { + // create a server that returns an invalid XML file + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("[]")) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, io.EOF) { + t.Fatal("unexpected error", err) + } + + // make sure the response is nil. + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("on success", func(t *testing.T) { + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`simone41`)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK + expect := &apiResponse{Name: "simone", Age: 41} + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + }) +} + +// This test ensures that GetXML sets correct HTTP headers +func TestGetXMLHeadersOkay(t *testing.T) { + var ( + gotheaders http.Header + gotmu sync.Mutex + ) + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // save the headers + gotmu.Lock() + gotheaders = r.Header + gotmu.Unlock() + + // send a minimal 200 Ok response + w.WriteHeader(200) + w.Write([]byte(``)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Authorization: "scribai", + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // send the request and receive the response + apiresp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + + // we do not expect to see an error here + if err != nil { + t.Fatal(err) + } + + // given the handler, we expect to see an empty structure here + if apiresp.Age != 0 || apiresp.Name != "" { + t.Fatal("expected empty response") + } + + // make sure there are no data races + defer gotmu.Unlock() + gotmu.Lock() + + // make sure we have sent the authorization header + if value := gotheaders.Get("Authorization"); value != "scribai" { + t.Fatal("unexpected Authorization value", value) + } + + // now make sure we have sent user-agent + if value := gotheaders.Get("User-Agent"); value != model.HTTPHeaderUserAgent { + t.Fatal("unexpected User-Agent value", value) + } + + // now make sure we have sent accept-encoding + if value := gotheaders.Get("Accept-Encoding"); value != "gzip" { + t.Fatal("unexpected Accept-Encoding value", value) + } +} + +// This test ensures GetXML logs the response body at Debug level. +func TestGetXMLLoggingOkay(t *testing.T) { + // create a server that returns a legit response + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`simone41`)) + })) + defer server.Close() + + // instantiate a logger that collects logs + logger := &testingx.Logger{} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: logger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK + expect := &apiResponse{Name: "simone", Age: 41} + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + + // collect and verify the debug lines + debuglines := logger.DebugLines() + t.Log(debuglines) + if len(debuglines) != 1 { + t.Fatal("expected to see a single debug line") + } + if !strings.Contains(debuglines[0], "raw response body:") { + t.Fatal("did not see raw response body log line") + } +} diff --git a/internal/httpclientx/httpclientx.go b/internal/httpclientx/httpclientx.go new file mode 100644 index 0000000000..b738a85ae0 --- /dev/null +++ b/internal/httpclientx/httpclientx.go @@ -0,0 +1,102 @@ +// Package httpclientx contains extensions to more easily invoke HTTP APIs. +package httpclientx + +// +// httpclientx.go - common code +// + +import ( + "compress/gzip" + "context" + "io" + "net/http" + + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +// ErrRequestFailed indicates that an HTTP request status indicates failure. +type ErrRequestFailed struct { + StatusCode int +} + +var _ error = &ErrRequestFailed{} + +// Error returns the error as a string. +// +// The string returned by this error starts with the httpx prefix for backwards +// compatibility with the legacy httpx package. +func (err *ErrRequestFailed) Error() string { + return "httpx: request failed" +} + +// zeroValue is a convenience function to return the zero value. +func zeroValue[T any]() T { + return *new(T) +} + +// newLimitReader is a wrapper for [io.LimitReader] that automatically +// sets the maximum readable amount of bytes. +func newLimitReader(r io.Reader) io.Reader { + return io.LimitReader(r, 1<<24) +} + +// do is the internal function to finish preparing the request and getting a raw response. +func do(ctx context.Context, req *http.Request, config *Config) ([]byte, error) { + // optionally assign authorization + if value := config.Authorization; value != "" { + req.Header.Set("Authorization", value) + } + + // assign the user agent + req.Header.Set("User-Agent", config.UserAgent) + + // say that we're accepting gzip encoded bodies + req.Header.Set("Accept-Encoding", "gzip") + + // get the response + resp, err := config.Client.Do(req) + + // handle the case of error + if err != nil { + return nil, err + } + + // eventually close the response body + defer resp.Body.Close() + + // Implementation note: here we choose to always read the response + // body before checking the status code because it helps a lot to log + // the response body received on failure when testing a backend + + var baseReader io.Reader = resp.Body + + // handle the case of gzip encoded body + if resp.Header.Get("Content-Encoding") == "gzip" { + gzreader, err := gzip.NewReader(baseReader) + if err != nil { + return nil, err + } + baseReader = gzreader + } + + // protect against unreasonably large response bodies + limitReader := newLimitReader(baseReader) + + // read the raw body + rawrespbody, err := netxlite.ReadAllContext(ctx, limitReader) + + // handle the case of failure + if err != nil { + return nil, err + } + + // log the response body for debugging purposes + config.Logger.Debugf("%s %s: raw response body: %s", req.Method, req.URL.String(), string(rawrespbody)) + + // handle the case of HTTP error + if resp.StatusCode != 200 { + return nil, &ErrRequestFailed{resp.StatusCode} + } + + return rawrespbody, nil +} diff --git a/internal/httpclientx/httpclientx_test.go b/internal/httpclientx/httpclientx_test.go new file mode 100644 index 0000000000..a58477c751 --- /dev/null +++ b/internal/httpclientx/httpclientx_test.go @@ -0,0 +1,144 @@ +package httpclientx + +import ( + "bytes" + "compress/gzip" + "context" + "errors" + "net/http" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +func TestGzipDecompression(t *testing.T) { + t.Run("we correctly handle gzip encoding", func(t *testing.T) { + expected := []byte(`Bonsoir, Elliot!!!`) + + // create a server returning compressed content + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var buffer bytes.Buffer + writer := gzip.NewWriter(&buffer) + _ = runtimex.Try1(writer.Write(expected)) + runtimex.Try0(writer.Close()) + w.Header().Add("Content-Encoding", "gzip") + w.Write(buffer.Bytes()) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // make sure we can read it + respbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(respbody) + t.Log(err) + + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(expected, respbody); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("we correctly handle the case where we cannot decode gzip", func(t *testing.T) { + expected := []byte(`Bonsoir, Elliot!!!`) + + // create a server pretending to return compressed content + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Encoding", "gzip") + w.Write(expected) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // attempt to get a response body + respbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(respbody) + t.Log(err) + + if err.Error() != "gzip: invalid header" { + t.Fatal(err) + } + + if respbody != nil { + t.Fatal("expected nil response body") + } + }) +} + +func TestHTTPStatusCodeHandling(t *testing.T) { + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerBlockpage451()) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + respbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(respbody) + t.Log(err) + + if err.Error() != "httpx: request failed" { + t.Fatal(err) + } + + if respbody != nil { + t.Fatal("expected nil response body") + } + + var orig *ErrRequestFailed + if !errors.As(err, &orig) { + t.Fatal("not an *ErrRequestFailed instance") + } + if orig.StatusCode != 451 { + t.Fatal("unexpected status code", orig.StatusCode) + } +} + +func TestHTTPReadBodyErrorsHandling(t *testing.T) { + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerResetWhileReadingBody()) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + respbody, err := GetRaw(context.Background(), config, server.URL) + + t.Log(respbody) + t.Log(err) + + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("expected ECONNRESET, got", err) + } + + if respbody != nil { + t.Fatal("expected nil response body") + } +} diff --git a/internal/httpclientx/overlapped.go b/internal/httpclientx/overlapped.go new file mode 100644 index 0000000000..f8c72886a5 --- /dev/null +++ b/internal/httpclientx/overlapped.go @@ -0,0 +1,159 @@ +package httpclientx + +// +// overlapped.go - overlapped operations. +// + +import ( + "context" + "errors" + "time" + + "github.com/ooni/probe-cli/v3/internal/erroror" +) + +// TODO(bassosimone): we still need to write tests for overlapped. + +// OverlappedDefaultScheduleInterval is the default schedule interval. After this interval +// has elapsed for a URL without seeing a success, we will schedule the next URL. +const OverlappedDefaultScheduleInterval = 15 * time.Second + +// Overlapped represents the possibility of overlapping HTTP calls for a set of +// functionally equivalent URLs, such that we start a new call if the previous one +// has failed to produce a result within the configured ScheduleInterval. +// +// # Limitations +// +// Under very bad networking conditions, [*Overlapped] would cause a new network +// call to start while the previous one is still in progress and very slowly downloading +// a response. A future implementation SHOULD probably account for this possibility. +type Overlapped[Output any] struct { + // RunFunc is the MANDATORY function that fetches the given URL. + RunFunc func(ctx context.Context, URL string) (Output, error) + + // ScheduleInterval is the MANDATORY scheduling interval. + ScheduleInterval time.Duration +} + +// NewOverlappedGetJSON constructs a [*Overlapped] for calling [GetJSON]. +func NewOverlappedGetJSON[Output any](ctx context.Context, config *Config) *Overlapped[Output] { + return &Overlapped[Output]{ + RunFunc: func(ctx context.Context, URL string) (Output, error) { + return GetJSON[Output](ctx, config, URL) + }, + ScheduleInterval: OverlappedDefaultScheduleInterval, + } +} + +// NewOverlappedGetRaw constructs a [*Overlapped] for calling [GetRaw]. +func NewOverlappedGetRaw(ctx context.Context, config *Config) *Overlapped[[]byte] { + return &Overlapped[[]byte]{ + RunFunc: func(ctx context.Context, URL string) ([]byte, error) { + return GetRaw(ctx, config, URL) + }, + ScheduleInterval: OverlappedDefaultScheduleInterval, + } +} + +// NewOverlappedGetXML constructs a [*Overlapped] for calling [GetXML]. +func NewOverlappedGetXML[Output any](ctx context.Context, config *Config) *Overlapped[Output] { + return &Overlapped[Output]{ + RunFunc: func(ctx context.Context, URL string) (Output, error) { + return GetXML[Output](ctx, config, URL) + }, + ScheduleInterval: OverlappedDefaultScheduleInterval, + } +} + +// NewOverlappedPostJSON constructs a [*Overlapped] for calling [PostJSON]. +func NewOverlappedPostJSON[Input, Output any](ctx context.Context, config *Config, input Input) *Overlapped[Output] { + return &Overlapped[Output]{ + RunFunc: func(ctx context.Context, URL string) (Output, error) { + return PostJSON[Input, Output](ctx, config, URL, input) + }, + ScheduleInterval: OverlappedDefaultScheduleInterval, + } +} + +// ErrGenericOverlappedFailure indicates that a generic [*Overlapped] failure occurred. +var ErrGenericOverlappedFailure = errors.New("overlapped: generic failure") + +// Run runs the overlapped operations, returning the result of the first operation +// that succeeds and otherwise returning an error describing what happened. +// +// # Limitations +// +// This implementation creates a new goroutine for each provided URL under the assumption that +// the overall number of URLs is small. A future revision would address this issue. +func (ovx *Overlapped[Output]) Run(ctx context.Context, URLs ...string) (Output, error) { + // create cancellable context for early cancellation + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + // construct channel for collecting the results + output := make(chan *erroror.Value[Output]) + + // schedule a measuring goroutine per URL. + for idx := 0; idx < len(URLs); idx++ { + go ovx.transact(ctx, idx, URLs[idx], output) + } + + // we expect to see exactly a response for each goroutine + var ( + firstOutput *Output + errorv []error + ) + for idx := 0; idx < len(URLs); idx++ { + // get a result from one of the goroutines + result := <-output + + // handle the error case + if result.Err != nil { + errorv = append(errorv, result.Err) + continue + } + + // possibly record the first success + if firstOutput == nil { + firstOutput = &result.Value + } + + // make sure we interrupt all the other goroutines + cancel() + } + + // handle the case of success + if firstOutput != nil { + return *firstOutput, nil + } + + // handle the case where there's no error + if len(errorv) <= 0 { + errorv = append(errorv, ErrGenericOverlappedFailure) + } + + // return zero value and errors list + return *new(Output), errors.Join(errorv...) +} + +// transact performs an HTTP transaction with the given URL and writes results to the output channel. +func (ovx *Overlapped[Output]) transact(ctx context.Context, idx int, URL string, output chan<- *erroror.Value[Output]) { + // wait for our time to start + // + // add one nanosecond to make sure the delay is always positive + timer := time.NewTimer(time.Duration(idx)*ovx.ScheduleInterval + time.Nanosecond) + defer timer.Stop() + select { + case <-ctx.Done(): + output <- &erroror.Value[Output]{Err: ctx.Err()} + return + case <-timer.C: + // fallthrough + } + + // obtain the results + value, err := ovx.RunFunc(ctx, URL) + + // emit the results + output <- &erroror.Value[Output]{Err: err, Value: value} +} diff --git a/internal/httpclientx/postjson.go b/internal/httpclientx/postjson.go new file mode 100644 index 0000000000..3f0b0f50bb --- /dev/null +++ b/internal/httpclientx/postjson.go @@ -0,0 +1,61 @@ +package httpclientx + +// +// postjson.go - POST a JSON request and read a JSON response. +// + +import ( + "bytes" + "context" + "encoding/json" + "net/http" +) + +// PostJSON sends a POST request with a JSON body and reads a JSON response. +// +// Arguments: +// +// - ctx is the cancellable context; +// +// - config is the config to use; +// +// - URL is the URL to use; +// +// - input is the input structure to JSON serialize as the request body. +// +// This function either returns an error or a valid Output. +func PostJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) { + // serialize the request body + rawreqbody, err := json.Marshal(input) + if err != nil { + return zeroValue[Output](), err + } + + // log the raw request body + config.Logger.Debugf("POST %s: raw request body: %s", URL, string(rawreqbody)) + + // construct the request to use + req, err := http.NewRequestWithContext(ctx, "POST", URL, bytes.NewReader(rawreqbody)) + if err != nil { + return zeroValue[Output](), err + } + + // assign the content type + req.Header.Set("Content-Type", "application/json") + + // get the raw response body + rawrespbody, err := do(ctx, req, config) + + // handle the case of error + if err != nil { + return zeroValue[Output](), err + } + + // parse the response body as JSON + var output Output + if err := json.Unmarshal(rawrespbody, &output); err != nil { + return zeroValue[Output](), err + } + + return output, nil +} diff --git a/internal/httpclientx/postjson_test.go b/internal/httpclientx/postjson_test.go new file mode 100644 index 0000000000..468ba35862 --- /dev/null +++ b/internal/httpclientx/postjson_test.go @@ -0,0 +1,314 @@ +package httpclientx + +import ( + "context" + "errors" + "net/http" + "strings" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +type apiRequest struct { + UserID int +} + +func TestPostJSON(t *testing.T) { + t.Run("when we cannot marshal the request body", func(t *testing.T) { + // a channel cannot be serialized + req := make(chan int) + close(req) + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[chan int, *apiResponse](context.Background(), config, "", req) + + t.Log(resp) + t.Log(err) + + if err.Error() != `json: unsupported type: chan int` { + t.Fatal("unexpected error", err) + } + + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when we cannot create a request", func(t *testing.T) { + req := &apiRequest{117} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[*apiRequest, *apiResponse]( + context.Background(), + config, + "\t", // <- invalid URL that we cannot parse + req, + ) + + t.Log(resp) + t.Log(err) + + if err.Error() != `parse "\t": net/url: invalid control character in URL` { + t.Fatal("unexpected error", err) + } + + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("in case of HTTP failure", func(t *testing.T) { + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer server.Close() + + req := &apiRequest{117} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + + t.Log(resp) + t.Log(err) + + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when we cannot parse the response body", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("[]")) + })) + defer server.Close() + + req := &apiRequest{117} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err.Error() != "json: cannot unmarshal array into Go value of type httpclientx.apiResponse" { + t.Fatal("unexpected error", err) + } + + // make sure the response is nil. + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("on success", func(t *testing.T) { + req := &apiRequest{117} + + expect := &apiResponse{Name: "simone", Age: 41} + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var gotreq apiRequest + data := runtimex.Try1(netxlite.ReadAllContext(r.Context(), r.Body)) + must.UnmarshalJSON(data, &gotreq) + if gotreq.UserID != req.UserID { + w.WriteHeader(404) + return + } + w.Write(must.MarshalJSON(expect)) + })) + defer server.Close() + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK. + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + }) +} + +// This test ensures that PostJSON sets correct HTTP headers and sends the right body. +func TestPostJSONCommunicationOkay(t *testing.T) { + var ( + gotheaders http.Header + gotrawbody []byte + gotmu sync.Mutex + ) + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // read the raw response body + rawbody := runtimex.Try1(netxlite.ReadAllContext(r.Context(), r.Body)) + + // save the raw response body and headers + gotmu.Lock() + gotrawbody = rawbody + gotheaders = r.Header + gotmu.Unlock() + + // send a minimal 200 Ok response + w.WriteHeader(200) + w.Write([]byte(`{}`)) + })) + defer server.Close() + + // create and serialize the expected request body + apireq := &apiRequest{ + UserID: 117, + } + rawapireq := must.MarshalJSON(apireq) + + // create API call config + config := &Config{ + Authorization: "scribai", + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + } + + // send the request and receive the response + apiresp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, apireq) + + // we do not expect to see an error here + if err != nil { + t.Fatal(err) + } + + // given the handler, we expect to see an empty structure here + if apiresp.Age != 0 || apiresp.Name != "" { + t.Fatal("expected empty response") + } + + // make sure there are no data races + defer gotmu.Unlock() + gotmu.Lock() + + // now verify what the handler has read as the raw request body + if diff := cmp.Diff(rawapireq, gotrawbody); diff != "" { + t.Fatal(diff) + } + + // make sure we have sent the authorization header + if value := gotheaders.Get("Authorization"); value != "scribai" { + t.Fatal("unexpected Authorization value", value) + } + + // now make sure we have sent content-type + if value := gotheaders.Get("Content-Type"); value != "application/json" { + t.Fatal("unexpected Content-Type value", value) + } + + // now make sure we have sent user-agent + if value := gotheaders.Get("User-Agent"); value != model.HTTPHeaderUserAgent { + t.Fatal("unexpected User-Agent value", value) + } + + // now make sure we have sent accept-encoding + if value := gotheaders.Get("Accept-Encoding"); value != "gzip" { + t.Fatal("unexpected Accept-Encoding value", value) + } +} + +// This test ensures PostJSON logs the request and response body at Debug level. +func TestPostJSONLoggingOkay(t *testing.T) { + req := &apiRequest{117} + + expect := &apiResponse{Name: "simone", Age: 41} + + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var gotreq apiRequest + data := runtimex.Try1(netxlite.ReadAllContext(r.Context(), r.Body)) + must.UnmarshalJSON(data, &gotreq) + if gotreq.UserID != req.UserID { + w.WriteHeader(404) + return + } + w.Write(must.MarshalJSON(expect)) + })) + defer server.Close() + + // instantiate a logger that collects logs + logger := &testingx.Logger{} + + // create API call config + config := &Config{ + Client: http.DefaultClient, + Logger: logger, + UserAgent: model.HTTPHeaderUserAgent, + } + + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if err != nil { + t.Fatal("unexpected error", err) + } + + // make sure the response is OK. + if diff := cmp.Diff(expect, resp); diff != "" { + t.Fatal(diff) + } + + // collect and verify the debug lines + debuglines := logger.DebugLines() + t.Log(debuglines) + if len(debuglines) != 2 { + t.Fatal("expected to see a single debug line") + } + if !strings.Contains(debuglines[0], "raw request body:") { + t.Fatal("did not see raw request body log line") + } + if !strings.Contains(debuglines[1], "raw response body:") { + t.Fatal("did not see raw response body log line") + } +} diff --git a/internal/httpclientx/singlecall.go b/internal/httpclientx/singlecall.go new file mode 100644 index 0000000000..00d5d01284 --- /dev/null +++ b/internal/httpclientx/singlecall.go @@ -0,0 +1,5 @@ +package httpclientx + +// SingleCall represents a single API call. +type SingleCall struct { +} diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index a6d82a5c7d..66993d88f8 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -14,7 +14,7 @@ import ( "github.com/hexops/gotextdiff" "github.com/hexops/gotextdiff/myers" "github.com/hexops/gotextdiff/span" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" @@ -67,21 +67,7 @@ var ErrHTTPRequestFailed = errors.New("oonirun: HTTP request failed") // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, logger model.Logger, URL string) (*V2Descriptor, error) { - template := httpx.APIClientTemplate{ - Accept: "", - Authorization: "", - BaseURL: URL, - HTTPClient: client, - Host: "", - LogBody: true, - Logger: logger, - UserAgent: model.HTTPHeaderUserAgent, - } - var desc V2Descriptor - if err := template.Build().GetJSON(ctx, "", &desc); err != nil { - return nil, err - } - return &desc, nil + return httpclientx.GetJSON[*V2Descriptor](ctx, URL, client, logger, model.HTTPHeaderUserAgent) } // v2DescriptorCache contains all the known v2Descriptor entries. diff --git a/internal/probeservices/bouncer.go b/internal/probeservices/bouncer.go index 7ddc3fd941..2e0cf78247 100644 --- a/internal/probeservices/bouncer.go +++ b/internal/probeservices/bouncer.go @@ -1,14 +1,27 @@ package probeservices +// +// bouncer.go - GET /api/v1/test-helpers +// + import ( "context" + "net/url" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" ) -// GetTestHelpers is like GetCollectors but for test helpers. -func (c Client) GetTestHelpers( - ctx context.Context) (output map[string][]model.OOAPIService, err error) { - err = c.APIClientTemplate.WithBodyLogging().Build().GetJSON(ctx, "/api/v1/test-helpers", &output) - return +// GetTestHelpers queries the /api/v1/test-helpers API. +func (c *Client) GetTestHelpers(ctx context.Context) (map[string][]model.OOAPIService, error) { + // construct the URL to use + URL, err := url.Parse(c.BaseURL) + if err != nil { + return nil, err + } + URL.Path = "/api/v1/test-helpers" + + // get the response + return httpclientx.GetJSON[map[string][]model.OOAPIService]( + ctx, URL.String(), c.HTTPClient, c.Logger, c.UserAgent) } diff --git a/internal/probeservices/checkin.go b/internal/probeservices/checkin.go index 08b12583e7..775311fa70 100644 --- a/internal/probeservices/checkin.go +++ b/internal/probeservices/checkin.go @@ -1,12 +1,15 @@ package probeservices +// +// checkin.go - POST /api/v1/check-in +// + import ( "context" + "net/url" - "github.com/ooni/probe-cli/v3/internal/checkincache" - "github.com/ooni/probe-cli/v3/internal/httpapi" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/ooapi" ) // CheckIn function is called by probes asking if there are tests to be run @@ -15,19 +18,15 @@ import ( // track selected parts of the check-in API response. // Returns the list of tests to run and the URLs, on success, // or an explanatory error, in case of failure. -func (c Client) CheckIn( - ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { - // prepare endpoint and descriptor for the API call - epnt := c.newHTTPAPIEndpoint() - desc := ooapi.NewDescriptorCheckIn(&config) - - // issue the API call and handle failures - resp, err := httpapi.Call(ctx, desc, epnt) +func (c *Client) CheckIn(ctx context.Context, input model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { + // construct the URL to use + URL, err := url.Parse(c.BaseURL) if err != nil { return nil, err } + URL.Path = "/api/v1/check-in" - // make sure we track selected parts of the response - _ = checkincache.Store(c.KVStore, resp) - return resp, nil + // get the response + return httpclientx.PostJSON[*model.OOAPICheckInConfig, *model.OOAPICheckInResult]( + ctx, URL.String(), c.HTTPClient, &input, c.Logger, c.UserAgent) } diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go index 46c00fdaf6..fdb59eecf3 100644 --- a/internal/probeservices/measurementmeta.go +++ b/internal/probeservices/measurementmeta.go @@ -1,33 +1,39 @@ package probeservices +// +// measurementmeta.go - GET /api/v1/measurement_meta +// + import ( "context" "net/url" - "github.com/ooni/probe-cli/v3/internal/httpx" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" ) // GetMeasurementMeta returns meta information about a measurement. func (c Client) GetMeasurementMeta( - ctx context.Context, config model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { + ctx context.Context, input *model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { + // construct the query to use query := url.Values{} - query.Add("report_id", config.ReportID) - if config.Input != "" { - query.Add("input", config.Input) + query.Add("report_id", input.ReportID) + if input.Input != "" { + query.Add("input", input.Input) } - if config.Full { + if input.Full { query.Add("full", "true") } - var response model.OOAPIMeasurementMeta - err := (&httpx.APIClientTemplate{ - BaseURL: c.BaseURL, - HTTPClient: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }).WithBodyLogging().Build().GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response) + + // construct the URL to use + URL, err := url.Parse(c.BaseURL) if err != nil { return nil, err } - return &response, nil + URL.Path = "/api/v1/measurement_meta" + URL.RawQuery = query.Encode() + + // get the response + return httpclientx.GetJSON[*model.OOAPIMeasurementMeta]( + ctx, URL.String(), c.HTTPClient, c.Logger, c.UserAgent) } diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go index 9c75c9558f..d28fee726d 100644 --- a/internal/probeservices/register.go +++ b/internal/probeservices/register.go @@ -1,12 +1,33 @@ package probeservices +// +// register.go - POST /api/v1/register +// + import ( "context" + "net/url" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/randx" ) +// Register invokes the /api/v1/register API. +func (c *Client) Register( + ctx context.Context, input *model.OOAPIRegisterRequest) (*model.OOAPIRegisterResponse, error) { + // construct the URL to use + URL, err := url.Parse(c.BaseURL) + if err != nil { + return nil, err + } + URL.Path = "/api/v1/register" + + // get the response + return httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( + ctx, URL.String(), c.HTTPClient, input, c.Logger, c.UserAgent) +} + // MaybeRegister registers this client if not already registered func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMetadata) error { if !metadata.Valid() { diff --git a/internal/testingx/httptestx.go b/internal/testingx/httptestx.go index 2129794914..5a8ce91290 100644 --- a/internal/testingx/httptestx.go +++ b/internal/testingx/httptestx.go @@ -6,8 +6,10 @@ import ( "net" "net/http" "net/url" + "time" "github.com/ooni/netem" + "github.com/ooni/probe-cli/v3/internal/randx" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -173,14 +175,7 @@ func HTTPHandlerTimeout() http.Handler { } func httpHandlerHijack(w http.ResponseWriter, r *http.Request, policy string) { - // Note: - // - // 1. we assume we can hihack the connection - // - // 2. Hijack won't fail the first time it's invoked - hijacker := w.(http.Hijacker) - conn, _ := runtimex.Try2(hijacker.Hijack()) - + conn := httpHijack(w) defer conn.Close() switch policy { @@ -194,3 +189,40 @@ func httpHandlerHijack(w http.ResponseWriter, r *http.Request, policy string) { // nothing } } + +// HTTPHandlerResetWhileReadingBody returns a handler that sends a +// connection reset by peer while the client is reading the body. +func HTTPHandlerResetWhileReadingBody() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn := httpHijack(w) + defer conn.Close() + + // write the HTTP response headers + conn.Write([]byte("HTTP/1.1 200 Ok\r\n")) + conn.Write([]byte("Content-Type: text/html\r\n")) + conn.Write([]byte("Content-Length: 65535\r\n")) + conn.Write([]byte("\r\n")) + + // start writing the response + content := randx.Letters(32768) + conn.Write([]byte(content)) + + // sleep for half a second simulating something wrong + time.Sleep(500 * time.Millisecond) + + // finally issue reset for the conn + tcpMaybeResetNetConn(conn) + }) +} + +// httpHijack is a convenience function to hijack the underlying connection. +func httpHijack(w http.ResponseWriter) net.Conn { + // Note: + // + // 1. we assume we can hihack the connection + // + // 2. Hijack won't fail the first time it's invoked + hijacker := w.(http.Hijacker) + conn, _ := runtimex.Try2(hijacker.Hijack()) + return conn +} diff --git a/internal/testingx/httptestx_test.go b/internal/testingx/httptestx_test.go index 57211ce70d..54df90e9ea 100644 --- a/internal/testingx/httptestx_test.go +++ b/internal/testingx/httptestx_test.go @@ -498,3 +498,43 @@ func TestHTTPTestxWithNetem(t *testing.T) { }) } } + +func TestHTTPHandlerResetWhileReadingBody(t *testing.T) { + // create a server for testing the given handler + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerResetWhileReadingBody()) + defer server.Close() + + // create a suitable HTTP transport using netxlite + netx := &netxlite.Netx{Underlying: nil} + dialer := netx.NewDialerWithoutResolver(log.Log) + handshaker := netx.NewTLSHandshakerStdlib(log.Log) + tlsDialer := netxlite.NewTLSDialer(dialer, handshaker) + txp := netxlite.NewHTTPTransportWithOptions(log.Log, dialer, tlsDialer) + + // create the request + req := runtimex.Try1(http.NewRequest("GET", server.URL, nil)) + + // perform the round trip + resp, err := txp.RoundTrip(req) + + // we do not expect an error during the round trip + if err != nil { + t.Fatal(err) + } + + // make sure we close the body + defer resp.Body.Close() + + // start reading the response where we expect to see a RST + respbody, err := netxlite.ReadAllContext(req.Context(), resp.Body) + + // verify we received a connection reset + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("expected ECONNRESET, got", err) + } + + // make sure we've got no bytes + if len(respbody) != 0 { + t.Fatal("expected to see zero bytes here") + } +} diff --git a/internal/testingx/logger.go b/internal/testingx/logger.go new file mode 100644 index 0000000000..8819cd8849 --- /dev/null +++ b/internal/testingx/logger.go @@ -0,0 +1,87 @@ +package testingx + +import ( + "fmt" + "sync" + + "github.com/ooni/probe-cli/v3/internal/logmodel" +) + +// Logger implements [logmodel.Logger] and collects all the log lines. +// +// The zero value of this struct is ready to use. +type Logger struct { + // debug contains debug lines. + debug []string + + // info contains info lines. + info []string + + // mu provides mutual exclusion. + mu sync.Mutex + + // warning contains warning lines. + warning []string +} + +var _ logmodel.Logger = &Logger{} + +// Debug implements logmodel.Logger. +func (l *Logger) Debug(msg string) { + l.mu.Lock() + l.debug = append(l.debug, msg) + l.mu.Unlock() +} + +// Debugf implements logmodel.Logger. +func (l *Logger) Debugf(format string, v ...interface{}) { + l.Debug(fmt.Sprintf(format, v...)) +} + +// Info implements logmodel.Logger. +func (l *Logger) Info(msg string) { + l.mu.Lock() + l.info = append(l.info, msg) + l.mu.Unlock() +} + +// Infof implements logmodel.Logger. +func (l *Logger) Infof(format string, v ...interface{}) { + l.Info(fmt.Sprintf(format, v...)) +} + +// Warn implements logmodel.Logger. +func (l *Logger) Warn(msg string) { + l.mu.Lock() + l.warning = append(l.warning, msg) + l.mu.Unlock() +} + +// Warnf implements logmodel.Logger. +func (l *Logger) Warnf(format string, v ...interface{}) { + l.Warn(fmt.Sprintf(format, v...)) +} + +// DebugLines returns a copy of the observed debug lines. +func (l *Logger) DebugLines() []string { + l.mu.Lock() + out := append([]string{}, l.debug...) + l.mu.Unlock() + return out +} + +// InfoLines returns a copy of the observed info lines. +func (l *Logger) InfoLines() []string { + l.mu.Lock() + out := append([]string{}, l.info...) + l.mu.Unlock() + return out +} + +// WarnLines returns a copy of the observed warn lines. +func (l *Logger) WarnLines() []string { + l.mu.Lock() + out := append([]string{}, l.warning...) + l.mu.Unlock() + return out +} diff --git a/internal/testingx/logger_test.go b/internal/testingx/logger_test.go new file mode 100644 index 0000000000..51a4086bb1 --- /dev/null +++ b/internal/testingx/logger_test.go @@ -0,0 +1,33 @@ +package testingx + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestLogger(t *testing.T) { + logger := &Logger{} + + logger.Debug("foobar") + logger.Debugf("foo%s", "baz") + expectDebug := []string{"foobar", "foobaz"} + + logger.Info("barfoo") + logger.Infof("bar%s", "baz") + expectInfo := []string{"barfoo", "barbaz"} + + logger.Warn("jarjar") + logger.Warnf("jar%s", "baz") + expectWarn := []string{"jarjar", "jarbaz"} + + if diff := cmp.Diff(expectDebug, logger.DebugLines()); diff != "" { + t.Fatal(diff) + } + if diff := cmp.Diff(expectInfo, logger.InfoLines()); diff != "" { + t.Fatal(diff) + } + if diff := cmp.Diff(expectWarn, logger.WarnLines()); diff != "" { + t.Fatal(diff) + } +} From f9210ec7754b617b0688762a6bd8720f845dee20 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 11:13:11 +0200 Subject: [PATCH 02/22] refactor to make testing the whole package easier --- internal/httpclientx/DESIGN.md | 10 +- internal/httpclientx/getjson.go | 4 + internal/httpclientx/getraw.go | 4 + internal/httpclientx/getxml.go | 4 + internal/httpclientx/overlapped.go | 65 +++---- internal/httpclientx/overlapped_test.go | 215 ++++++++++++++++++++++++ internal/httpclientx/postjson.go | 4 + 7 files changed, 270 insertions(+), 36 deletions(-) create mode 100644 internal/httpclientx/overlapped_test.go diff --git a/internal/httpclientx/DESIGN.md b/internal/httpclientx/DESIGN.md index 1a69110aeb..97d7a0bee1 100644 --- a/internal/httpclientx/DESIGN.md +++ b/internal/httpclientx/DESIGN.md @@ -124,13 +124,13 @@ for the first endpoint to complete. We allow for overlapped operations by defining these constructors: ```Go -func NewOverlappedGetJSON[Output any](ctx context.Context, config *Config) *Overlapped[Output] +func NewOverlappedGetJSON[Output any](config *Config) *Overlapped[Output] -func NewOverlappedGetRaw(ctx context.Context, config *Config) *Overlapped[[]byte] +func NewOverlappedGetRaw(config *Config) *Overlapped[[]byte] -func NewOverlappedGetXML[Output any](ctx context.Context, config *Config) *Overlapped[Output] +func NewOverlappedGetXML[Output any](config *Config) *Overlapped[Output] -func NewOverlappedPostJSON[Input, Output any](ctx context.Context, config *Config, input Input) *Overlapped[Output] +func NewOverlappedPostJSON[Input, Output any](config *Config, input Input) *Overlapped[Output] ``` They all construct the same `*Overlapped` struct, which looks like this: @@ -144,7 +144,7 @@ type Overlapped[Output any] struct { ``` The constructor configures `RunFunc` to invoke the call corresponding to the construct -name (i.e., `NewOverlappedGetXML` configures `RunFunc` to be `GetXML`). +name (i.e., `NewOverlappedGetXML` configures `RunFunc` to run `GetXML`). Then, we define the following method: diff --git a/internal/httpclientx/getjson.go b/internal/httpclientx/getjson.go index 11cf21a35c..971edaf23d 100644 --- a/internal/httpclientx/getjson.go +++ b/internal/httpclientx/getjson.go @@ -21,6 +21,10 @@ import ( // // This function either returns an error or a valid Output. func GetJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) { + return NewOverlappedGetJSON[Output](config).Run(ctx, URL) +} + +func getJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) { // read the raw body rawrespbody, err := GetRaw(ctx, config, URL) diff --git a/internal/httpclientx/getraw.go b/internal/httpclientx/getraw.go index 95e8d5e6b5..0445f877cf 100644 --- a/internal/httpclientx/getraw.go +++ b/internal/httpclientx/getraw.go @@ -21,6 +21,10 @@ import ( // // This function either returns an error or a valid Output. func GetRaw(ctx context.Context, config *Config, URL string) ([]byte, error) { + return NewOverlappedGetRaw(config).Run(ctx, URL) +} + +func getRaw(ctx context.Context, config *Config, URL string) ([]byte, error) { // construct the request to use req, err := http.NewRequestWithContext(ctx, "GET", URL, nil) if err != nil { diff --git a/internal/httpclientx/getxml.go b/internal/httpclientx/getxml.go index d5c01c5cf5..7681bd172a 100644 --- a/internal/httpclientx/getxml.go +++ b/internal/httpclientx/getxml.go @@ -21,6 +21,10 @@ import ( // // This function either returns an error or a valid Output. func GetXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) { + return NewOverlappedGetXML[Output](config).Run(ctx, URL) +} + +func getXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) { // read the raw body rawrespbody, err := GetRaw(ctx, config, URL) diff --git a/internal/httpclientx/overlapped.go b/internal/httpclientx/overlapped.go index f8c72886a5..a37fee3bfc 100644 --- a/internal/httpclientx/overlapped.go +++ b/internal/httpclientx/overlapped.go @@ -12,8 +12,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/erroror" ) -// TODO(bassosimone): we still need to write tests for overlapped. - // OverlappedDefaultScheduleInterval is the default schedule interval. After this interval // has elapsed for a URL without seeing a success, we will schedule the next URL. const OverlappedDefaultScheduleInterval = 15 * time.Second @@ -29,50 +27,55 @@ const OverlappedDefaultScheduleInterval = 15 * time.Second // a response. A future implementation SHOULD probably account for this possibility. type Overlapped[Output any] struct { // RunFunc is the MANDATORY function that fetches the given URL. + // + // This field is typically initialized by [NewOverlappedGetJSON], [NewOverlappedGetRaw], + // [NewOverlappedGetXML], or [NewOverlappedPostJSON] to be the proper function that + // makes sense for the operation that you requested with the constructor. + // + // If you set it manually, you MUST modify it before calling [*Overlapped.Run]. RunFunc func(ctx context.Context, URL string) (Output, error) // ScheduleInterval is the MANDATORY scheduling interval. + // This field is typically initialized by [NewOverlappedGetJSON], [NewOverlappedGetRaw], + // [NewOverlappedGetXML], or [NewOverlappedPostJSON] to be [OverlappedDefaultScheduleInterval]. + // + // If you set it manually, you MUST modify it before calling [*Overlapped.Run]. ScheduleInterval time.Duration } -// NewOverlappedGetJSON constructs a [*Overlapped] for calling [GetJSON]. -func NewOverlappedGetJSON[Output any](ctx context.Context, config *Config) *Overlapped[Output] { +func newOverlappedWithFunc[Output any](fx func(context.Context, string) (Output, error)) *Overlapped[Output] { return &Overlapped[Output]{ - RunFunc: func(ctx context.Context, URL string) (Output, error) { - return GetJSON[Output](ctx, config, URL) - }, + RunFunc: fx, ScheduleInterval: OverlappedDefaultScheduleInterval, } } -// NewOverlappedGetRaw constructs a [*Overlapped] for calling [GetRaw]. -func NewOverlappedGetRaw(ctx context.Context, config *Config) *Overlapped[[]byte] { - return &Overlapped[[]byte]{ - RunFunc: func(ctx context.Context, URL string) ([]byte, error) { - return GetRaw(ctx, config, URL) - }, - ScheduleInterval: OverlappedDefaultScheduleInterval, - } +// NewOverlappedGetJSON constructs a [*Overlapped] for calling [GetJSON] with multiple URLs. +func NewOverlappedGetJSON[Output any](config *Config) *Overlapped[Output] { + return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { + return getJSON[Output](ctx, config, URL) + }) } -// NewOverlappedGetXML constructs a [*Overlapped] for calling [GetXML]. -func NewOverlappedGetXML[Output any](ctx context.Context, config *Config) *Overlapped[Output] { - return &Overlapped[Output]{ - RunFunc: func(ctx context.Context, URL string) (Output, error) { - return GetXML[Output](ctx, config, URL) - }, - ScheduleInterval: OverlappedDefaultScheduleInterval, - } +// NewOverlappedGetRaw constructs a [*Overlapped] for calling [GetRaw] with multiple URLs. +func NewOverlappedGetRaw(config *Config) *Overlapped[[]byte] { + return newOverlappedWithFunc(func(ctx context.Context, URL string) ([]byte, error) { + return getRaw(ctx, config, URL) + }) } -// NewOverlappedPostJSON constructs a [*Overlapped] for calling [PostJSON]. -func NewOverlappedPostJSON[Input, Output any](ctx context.Context, config *Config, input Input) *Overlapped[Output] { - return &Overlapped[Output]{ - RunFunc: func(ctx context.Context, URL string) (Output, error) { - return PostJSON[Input, Output](ctx, config, URL, input) - }, - ScheduleInterval: OverlappedDefaultScheduleInterval, - } +// NewOverlappedGetXML constructs a [*Overlapped] for calling [GetXML] with multiple URLs. +func NewOverlappedGetXML[Output any](config *Config) *Overlapped[Output] { + return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { + return getXML[Output](ctx, config, URL) + }) +} + +// NewOverlappedPostJSON constructs a [*Overlapped] for calling [PostJSON] with multiple URLs. +func NewOverlappedPostJSON[Input, Output any](config *Config, input Input) *Overlapped[Output] { + return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { + return postJSON[Input, Output](ctx, config, URL, input) + }) } // ErrGenericOverlappedFailure indicates that a generic [*Overlapped] failure occurred. diff --git a/internal/httpclientx/overlapped_test.go b/internal/httpclientx/overlapped_test.go new file mode 100644 index 0000000000..0fd1afb64a --- /dev/null +++ b/internal/httpclientx/overlapped_test.go @@ -0,0 +1,215 @@ +package httpclientx + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/apex/log" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +// Implementation note: because top-level functions such as GetRaw always use +// an [*Overlapped], we do not necessarily need to test that each top-level constructor +// are WAI; rather, we should focus on the mechanics of multiple URLs. + +func TestNewOverlappedPostJSONIsPerformingOverlappedCalls(t *testing.T) { + + // Scenario: + // + // - 0.th.ooni.org is SNI blocked + // - 1.th.ooni.org is SNI blocked + // - 2.th.ooni.org is SNI blocked + // - 3.th.ooni.org WAIs + + zeroTh := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer zeroTh.Close() + + oneTh := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer oneTh.Close() + + twoTh := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer twoTh.Close() + + expectedResponse := &apiResponse{ + Age: 41, + Name: "sbs", + } + + threeTh := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(must.MarshalJSON(expectedResponse)) + })) + defer threeTh.Close() + + // Create client configuration. We don't care much about the + // JSON requests and reponses being aligned to reality. + + apiReq := &apiRequest{ + UserID: 117, + } + + config := &Config{ + Authorization: "", // not relevant for this test + Client: http.DefaultClient, + Logger: log.Log, + UserAgent: model.HTTPHeaderUserAgent, + } + + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + + // make sure we set a low scheduling interval to make test faster + overlapped.ScheduleInterval = time.Second + + // Now we issue the requests and check we're getting the correct response. + + apiResp, err := overlapped.Run( + context.Background(), + zeroTh.URL, + oneTh.URL, + twoTh.URL, + threeTh.URL, + ) + + // we do not expect to see a failure because threeTh is WAI + if err != nil { + t.Fatal(err) + } + + // compare response to expectation + if diff := cmp.Diff(expectedResponse, apiResp); diff != "" { + t.Fatal(diff) + } +} + +func TestNewOverlappedPostJSONCancelsPendingCalls(t *testing.T) { + + // Scenario: + // + // - 0.th.ooni.org is WAI but slow + // - 1.th.ooni.org is WAI + // - 2.th.ooni.org is WAI + // - 3.th.ooni.org is WAI + + expectedResponse := &apiResponse{ + Age: 41, + Name: "sbs", + } + + slowwakeup := make(chan any) + + zeroTh := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-slowwakeup + w.Write(must.MarshalJSON(expectedResponse)) + })) + defer zeroTh.Close() + + oneTh := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-slowwakeup + w.Write(must.MarshalJSON(expectedResponse)) + })) + defer oneTh.Close() + + twoTh := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-slowwakeup + w.Write(must.MarshalJSON(expectedResponse)) + })) + defer twoTh.Close() + + threeTh := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-slowwakeup + w.Write(must.MarshalJSON(expectedResponse)) + })) + defer threeTh.Close() + + // Create client configuration. We don't care much about the + // JSON requests and reponses being aligned to reality. + + apiReq := &apiRequest{ + UserID: 117, + } + + config := &Config{ + Authorization: "", // not relevant for this test + Client: http.DefaultClient, + Logger: log.Log, + UserAgent: model.HTTPHeaderUserAgent, + } + + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + + // make sure the schedule interval is high because we want + // all the goroutines but the first to be waiting for permission + // to fetch from their respective URLs. + overlapped.ScheduleInterval = 15 * time.Second + + // In the background we're going to emit slow wakeup signals at fixed intervals + // after an initial waiting interval, such that goroutines unblock in order + + go func() { + time.Sleep(250 * time.Millisecond) + for idx := 0; idx < 4; idx++ { + slowwakeup <- true + time.Sleep(250 * time.Millisecond) + } + close(slowwakeup) + }() + + // Now we issue the requests and check we're getting the correct response. + + apiResp, err := overlapped.Run( + context.Background(), + zeroTh.URL, + oneTh.URL, + twoTh.URL, + threeTh.URL, + ) + + // we do not expect to see a failure because threeTh is WAI + if err != nil { + t.Fatal(err) + } + + // compare response to expectation + if diff := cmp.Diff(expectedResponse, apiResp); diff != "" { + t.Fatal(diff) + } +} + +func TestNewOverlappedPostJSONWithNoURLs(t *testing.T) { + + // Create client configuration. We don't care much about the + // JSON requests and reponses being aligned to reality. + + apiReq := &apiRequest{ + UserID: 117, + } + + config := &Config{ + Authorization: "", // not relevant for this test + Client: http.DefaultClient, + Logger: log.Log, + UserAgent: model.HTTPHeaderUserAgent, + } + + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + + // Now we issue the requests without any URLs and make sure + // the result we get is the generic overlapped error + + apiResp, err := overlapped.Run(context.Background()) + + // we do not expect to see a failure because threeTh is WAI + if !errors.Is(err, ErrGenericOverlappedFailure) { + t.Fatal("unexpected error", err) + } + + // we expect a nil response + if apiResp != nil { + t.Fatal("expected nil API response") + } +} diff --git a/internal/httpclientx/postjson.go b/internal/httpclientx/postjson.go index 3f0b0f50bb..2073a920a0 100644 --- a/internal/httpclientx/postjson.go +++ b/internal/httpclientx/postjson.go @@ -25,6 +25,10 @@ import ( // // This function either returns an error or a valid Output. func PostJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) { + return NewOverlappedPostJSON[Input, Output](config, input).Run(ctx, URL) +} + +func postJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) { // serialize the request body rawreqbody, err := json.Marshal(input) if err != nil { From 5c953f0be2327e9aebd9e9d761a815f5a1e9ad53 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 14:02:41 +0200 Subject: [PATCH 03/22] x --- internal/engine/session.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index 961a2b6c11..855191f95a 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -645,13 +645,7 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error { } s.logger.Infof("session: using probe services: %+v", selected.Endpoint) s.selectedProbeService = &selected.Endpoint - s.availableTestHelpers = map[string][]model.OOAPIService{ - "web-connectivity": {{ - Address: "https://4.th.ooni.org/", - Type: "https", - Front: "", - }}, - } + s.availableTestHelpers = selected.TestHelpers return nil } From e03e8101bbe237cd26c87214c09532853db13c96 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 14:47:02 +0200 Subject: [PATCH 04/22] x --- internal/cmd/apitool/main.go | 2 +- internal/enginelocate/cloudflare.go | 13 +++--- internal/enginelocate/ubuntu.go | 13 +++--- internal/httpclientx/DESIGN.md | 13 ++++-- internal/httpclientx/getjson.go | 10 ++-- internal/httpclientx/getjson_test.go | 45 ++++++------------ internal/httpclientx/getraw.go | 4 +- internal/httpclientx/getraw_test.go | 34 +++++--------- internal/httpclientx/getxml.go | 10 ++-- internal/httpclientx/getxml_test.go | 45 ++++++------------ internal/httpclientx/httpclientx_test.go | 32 ++++--------- internal/httpclientx/overlapped.go | 10 ++-- internal/httpclientx/overlapped_test.go | 18 +++---- internal/httpclientx/postjson.go | 12 ++--- internal/httpclientx/postjson_test.go | 57 +++++++---------------- internal/httpclientx/singlecall.go | 5 -- internal/oonirun/v2.go | 7 ++- internal/probeservices/bouncer.go | 7 ++- internal/probeservices/checkin.go | 27 +++++------ internal/probeservices/measurementmeta.go | 9 ++-- internal/probeservices/register.go | 34 +++++++------- 21 files changed, 167 insertions(+), 240 deletions(-) delete mode 100644 internal/httpclientx/singlecall.go diff --git a/internal/cmd/apitool/main.go b/internal/cmd/apitool/main.go index 2136d4e8ad..0b24e55f31 100644 --- a/internal/cmd/apitool/main.go +++ b/internal/cmd/apitool/main.go @@ -107,7 +107,7 @@ func mmeta(c probeservices.Client, full bool) *model.OOAPIMeasurementMeta { Input: *input, } ctx := context.Background() - m, err := c.GetMeasurementMeta(ctx, &config) + m, err := c.GetMeasurementMeta(ctx, config) fatalOnError(err, "client.GetMeasurementMeta failed") return m } diff --git a/internal/enginelocate/cloudflare.go b/internal/enginelocate/cloudflare.go index 7d5fc89d7a..5f9257bb62 100644 --- a/internal/enginelocate/cloudflare.go +++ b/internal/enginelocate/cloudflare.go @@ -18,13 +18,12 @@ func cloudflareIPLookup( resolver model.Resolver, ) (string, error) { // get the raw response body - data, err := httpclientx.GetRaw( - ctx, - "https://www.cloudflare.com/cdn-cgi/trace", - httpClient, - logger, - userAgent, - ) + data, err := httpclientx.GetRaw(ctx, "https://www.cloudflare.com/cdn-cgi/trace", &httpclientx.Config{ + Authorization: "", // not needed + Client: httpClient, + Logger: logger, + UserAgent: userAgent, + }) // handle the error case if err != nil { diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index 9df193dae3..aabdc59704 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -22,13 +22,12 @@ func ubuntuIPLookup( resolver model.Resolver, ) (string, error) { // read the HTTP response and parse as XML - resp, err := httpclientx.GetXML[*ubuntuResponse]( - ctx, - "https://geoip.ubuntu.com/lookup", - httpClient, - logger, - userAgent, - ) + resp, err := httpclientx.GetXML[*ubuntuResponse](ctx, "https://geoip.ubuntu.com/lookup", &httpclientx.Config{ + Authorization: "", // not needed + Client: httpClient, + Logger: logger, + UserAgent: userAgent, + }) // handle the error case if err != nil { diff --git a/internal/httpclientx/DESIGN.md b/internal/httpclientx/DESIGN.md index 97d7a0bee1..36f21eb62c 100644 --- a/internal/httpclientx/DESIGN.md +++ b/internal/httpclientx/DESIGN.md @@ -77,15 +77,18 @@ type Config struct { UserAgent string } -func GetJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) +func GetJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) -func GetRaw(ctx context.Context, config *Config, URL string) ([]byte, error) +func GetRaw(ctx context.Context, URL string, config *Config) ([]byte, error) -func GetXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) +func GetXML[Output any](ctx context.Context, URL string, config *Config) (Output, error) -func PostJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) +func PostJSON[Input, Output any](ctx context.Context, URL string, input Input, config *Config) (Output, error) ``` +(The `*Config` is the last argument because it is handy to create it inline when calling +and having it last reduces readability the least.) + These operations implement all the actions listed in the first requirement. The `Config` struct allows to add new optional fields to implement new functionality without @@ -130,7 +133,7 @@ func NewOverlappedGetRaw(config *Config) *Overlapped[[]byte] func NewOverlappedGetXML[Output any](config *Config) *Overlapped[Output] -func NewOverlappedPostJSON[Input, Output any](config *Config, input Input) *Overlapped[Output] +func NewOverlappedPostJSON[Input, Output any](input Input, config *Config) *Overlapped[Output] ``` They all construct the same `*Overlapped` struct, which looks like this: diff --git a/internal/httpclientx/getjson.go b/internal/httpclientx/getjson.go index 971edaf23d..d82bb1b32e 100644 --- a/internal/httpclientx/getjson.go +++ b/internal/httpclientx/getjson.go @@ -15,18 +15,18 @@ import ( // // - ctx is the cancellable context; // -// - config contains the config; +// - URL is the URL to use; // -// - URL is the URL to use. +// - config contains the config. // // This function either returns an error or a valid Output. -func GetJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) { +func GetJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) { return NewOverlappedGetJSON[Output](config).Run(ctx, URL) } -func getJSON[Output any](ctx context.Context, config *Config, URL string) (Output, error) { +func getJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) { // read the raw body - rawrespbody, err := GetRaw(ctx, config, URL) + rawrespbody, err := GetRaw(ctx, URL, config) // handle the case of error if err != nil { diff --git a/internal/httpclientx/getjson_test.go b/internal/httpclientx/getjson_test.go index 61371d9289..7b6340bcca 100644 --- a/internal/httpclientx/getjson_test.go +++ b/internal/httpclientx/getjson_test.go @@ -25,15 +25,12 @@ func TestGetJSON(t *testing.T) { server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -56,15 +53,12 @@ func TestGetJSON(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -87,15 +81,12 @@ func TestGetJSON(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -132,16 +123,13 @@ func TestGetJSONHeadersOkay(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // send the request and receive the response + apiresp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ Authorization: "scribai", Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // send the request and receive the response - apiresp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + }) // we do not expect to see an error here if err != nil { @@ -184,15 +172,12 @@ func TestGetJSONLoggingOkay(t *testing.T) { // instantiate a logger that collects logs logger := &testingx.Logger{} - // create API call config - config := &Config{ + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetJSON[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) diff --git a/internal/httpclientx/getraw.go b/internal/httpclientx/getraw.go index 0445f877cf..a1199dbd9d 100644 --- a/internal/httpclientx/getraw.go +++ b/internal/httpclientx/getraw.go @@ -20,11 +20,11 @@ import ( // - URL is the URL to use. // // This function either returns an error or a valid Output. -func GetRaw(ctx context.Context, config *Config, URL string) ([]byte, error) { +func GetRaw(ctx context.Context, URL string, config *Config) ([]byte, error) { return NewOverlappedGetRaw(config).Run(ctx, URL) } -func getRaw(ctx context.Context, config *Config, URL string) ([]byte, error) { +func getRaw(ctx context.Context, URL string, config *Config) ([]byte, error) { // construct the request to use req, err := http.NewRequestWithContext(ctx, "GET", URL, nil) if err != nil { diff --git a/internal/httpclientx/getraw_test.go b/internal/httpclientx/getraw_test.go index 2b4be2ba2b..28c03d410a 100644 --- a/internal/httpclientx/getraw_test.go +++ b/internal/httpclientx/getraw_test.go @@ -15,16 +15,15 @@ import ( func TestGetRaw(t *testing.T) { t.Run("when we cannot create a request", func(t *testing.T) { // create API call config - config := &Config{ - Client: http.DefaultClient, - Logger: model.DiscardLogger, - UserAgent: model.HTTPHeaderUserAgent, - } rawrespbody, err := GetRaw( context.Background(), - config, "\t", // <- invalid URL that we cannot parse + &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }, ) t.Log(rawrespbody) @@ -48,14 +47,11 @@ func TestGetRaw(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + rawrespbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - rawrespbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(rawrespbody) t.Log(err) @@ -89,16 +85,13 @@ func TestGetRawHeadersOkay(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // send the request and receive the response + rawresp, err := GetRaw(context.Background(), server.URL, &Config{ Authorization: "scribai", Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // send the request and receive the response - rawresp, err := GetRaw(context.Background(), config, server.URL) + }) // we do not expect to see an error here if err != nil { @@ -143,14 +136,11 @@ func TestGetRawLoggingOkay(t *testing.T) { // instantiate a logger that collects logs logger := &testingx.Logger{} - // create API call config - config := &Config{ + rawrespbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, - } - - rawrespbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(rawrespbody) t.Log(err) diff --git a/internal/httpclientx/getxml.go b/internal/httpclientx/getxml.go index 7681bd172a..86701e5059 100644 --- a/internal/httpclientx/getxml.go +++ b/internal/httpclientx/getxml.go @@ -15,18 +15,18 @@ import ( // // - ctx is the cancellable context; // -// - config is the config to use; +// - URL is the URL to use; // -// - URL is the URL to use. +// - config is the config to use. // // This function either returns an error or a valid Output. -func GetXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) { +func GetXML[Output any](ctx context.Context, URL string, config *Config) (Output, error) { return NewOverlappedGetXML[Output](config).Run(ctx, URL) } -func getXML[Output any](ctx context.Context, config *Config, URL string) (Output, error) { +func getXML[Output any](ctx context.Context, URL string, config *Config) (Output, error) { // read the raw body - rawrespbody, err := GetRaw(ctx, config, URL) + rawrespbody, err := GetRaw(ctx, URL, config) // handle the case of error if err != nil { diff --git a/internal/httpclientx/getxml_test.go b/internal/httpclientx/getxml_test.go index 96e9262a0a..e7a85337dc 100644 --- a/internal/httpclientx/getxml_test.go +++ b/internal/httpclientx/getxml_test.go @@ -21,15 +21,12 @@ func TestGetXML(t *testing.T) { server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -52,15 +49,12 @@ func TestGetXML(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -83,15 +77,12 @@ func TestGetXML(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) @@ -128,16 +119,13 @@ func TestGetXMLHeadersOkay(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // send the request and receive the response + apiresp, err := GetXML[*apiResponse](context.Background(), server.URL, &Config{ Authorization: "scribai", Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // send the request and receive the response - apiresp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + }) // we do not expect to see an error here if err != nil { @@ -180,15 +168,12 @@ func TestGetXMLLoggingOkay(t *testing.T) { // instantiate a logger that collects logs logger := &testingx.Logger{} - // create API call config - config := &Config{ + // invoke the API + resp, err := GetXML[*apiResponse](context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, - } - - // invoke the API - resp, err := GetXML[*apiResponse](context.Background(), config, server.URL) + }) t.Log(resp) t.Log(err) diff --git a/internal/httpclientx/httpclientx_test.go b/internal/httpclientx/httpclientx_test.go index a58477c751..26a7fbd467 100644 --- a/internal/httpclientx/httpclientx_test.go +++ b/internal/httpclientx/httpclientx_test.go @@ -30,15 +30,12 @@ func TestGzipDecompression(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // make sure we can read it + respbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // make sure we can read it - respbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(respbody) t.Log(err) @@ -62,15 +59,12 @@ func TestGzipDecompression(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + // attempt to get a response body + respbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // attempt to get a response body - respbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(respbody) t.Log(err) @@ -89,14 +83,11 @@ func TestHTTPStatusCodeHandling(t *testing.T) { server := testingx.MustNewHTTPServer(testingx.HTTPHandlerBlockpage451()) defer server.Close() - // create API call config - config := &Config{ + respbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - respbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(respbody) t.Log(err) @@ -122,14 +113,11 @@ func TestHTTPReadBodyErrorsHandling(t *testing.T) { server := testingx.MustNewHTTPServer(testingx.HTTPHandlerResetWhileReadingBody()) defer server.Close() - // create API call config - config := &Config{ + respbody, err := GetRaw(context.Background(), server.URL, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - respbody, err := GetRaw(context.Background(), config, server.URL) + }) t.Log(respbody) t.Log(err) diff --git a/internal/httpclientx/overlapped.go b/internal/httpclientx/overlapped.go index a37fee3bfc..70a30a6aac 100644 --- a/internal/httpclientx/overlapped.go +++ b/internal/httpclientx/overlapped.go @@ -53,28 +53,28 @@ func newOverlappedWithFunc[Output any](fx func(context.Context, string) (Output, // NewOverlappedGetJSON constructs a [*Overlapped] for calling [GetJSON] with multiple URLs. func NewOverlappedGetJSON[Output any](config *Config) *Overlapped[Output] { return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { - return getJSON[Output](ctx, config, URL) + return getJSON[Output](ctx, URL, config) }) } // NewOverlappedGetRaw constructs a [*Overlapped] for calling [GetRaw] with multiple URLs. func NewOverlappedGetRaw(config *Config) *Overlapped[[]byte] { return newOverlappedWithFunc(func(ctx context.Context, URL string) ([]byte, error) { - return getRaw(ctx, config, URL) + return getRaw(ctx, URL, config) }) } // NewOverlappedGetXML constructs a [*Overlapped] for calling [GetXML] with multiple URLs. func NewOverlappedGetXML[Output any](config *Config) *Overlapped[Output] { return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { - return getXML[Output](ctx, config, URL) + return getXML[Output](ctx, URL, config) }) } // NewOverlappedPostJSON constructs a [*Overlapped] for calling [PostJSON] with multiple URLs. -func NewOverlappedPostJSON[Input, Output any](config *Config, input Input) *Overlapped[Output] { +func NewOverlappedPostJSON[Input, Output any](input Input, config *Config) *Overlapped[Output] { return newOverlappedWithFunc(func(ctx context.Context, URL string) (Output, error) { - return postJSON[Input, Output](ctx, config, URL, input) + return postJSON[Input, Output](ctx, URL, input, config) }) } diff --git a/internal/httpclientx/overlapped_test.go b/internal/httpclientx/overlapped_test.go index 0fd1afb64a..350823e9f5 100644 --- a/internal/httpclientx/overlapped_test.go +++ b/internal/httpclientx/overlapped_test.go @@ -53,14 +53,12 @@ func TestNewOverlappedPostJSONIsPerformingOverlappedCalls(t *testing.T) { UserID: 117, } - config := &Config{ + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](apiReq, &Config{ Authorization: "", // not relevant for this test Client: http.DefaultClient, Logger: log.Log, UserAgent: model.HTTPHeaderUserAgent, - } - - overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + }) // make sure we set a low scheduling interval to make test faster overlapped.ScheduleInterval = time.Second @@ -133,14 +131,12 @@ func TestNewOverlappedPostJSONCancelsPendingCalls(t *testing.T) { UserID: 117, } - config := &Config{ + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](apiReq, &Config{ Authorization: "", // not relevant for this test Client: http.DefaultClient, Logger: log.Log, UserAgent: model.HTTPHeaderUserAgent, - } - - overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + }) // make sure the schedule interval is high because we want // all the goroutines but the first to be waiting for permission @@ -189,14 +185,12 @@ func TestNewOverlappedPostJSONWithNoURLs(t *testing.T) { UserID: 117, } - config := &Config{ + overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](apiReq, &Config{ Authorization: "", // not relevant for this test Client: http.DefaultClient, Logger: log.Log, UserAgent: model.HTTPHeaderUserAgent, - } - - overlapped := NewOverlappedPostJSON[*apiRequest, *apiResponse](config, apiReq) + }) // Now we issue the requests without any URLs and make sure // the result we get is the generic overlapped error diff --git a/internal/httpclientx/postjson.go b/internal/httpclientx/postjson.go index 2073a920a0..b2e5c8e8dd 100644 --- a/internal/httpclientx/postjson.go +++ b/internal/httpclientx/postjson.go @@ -17,18 +17,18 @@ import ( // // - ctx is the cancellable context; // -// - config is the config to use; -// // - URL is the URL to use; // -// - input is the input structure to JSON serialize as the request body. +// - input is the input structure to JSON serialize as the request body; +// +// - config is the config to use. // // This function either returns an error or a valid Output. -func PostJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) { - return NewOverlappedPostJSON[Input, Output](config, input).Run(ctx, URL) +func PostJSON[Input, Output any](ctx context.Context, URL string, input Input, config *Config) (Output, error) { + return NewOverlappedPostJSON[Input, Output](input, config).Run(ctx, URL) } -func postJSON[Input, Output any](ctx context.Context, config *Config, URL string, input Input) (Output, error) { +func postJSON[Input, Output any](ctx context.Context, URL string, input Input, config *Config) (Output, error) { // serialize the request body rawreqbody, err := json.Marshal(input) if err != nil { diff --git a/internal/httpclientx/postjson_test.go b/internal/httpclientx/postjson_test.go index 468ba35862..1bad8a7c87 100644 --- a/internal/httpclientx/postjson_test.go +++ b/internal/httpclientx/postjson_test.go @@ -26,14 +26,11 @@ func TestPostJSON(t *testing.T) { req := make(chan int) close(req) - // create API call config - config := &Config{ + resp, err := PostJSON[chan int, *apiResponse](context.Background(), "", req, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - resp, err := PostJSON[chan int, *apiResponse](context.Background(), config, "", req) + }) t.Log(resp) t.Log(err) @@ -50,18 +47,15 @@ func TestPostJSON(t *testing.T) { t.Run("when we cannot create a request", func(t *testing.T) { req := &apiRequest{117} - // create API call config - config := &Config{ - Client: http.DefaultClient, - Logger: model.DiscardLogger, - UserAgent: model.HTTPHeaderUserAgent, - } - resp, err := PostJSON[*apiRequest, *apiResponse]( context.Background(), - config, "\t", // <- invalid URL that we cannot parse req, + &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }, ) t.Log(resp) @@ -82,14 +76,11 @@ func TestPostJSON(t *testing.T) { req := &apiRequest{117} - // create API call config - config := &Config{ + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, req, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + }) t.Log(resp) t.Log(err) @@ -111,14 +102,11 @@ func TestPostJSON(t *testing.T) { req := &apiRequest{117} - // create API call config - config := &Config{ + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, req, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + }) t.Log(resp) t.Log(err) @@ -151,14 +139,11 @@ func TestPostJSON(t *testing.T) { })) defer server.Close() - // create API call config - config := &Config{ + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, req, &Config{ Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + }) t.Log(resp) t.Log(err) @@ -205,16 +190,13 @@ func TestPostJSONCommunicationOkay(t *testing.T) { } rawapireq := must.MarshalJSON(apireq) - // create API call config - config := &Config{ + // send the request and receive the response + apiresp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, apireq, &Config{ Authorization: "scribai", Client: http.DefaultClient, Logger: model.DiscardLogger, UserAgent: model.HTTPHeaderUserAgent, - } - - // send the request and receive the response - apiresp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, apireq) + }) // we do not expect to see an error here if err != nil { @@ -277,14 +259,11 @@ func TestPostJSONLoggingOkay(t *testing.T) { // instantiate a logger that collects logs logger := &testingx.Logger{} - // create API call config - config := &Config{ + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, req, &Config{ Client: http.DefaultClient, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, - } - - resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), config, server.URL, req) + }) t.Log(resp) t.Log(err) diff --git a/internal/httpclientx/singlecall.go b/internal/httpclientx/singlecall.go deleted file mode 100644 index 00d5d01284..0000000000 --- a/internal/httpclientx/singlecall.go +++ /dev/null @@ -1,5 +0,0 @@ -package httpclientx - -// SingleCall represents a single API call. -type SingleCall struct { -} diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 66993d88f8..98c8fe7644 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -67,7 +67,12 @@ var ErrHTTPRequestFailed = errors.New("oonirun: HTTP request failed") // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, logger model.Logger, URL string) (*V2Descriptor, error) { - return httpclientx.GetJSON[*V2Descriptor](ctx, URL, client, logger, model.HTTPHeaderUserAgent) + return httpclientx.GetJSON[*V2Descriptor](ctx, URL, &httpclientx.Config{ + Authorization: "", + Client: client, + Logger: logger, + UserAgent: model.HTTPHeaderUserAgent, + }) } // v2DescriptorCache contains all the known v2Descriptor entries. diff --git a/internal/probeservices/bouncer.go b/internal/probeservices/bouncer.go index 2e0cf78247..9989092c0f 100644 --- a/internal/probeservices/bouncer.go +++ b/internal/probeservices/bouncer.go @@ -22,6 +22,9 @@ func (c *Client) GetTestHelpers(ctx context.Context) (map[string][]model.OOAPISe URL.Path = "/api/v1/test-helpers" // get the response - return httpclientx.GetJSON[map[string][]model.OOAPIService]( - ctx, URL.String(), c.HTTPClient, c.Logger, c.UserAgent) + return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL.String(), &httpclientx.Config{ + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }) } diff --git a/internal/probeservices/checkin.go b/internal/probeservices/checkin.go index 775311fa70..08b12583e7 100644 --- a/internal/probeservices/checkin.go +++ b/internal/probeservices/checkin.go @@ -1,15 +1,12 @@ package probeservices -// -// checkin.go - POST /api/v1/check-in -// - import ( "context" - "net/url" - "github.com/ooni/probe-cli/v3/internal/httpclientx" + "github.com/ooni/probe-cli/v3/internal/checkincache" + "github.com/ooni/probe-cli/v3/internal/httpapi" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/ooapi" ) // CheckIn function is called by probes asking if there are tests to be run @@ -18,15 +15,19 @@ import ( // track selected parts of the check-in API response. // Returns the list of tests to run and the URLs, on success, // or an explanatory error, in case of failure. -func (c *Client) CheckIn(ctx context.Context, input model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { - // construct the URL to use - URL, err := url.Parse(c.BaseURL) +func (c Client) CheckIn( + ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { + // prepare endpoint and descriptor for the API call + epnt := c.newHTTPAPIEndpoint() + desc := ooapi.NewDescriptorCheckIn(&config) + + // issue the API call and handle failures + resp, err := httpapi.Call(ctx, desc, epnt) if err != nil { return nil, err } - URL.Path = "/api/v1/check-in" - // get the response - return httpclientx.PostJSON[*model.OOAPICheckInConfig, *model.OOAPICheckInResult]( - ctx, URL.String(), c.HTTPClient, &input, c.Logger, c.UserAgent) + // make sure we track selected parts of the response + _ = checkincache.Store(c.KVStore, resp) + return resp, nil } diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go index fdb59eecf3..3882718452 100644 --- a/internal/probeservices/measurementmeta.go +++ b/internal/probeservices/measurementmeta.go @@ -14,7 +14,7 @@ import ( // GetMeasurementMeta returns meta information about a measurement. func (c Client) GetMeasurementMeta( - ctx context.Context, input *model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { + ctx context.Context, input model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { // construct the query to use query := url.Values{} query.Add("report_id", input.ReportID) @@ -34,6 +34,9 @@ func (c Client) GetMeasurementMeta( URL.RawQuery = query.Encode() // get the response - return httpclientx.GetJSON[*model.OOAPIMeasurementMeta]( - ctx, URL.String(), c.HTTPClient, c.Logger, c.UserAgent) + return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL.String(), &httpclientx.Config{ + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }) } diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go index d28fee726d..19c309ecef 100644 --- a/internal/probeservices/register.go +++ b/internal/probeservices/register.go @@ -13,21 +13,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/randx" ) -// Register invokes the /api/v1/register API. -func (c *Client) Register( - ctx context.Context, input *model.OOAPIRegisterRequest) (*model.OOAPIRegisterResponse, error) { - // construct the URL to use - URL, err := url.Parse(c.BaseURL) - if err != nil { - return nil, err - } - URL.Path = "/api/v1/register" - - // get the response - return httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( - ctx, URL.String(), c.HTTPClient, input, c.Logger, c.UserAgent) -} - // MaybeRegister registers this client if not already registered func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMetadata) error { if !metadata.Valid() { @@ -45,11 +30,24 @@ func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMeta OOAPIProbeMetadata: metadata, Password: pwd, } - var resp model.OOAPIRegisterResponse - if err := c.APIClientTemplate.Build().PostJSON( - ctx, "/api/v1/register", req, &resp); err != nil { + + URL, err := url.Parse(c.BaseURL) + if err != nil { + return err + } + URL.Path = "/api/v1/register" + + resp, err := httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( + ctx, URL.String(), req, &httpclientx.Config{ + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }, + ) + if err != nil { return err } + state.ClientID = resp.ClientID state.Password = pwd return c.StateFile.Set(state) From a6046fd976274accfdef44014017dd91b075adb4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 14:50:56 +0200 Subject: [PATCH 05/22] x --- internal/probeservices/measurementmeta.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go index 3882718452..ec8de68194 100644 --- a/internal/probeservices/measurementmeta.go +++ b/internal/probeservices/measurementmeta.go @@ -14,14 +14,14 @@ import ( // GetMeasurementMeta returns meta information about a measurement. func (c Client) GetMeasurementMeta( - ctx context.Context, input model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { + ctx context.Context, config model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { // construct the query to use query := url.Values{} - query.Add("report_id", input.ReportID) - if input.Input != "" { - query.Add("input", input.Input) + query.Add("report_id", config.ReportID) + if config.Input != "" { + query.Add("input", config.Input) } - if input.Full { + if config.Full { query.Add("full", "true") } From 341fcf2a31eb20956eafc910e2dacc36ef06ca59 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 15:21:35 +0200 Subject: [PATCH 06/22] x --- internal/probeservices/tor.go | 7 ++ internal/urlx/DESIGN.md | 32 +++++++++ internal/urlx/urlx.go | 32 +++++++++ internal/urlx/urlx_test.go | 127 ++++++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+) create mode 100644 internal/urlx/DESIGN.md create mode 100644 internal/urlx/urlx.go create mode 100644 internal/urlx/urlx_test.go diff --git a/internal/probeservices/tor.go b/internal/probeservices/tor.go index 8c5cef7395..b6ee3e6973 100644 --- a/internal/probeservices/tor.go +++ b/internal/probeservices/tor.go @@ -10,14 +10,21 @@ import ( // FetchTorTargets returns the targets for the tor experiment. func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[string]model.OOAPITorTarget, err error) { + // get credentials and authentication token _, auth, err := c.GetCredsAndAuth() if err != nil { return nil, err } + + // format Authorization header value s := fmt.Sprintf("Bearer %s", auth.Token) + client := c.APIClientTemplate.BuildWithAuthorization(s) + + // create query string query := url.Values{} query.Add("country_code", cc) + err = client.GetJSONWithQuery( ctx, "/api/v1/test-list/tor-targets", query, &result) return diff --git a/internal/urlx/DESIGN.md b/internal/urlx/DESIGN.md new file mode 100644 index 0000000000..6823aa962b --- /dev/null +++ b/internal/urlx/DESIGN.md @@ -0,0 +1,32 @@ +# ./internal/urlx + +This package contains algorithms to operate on URLs. + +## ResolveReference + +This function has the following signature: + +```Go +func ResolveReference(baseURL, path, rawQuery string) (string, error) +``` + +It solves the problem of computing a composed URL starting from a base URL, an +extra path and a possibly empty raw query. The algorithm will ignore the path and +the query of the base URL and only use the scheme and the host. + +For example, assuming the following: + +```Go +baseURL := "https://api.ooni.io/antani?foo=bar" +path := "/api/v1/check-in" +query := "bar=baz" +``` + +This function will return this URL: + +```Go +"https://api.ooni.io/ap1/v1/check-in?bar=baz" +``` + +We need this functionality when implementing communication with the probe services, +where we have a base URL and specific path and optional query for each API. diff --git a/internal/urlx/urlx.go b/internal/urlx/urlx.go new file mode 100644 index 0000000000..d41c061e96 --- /dev/null +++ b/internal/urlx/urlx.go @@ -0,0 +1,32 @@ +// Package urlx contains URL extensions. +package urlx + +import ( + "net/url" +) + +// ResolveReference constructs a new URL consisting of the given base URL with +// the path appended to the given path and the optional query. +// +// For example, given: +// +// URL := "https://api.ooni.io/api/v1" +// path := "/measurement_meta" +// rawQuery := "full=true" +// +// This function will return: +// +// result := "https://api.ooni.io/api/v1/measurement_meta?full=true" +// +// This function fails when we cannot parse URL as a [*net.URL]. +func ResolveReference(baseURL, path, rawQuery string) (string, error) { + parsedBase, err := url.Parse(baseURL) + if err != nil { + return "", err + } + ref := &url.URL{ + Path: path, + RawQuery: rawQuery, + } + return parsedBase.ResolveReference(ref).String(), nil +} diff --git a/internal/urlx/urlx_test.go b/internal/urlx/urlx_test.go new file mode 100644 index 0000000000..5db0c895b6 --- /dev/null +++ b/internal/urlx/urlx_test.go @@ -0,0 +1,127 @@ +package urlx + +import ( + "errors" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestResolveReference(t *testing.T) { + // testcase is a test case used by this function + type testcase struct { + // Name is the test case name. + Name string + + // BaseURL is the base URL. + BaseURL string + + // Path is the extra path to append. + Path string + + // RawQuery is the raw query. + RawQuery string + + // ExpectErr is the expected err. + ExpectErr error + + // ExpectURL is what we expect to see. + ExpectURL string + } + + cases := []testcase{{ + Name: "when we cannot parse the base URL", + BaseURL: "\t", // invalid + Path: "", + RawQuery: "", + ExpectErr: errors.New(`parse "\t": net/url: invalid control character in URL`), + ExpectURL: "", + }, { + Name: "when there's only the base URL", + BaseURL: "https://api.ooni.io/", + Path: "", + RawQuery: "", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/", + }, { + Name: "when there's only the path", + BaseURL: "", + Path: "/api/v1/check-in", + RawQuery: "", + ExpectErr: nil, + ExpectURL: "/api/v1/check-in", + }, { + Name: "when there's only the qiery", + BaseURL: "", + Path: "", + RawQuery: "key1=value1&key1=value2&key3=value3", + ExpectErr: nil, + ExpectURL: "?key1=value1&key1=value2&key3=value3", + }, { + Name: "with base URL and path", + BaseURL: "https://api.ooni.io/", + Path: "/api/v1/check-in", + RawQuery: "", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/api/v1/check-in", + }, { + Name: "with base URL and query", + BaseURL: "https://api.ooni.io/", + Path: "", + RawQuery: "key1=value1&key1=value2&key3=value3", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/?key1=value1&key1=value2&key3=value3", + }, { + Name: "with base URL, path, and query", + BaseURL: "https://api.ooni.io/", + Path: "/api/v1/check-in", + RawQuery: "key1=value1&key1=value2&key3=value3", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/api/v1/check-in?key1=value1&key1=value2&key3=value3", + }, { + Name: "with base URL with path, path, and query", + BaseURL: "https://api.ooni.io/api", + Path: "/v1/check-in", + RawQuery: "key1=value1&key1=value2&key3=value3", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/v1/check-in?key1=value1&key1=value2&key3=value3", + }, { + Name: "with base URL with path and query, path, and query", + BaseURL: "https://api.ooni.io/api?foo=bar", + Path: "/v1/check-in", + RawQuery: "key1=value1&key1=value2&key3=value3", + ExpectErr: nil, + ExpectURL: "https://api.ooni.io/v1/check-in?key1=value1&key1=value2&key3=value3", + }} + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + // invoke the API we're currently testing + got, err := ResolveReference(tc.BaseURL, tc.Path, tc.RawQuery) + + // check for errors + switch { + case err == nil && tc.ExpectErr == nil: + if diff := cmp.Diff(tc.ExpectURL, got); diff != "" { + t.Fatal(diff) + } + return + + case err != nil && tc.ExpectErr == nil: + t.Fatal("expected err", tc.ExpectErr, "got", err) + return + + case err == nil && tc.ExpectErr != nil: + t.Fatal("expected err", tc.ExpectErr, "got", err) + return + + case err != nil && tc.ExpectErr != nil: + if err.Error() != tc.ExpectErr.Error() { + t.Fatal("expected err", tc.ExpectErr, "got", err) + } + return + } + + }) + } +} From 8c345249fb03fc35ebbfe95cbc4b4d0360a44865 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 15:22:35 +0200 Subject: [PATCH 07/22] x --- internal/httpclientx/DESIGN.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/httpclientx/DESIGN.md b/internal/httpclientx/DESIGN.md index 36f21eb62c..bd2ebeecd9 100644 --- a/internal/httpclientx/DESIGN.md +++ b/internal/httpclientx/DESIGN.md @@ -220,7 +220,8 @@ Regarding all the other cases for which `GetJSON` is marked as "NO": 1. Enforcing a call timeout is better done just through the context like `httpx` does. 2. `GetJSON` lets the caller completely manage the construction of the URL, so we do not need -code to join together a base URL, possibly including a base path, a path, and a query. +code to join together a base URL, possibly including a base path, a path, and a query (and we're +introducing the new `./internal/urlx` package to handle this situation). 3. `GetJSON` does not handle cloud fronting because we don't use it. The design where the `Config` contains mandatory and optional fields would still allow doing that easily. From 4b464ff60f080eb45b8ce34939f02c4147fa80c0 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 15:44:40 +0200 Subject: [PATCH 08/22] try to entirely remove httpx usages --- internal/probeservices/bouncer.go | 7 ++-- internal/probeservices/collector.go | 45 +++++++++++++++++++---- internal/probeservices/login.go | 21 +++++++++-- internal/probeservices/measurementmeta.go | 7 ++-- internal/probeservices/psiphon.go | 22 ++++++++++- internal/probeservices/register.go | 8 ++-- internal/probeservices/tor.go | 22 ++++++++--- 7 files changed, 101 insertions(+), 31 deletions(-) diff --git a/internal/probeservices/bouncer.go b/internal/probeservices/bouncer.go index 9989092c0f..4320f83efe 100644 --- a/internal/probeservices/bouncer.go +++ b/internal/probeservices/bouncer.go @@ -6,23 +6,22 @@ package probeservices import ( "context" - "net/url" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // GetTestHelpers queries the /api/v1/test-helpers API. func (c *Client) GetTestHelpers(ctx context.Context) (map[string][]model.OOAPIService, error) { // construct the URL to use - URL, err := url.Parse(c.BaseURL) + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-helpers", "") if err != nil { return nil, err } - URL.Path = "/api/v1/test-helpers" // get the response - return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL.String(), &httpclientx.Config{ + return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, diff --git a/internal/probeservices/collector.go b/internal/probeservices/collector.go index df25570fec..a566eaef3b 100644 --- a/internal/probeservices/collector.go +++ b/internal/probeservices/collector.go @@ -8,7 +8,9 @@ import ( "reflect" "sync" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/urlx" ) var ( @@ -58,10 +60,24 @@ func (c Client) OpenReport(ctx context.Context, rt model.OOAPIReportTemplate) (R if rt.Format != model.OOAPIReportDefaultFormat { return nil, ErrUnsupportedFormat } - var cor model.OOAPICollectorOpenResponse - if err := c.APIClientTemplate.WithBodyLogging().Build().PostJSON(ctx, "/report", rt, &cor); err != nil { + + URL, err := urlx.ResolveReference(c.BaseURL, "/report", "") + if err != nil { return nil, err } + + cor, err := httpclientx.PostJSON[model.OOAPIReportTemplate, model.OOAPICollectorOpenResponse]( + ctx, URL, rt, &httpclientx.Config{ + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }, + ) + + if err != nil { + return nil, err + } + for _, format := range cor.SupportedFormats { if format == "json" { return &reportChan{ID: cor.ReportID, client: c, tmpl: rt}, nil @@ -83,14 +99,27 @@ func (r reportChan) CanSubmit(m *model.Measurement) bool { // submitted. Otherwise, we'll set the report ID to the empty // string, so that you know which measurements weren't submitted. func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) error { - var updateResponse model.OOAPICollectorUpdateResponse m.ReportID = r.ID - err := r.client.APIClientTemplate.WithBodyLogging().Build().PostJSON( - ctx, fmt.Sprintf("/report/%s", r.ID), model.OOAPICollectorUpdateRequest{ - Format: "json", - Content: m, - }, &updateResponse, + + URL, err := urlx.ResolveReference(r.client.BaseURL, fmt.Sprintf("/report/%s", r.ID), "") + if err != nil { + return err + } + + apiReq := model.OOAPICollectorUpdateRequest{ + Format: "json", + Content: m, + } + + updateResponse, err := httpclientx.PostJSON[ + model.OOAPICollectorUpdateRequest, model.OOAPICollectorUpdateResponse]( + ctx, URL, apiReq, &httpclientx.Config{ + Client: r.client.HTTPClient, + Logger: r.client.Logger, + UserAgent: r.client.UserAgent, + }, ) + if err != nil { m.ReportID = "" return err diff --git a/internal/probeservices/login.go b/internal/probeservices/login.go index d6bd4c1c93..460b602589 100644 --- a/internal/probeservices/login.go +++ b/internal/probeservices/login.go @@ -3,7 +3,9 @@ package probeservices import ( "context" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // MaybeLogin performs login if necessary @@ -17,11 +19,24 @@ func (c Client) MaybeLogin(ctx context.Context) error { return ErrNotRegistered } c.LoginCalls.Add(1) - var auth model.OOAPILoginAuth - if err := c.APIClientTemplate.Build().PostJSON( - ctx, "/api/v1/login", *creds, &auth); err != nil { + + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/login", "") + if err != nil { + return err + } + + auth, err := httpclientx.PostJSON[*model.OOAPILoginCredentials, model.OOAPILoginAuth]( + ctx, URL, creds, &httpclientx.Config{ + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }, + ) + + if err != nil { return err } + state.Expire = auth.Expire state.Token = auth.Token return c.StateFile.Set(state) diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go index ec8de68194..a2646657e1 100644 --- a/internal/probeservices/measurementmeta.go +++ b/internal/probeservices/measurementmeta.go @@ -10,6 +10,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // GetMeasurementMeta returns meta information about a measurement. @@ -26,15 +27,13 @@ func (c Client) GetMeasurementMeta( } // construct the URL to use - URL, err := url.Parse(c.BaseURL) + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/measurement_meta", query.Encode()) if err != nil { return nil, err } - URL.Path = "/api/v1/measurement_meta" - URL.RawQuery = query.Encode() // get the response - return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL.String(), &httpclientx.Config{ + return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, diff --git a/internal/probeservices/psiphon.go b/internal/probeservices/psiphon.go index a01421d051..41ec925f14 100644 --- a/internal/probeservices/psiphon.go +++ b/internal/probeservices/psiphon.go @@ -3,15 +3,33 @@ package probeservices import ( "context" "fmt" + + "github.com/ooni/probe-cli/v3/internal/httpclientx" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // FetchPsiphonConfig fetches psiphon config from authenticated OONI orchestra. func (c Client) FetchPsiphonConfig(ctx context.Context) ([]byte, error) { + // get credentials and authentication token _, auth, err := c.GetCredsAndAuth() if err != nil { return nil, err } + + // format Authorization header value s := fmt.Sprintf("Bearer %s", auth.Token) - client := c.APIClientTemplate.BuildWithAuthorization(s) - return client.FetchResource(ctx, "/api/v1/test-list/psiphon-config") + + // construct the URL to use + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/psiphon-config", "") + if err != nil { + return nil, err + } + + // get response + return httpclientx.GetRaw(ctx, URL, &httpclientx.Config{ + Authorization: s, + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }) } diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go index 19c309ecef..43dbf96a47 100644 --- a/internal/probeservices/register.go +++ b/internal/probeservices/register.go @@ -6,11 +6,11 @@ package probeservices import ( "context" - "net/url" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/randx" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // MaybeRegister registers this client if not already registered @@ -31,14 +31,14 @@ func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMeta Password: pwd, } - URL, err := url.Parse(c.BaseURL) + // construct the URL to use + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/register", "") if err != nil { return err } - URL.Path = "/api/v1/register" resp, err := httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( - ctx, URL.String(), req, &httpclientx.Config{ + ctx, URL, req, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, diff --git a/internal/probeservices/tor.go b/internal/probeservices/tor.go index b6ee3e6973..5ce50e6962 100644 --- a/internal/probeservices/tor.go +++ b/internal/probeservices/tor.go @@ -5,11 +5,13 @@ import ( "fmt" "net/url" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/urlx" ) // FetchTorTargets returns the targets for the tor experiment. -func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[string]model.OOAPITorTarget, err error) { +func (c Client) FetchTorTargets(ctx context.Context, cc string) (map[string]model.OOAPITorTarget, error) { // get credentials and authentication token _, auth, err := c.GetCredsAndAuth() if err != nil { @@ -19,13 +21,21 @@ func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[stri // format Authorization header value s := fmt.Sprintf("Bearer %s", auth.Token) - client := c.APIClientTemplate.BuildWithAuthorization(s) - // create query string query := url.Values{} query.Add("country_code", cc) - err = client.GetJSONWithQuery( - ctx, "/api/v1/test-list/tor-targets", query, &result) - return + // construct the URL to use + URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/tor-targets", query.Encode()) + if err != nil { + return nil, err + } + + // get response + return httpclientx.GetJSON[map[string]model.OOAPITorTarget](ctx, URL, &httpclientx.Config{ + Authorization: s, + Client: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }) } From 6d571840b100fb441c269f21b8e07df904278eaa Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 18:10:52 +0200 Subject: [PATCH 09/22] fix: make sure there is nil safety Spotted thanks to a very paranoid check inside ./internal/oonirun. Was not a problem before for `httpx` because of its usage pattern and may or may not be a problem for the `httpapi` package (did not check since this work is focused on replacing both `httpx` and `httpapi`). --- internal/httpclientx/DESIGN.md | 38 ++++++ internal/httpclientx/getjson.go | 3 +- internal/httpclientx/getjson_test.go | 85 ++++++++++++ internal/httpclientx/getxml.go | 7 +- internal/httpclientx/httpclientx.go | 3 +- internal/httpclientx/nilsafety.go | 31 +++++ internal/httpclientx/nilsafety_test.go | 122 +++++++++++++++++ internal/httpclientx/postjson.go | 8 +- internal/httpclientx/postjson_test.go | 175 +++++++++++++++++++++++++ 9 files changed, 468 insertions(+), 4 deletions(-) create mode 100644 internal/httpclientx/nilsafety.go create mode 100644 internal/httpclientx/nilsafety_test.go diff --git a/internal/httpclientx/DESIGN.md b/internal/httpclientx/DESIGN.md index bd2ebeecd9..f01c051fc3 100644 --- a/internal/httpclientx/DESIGN.md +++ b/internal/httpclientx/DESIGN.md @@ -23,6 +23,7 @@ The rest of this document explains the requirements and describes the design. - [GetRaw](#getraw) - [GetXML](#getxml) - [PostJSON](#postjson) +- [Nil Safety](#nil-safety) - [Refactoring Plan](#refactoring-plan) - [Limitations and Future Work](#limitations-and-future-work) @@ -326,6 +327,43 @@ practical significance for all the APIs we invoke and the new behavior is strict The `yes~` means that `httpapi` already receives a marshaled body from a higher-level API that is part of the same package, while in this package we marshal in `PostJSON`. +## Nil Safety + +Consider the following code snippet: + +```Go +resp, err := httpclientx.GetJSON[*APIResponse](ctx, URL, config) +runtimex.Assert((resp == nil && err != nil) || (resp != nil && err == nil), "ouch") +``` + +Now, consider the case where `URL` refers to a server that returns `null` as the JSON +answer, rather than returning a JSON object. The `encoding/json` package will accept the +`null` value and unmarshal it into a `nil` pointer. So, `GetJSON` will return `nil` and +`nil`, and the `runtimex.Assert` will fail. + +The `httpx` package did not have this issue because the usage pattern was: + +```Go +var resp APIResponse +err := apiClient.GetJSON(ctx, "/foobar", &resp) // where apiClient implements httpx.APIClient +``` + +In such a case, the `null` would have no effect and `resp` would be an empty response. + +However, it is still handy to return a value and an error, and it is the most commonly used +pattern in Go and, as a result, in OONI Probe. So, what do we do? + +Well, here's the strategy: + +1. When sending pointers, slices, or maps in `PostJSON`, we return `ErrIsNil` if the pointer, +slice, or map is `nil`, to avoid sending literal `null` to servers. + +2. `GetJSON`, `GetXML`, and `PostJSON` include checks after unmarshaling so that, if the API response +type is a slice, pointer, or map, and it is `nil`, we also return `ErrIsNil`. + +Strictly speaking, it is still unclear to us whether this could happen with `GetXML` but we have +decided to implements these checks for `GetXML` as well, just in case. + ## Refactoring Plan The overall goal is to replace usages of `httpapi` and `httpx` with usages of `httpclient`. diff --git a/internal/httpclientx/getjson.go b/internal/httpclientx/getjson.go index d82bb1b32e..fb0896656d 100644 --- a/internal/httpclientx/getjson.go +++ b/internal/httpclientx/getjson.go @@ -39,5 +39,6 @@ func getJSON[Output any](ctx context.Context, URL string, config *Config) (Outpu return zeroValue[Output](), err } - return output, nil + // avoid returning nil pointers, maps, slices + return NilSafetyErrorIfNil(output) } diff --git a/internal/httpclientx/getjson_test.go b/internal/httpclientx/getjson_test.go index 7b6340bcca..bbc453c75c 100644 --- a/internal/httpclientx/getjson_test.go +++ b/internal/httpclientx/getjson_test.go @@ -203,3 +203,88 @@ func TestGetJSONLoggingOkay(t *testing.T) { t.Fatal("did not see raw response body log line") } } + +// TestGetJSONCorrectlyRejectsNilValues ensures we correctly reject nil values. +func TestGetJSONCorrectlyRejectsNilValues(t *testing.T) { + + t.Run("when unmarshaling into a map", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // invoke the API + resp, err := GetJSON[map[string]string](context.Background(), server.URL, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when unmarshaling into a struct pointer", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // invoke the API + resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when unmarshaling into a slice", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // invoke the API + resp, err := GetJSON[[]string](context.Background(), server.URL, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) +} diff --git a/internal/httpclientx/getxml.go b/internal/httpclientx/getxml.go index 86701e5059..34ddac2ede 100644 --- a/internal/httpclientx/getxml.go +++ b/internal/httpclientx/getxml.go @@ -39,5 +39,10 @@ func getXML[Output any](ctx context.Context, URL string, config *Config) (Output return zeroValue[Output](), err } - return output, nil + // TODO(bassosimone): it's unclear to me whether output can be nil when unmarshaling + // XML input, since there is no "null" in XML. In any case, the code below checks for + // and avoids emitting nil, so I guess we should be fine here. + + // avoid returning nil pointers, maps, slices + return NilSafetyErrorIfNil(output) } diff --git a/internal/httpclientx/httpclientx.go b/internal/httpclientx/httpclientx.go index b738a85ae0..a5e826e7d9 100644 --- a/internal/httpclientx/httpclientx.go +++ b/internal/httpclientx/httpclientx.go @@ -98,5 +98,6 @@ func do(ctx context.Context, req *http.Request, config *Config) ([]byte, error) return nil, &ErrRequestFailed{resp.StatusCode} } - return rawrespbody, nil + // make sure we replace a nil slice with an empty slice + return NilSafetyAvoidNilBytesSlice(rawrespbody), nil } diff --git a/internal/httpclientx/nilsafety.go b/internal/httpclientx/nilsafety.go new file mode 100644 index 0000000000..e66910a106 --- /dev/null +++ b/internal/httpclientx/nilsafety.go @@ -0,0 +1,31 @@ +package httpclientx + +import ( + "errors" + "reflect" +) + +// ErrIsNil indicates that [NilSafetyErrorIfNil] was passed a nil value. +var ErrIsNil = errors.New("nil map, pointer or slice") + +// NilSafetyErrorIfNil returns [ErrIsNil] iff input is a nil map, struct, or slice. +// +// This mechanism prevents us from mistakenly sending to a server a literal JSON "null" and +// protects us from attempting to process a literal JSON "null" from a server. +func NilSafetyErrorIfNil[Type any](value Type) (Type, error) { + switch rv := reflect.ValueOf(value); rv.Kind() { + case reflect.Map, reflect.Pointer, reflect.Slice: + if rv.IsNil() { + return zeroValue[Type](), ErrIsNil + } + } + return value, nil +} + +// NilSafetyAvoidNilBytesSlice replaces a nil bytes slice with an empty slice. +func NilSafetyAvoidNilBytesSlice(input []byte) []byte { + if input == nil { + input = []byte{} + } + return input +} diff --git a/internal/httpclientx/nilsafety_test.go b/internal/httpclientx/nilsafety_test.go new file mode 100644 index 0000000000..85551266e2 --- /dev/null +++ b/internal/httpclientx/nilsafety_test.go @@ -0,0 +1,122 @@ +package httpclientx + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestNilSafetyErrorIfNil(t *testing.T) { + + // testcase is a test case implemented by this function. + type testcase struct { + name string + input any + err error + output any + } + + cases := []testcase{{ + name: "with a nil map", + input: func() any { + var v map[string]string + return v + }(), + err: ErrIsNil, + output: nil, + }, { + name: "with a non-nil but empty map", + input: make(map[string]string), + err: nil, + output: make(map[string]string), + }, { + name: "with a non-nil non-empty map", + input: map[string]string{"a": "b"}, + err: nil, + output: map[string]string{"a": "b"}, + }, { + name: "with a nil pointer", + input: func() any { + var v *apiRequest + return v + }(), + err: ErrIsNil, + output: nil, + }, { + name: "with a non-nil empty pointer", + input: &apiRequest{}, + err: nil, + output: &apiRequest{}, + }, { + name: "with a non-nil non-empty pointer", + input: &apiRequest{UserID: 11}, + err: nil, + output: &apiRequest{UserID: 11}, + }, { + name: "with a nil slice", + input: func() any { + var v []int + return v + }(), + err: ErrIsNil, + output: nil, + }, { + name: "with a non-nil empty slice", + input: []int{}, + err: nil, + output: []int{}, + }, { + name: "with a non-nil non-empty slice", + input: []int{44}, + err: nil, + output: []int{44}, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + output, err := NilSafetyErrorIfNil(tc.input) + + switch { + case err == nil && tc.err == nil: + if diff := cmp.Diff(tc.output, output); diff != "" { + t.Fatal(diff) + } + return + + case err != nil && tc.err == nil: + t.Fatal("expected", tc.err.Error(), "got", err.Error()) + return + + case err == nil && tc.err != nil: + t.Fatal("expected", tc.err.Error(), "got", err.Error()) + return + + case err != nil && tc.err != nil: + if err.Error() != tc.err.Error() { + t.Fatal("expected", tc.err.Error(), "got", err.Error()) + } + return + } + }) + } +} + +func TestNilSafetyAvoidNilByteSlice(t *testing.T) { + t.Run("for nil byte slice", func(t *testing.T) { + output := NilSafetyAvoidNilBytesSlice(nil) + if output == nil { + t.Fatal("expected non-nil") + } + if len(output) != 0 { + t.Fatal("expected zero length") + } + }) + + t.Run("for non-nil byte slice", func(t *testing.T) { + expected := []byte{44} + output := NilSafetyAvoidNilBytesSlice(expected) + if diff := cmp.Diff(expected, output); diff != "" { + t.Fatal("not the same pointer") + } + }) +} diff --git a/internal/httpclientx/postjson.go b/internal/httpclientx/postjson.go index b2e5c8e8dd..5c559b1848 100644 --- a/internal/httpclientx/postjson.go +++ b/internal/httpclientx/postjson.go @@ -29,6 +29,11 @@ func PostJSON[Input, Output any](ctx context.Context, URL string, input Input, c } func postJSON[Input, Output any](ctx context.Context, URL string, input Input, config *Config) (Output, error) { + // ensure we're not sending a nil map, pointer, or slice + if _, err := NilSafetyErrorIfNil(input); err != nil { + return zeroValue[Output](), err + } + // serialize the request body rawreqbody, err := json.Marshal(input) if err != nil { @@ -61,5 +66,6 @@ func postJSON[Input, Output any](ctx context.Context, URL string, input Input, c return zeroValue[Output](), err } - return output, nil + // avoid returning nil pointers, maps, slices + return NilSafetyErrorIfNil(output) } diff --git a/internal/httpclientx/postjson_test.go b/internal/httpclientx/postjson_test.go index 1bad8a7c87..ebde2fd055 100644 --- a/internal/httpclientx/postjson_test.go +++ b/internal/httpclientx/postjson_test.go @@ -291,3 +291,178 @@ func TestPostJSONLoggingOkay(t *testing.T) { t.Fatal("did not see raw response body log line") } } + +// TestPostJSONCorrectlyRejectsNilValues ensures we correctly reject nil values. +func TestPostJSONCorrectlyRejectsNilValues(t *testing.T) { + + t.Run("when sending a nil map", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{}`)) + })) + defer server.Close() + + // invoke the API + resp, err := PostJSON[map[string]string, *apiResponse](context.Background(), server.URL, nil, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when sending a nil struct pointer", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{}`)) + })) + defer server.Close() + + // invoke the API + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, nil, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when sending a nil slice", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{}`)) + })) + defer server.Close() + + // invoke the API + resp, err := PostJSON[[]string, *apiResponse](context.Background(), server.URL, nil, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when unmarshaling into a map", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // create an empty request + apireq := &apiRequest{} + + // invoke the API + resp, err := PostJSON[*apiRequest, map[string]string](context.Background(), server.URL, apireq, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when unmarshaling into a struct pointer", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // create an empty request + apireq := &apiRequest{} + + // invoke the API + resp, err := PostJSON[*apiRequest, *apiResponse](context.Background(), server.URL, apireq, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) + + t.Run("when unmarshaling into a slice", func(t *testing.T) { + server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`null`)) + })) + defer server.Close() + + // create an empty request + apireq := &apiRequest{} + + // invoke the API + resp, err := PostJSON[*apiRequest, []string](context.Background(), server.URL, apireq, &Config{ + Client: http.DefaultClient, + Logger: model.DiscardLogger, + UserAgent: model.HTTPHeaderUserAgent, + }) + + t.Log(resp) + t.Log(err) + + // make sure that the error is the expected one + if !errors.Is(err, ErrIsNil) { + t.Fatal("unexpected error", err) + } + + // make sure resp is nil + if resp != nil { + t.Fatal("expected nil resp") + } + }) +} From 9c2a226a8c59c19763f5d7b2b34d08716e7e7dbb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 18:15:18 +0200 Subject: [PATCH 10/22] oxford comma: yes/no? --- internal/httpclientx/nilsafety.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/httpclientx/nilsafety.go b/internal/httpclientx/nilsafety.go index e66910a106..c73323cb15 100644 --- a/internal/httpclientx/nilsafety.go +++ b/internal/httpclientx/nilsafety.go @@ -6,7 +6,7 @@ import ( ) // ErrIsNil indicates that [NilSafetyErrorIfNil] was passed a nil value. -var ErrIsNil = errors.New("nil map, pointer or slice") +var ErrIsNil = errors.New("nil map, pointer, or slice") // NilSafetyErrorIfNil returns [ErrIsNil] iff input is a nil map, struct, or slice. // From 1123b4ee78217c06c8f109320fb14fea8fe55415 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Apr 2024 18:17:11 +0200 Subject: [PATCH 11/22] x --- internal/httpclientx/postjson_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/httpclientx/postjson_test.go b/internal/httpclientx/postjson_test.go index ebde2fd055..bed543d8ae 100644 --- a/internal/httpclientx/postjson_test.go +++ b/internal/httpclientx/postjson_test.go @@ -292,7 +292,7 @@ func TestPostJSONLoggingOkay(t *testing.T) { } } -// TestPostJSONCorrectlyRejectsNilValues ensures we correctly reject nil values. +// TestPostJSONCorrectlyRejectsNilValues ensures we do not emit and correctly reject nil values. func TestPostJSONCorrectlyRejectsNilValues(t *testing.T) { t.Run("when sending a nil map", func(t *testing.T) { From d421d24120ec067bec5c93f5841cbc9d7c668355 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 10:13:49 +0200 Subject: [PATCH 12/22] fix: unit test needs to be adapted Previously, we were gracefully handling this case, but honestly it is not the best approach to pretend there's an empty structure if the server breaks the API and returns `"null"` rather than an object. That said, it was still awesome to have this test in place because it helped us to figure out this extra condition of which httpclientx should be aware and that this problem needs to be dealt with systematically inside the httpclientx package. --- internal/oonirun/v2_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index a802b6069a..9ba06db5e0 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" @@ -162,8 +163,8 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) { Session: newMinimalFakeSession(), } r := NewLinkRunner(config, server.URL) - if err := r.Run(ctx); err != nil { - t.Fatal(err) + if err := r.Run(ctx); !errors.Is(err, httpclientx.ErrIsNil) { + t.Fatal("unexpected error", err) } } From 67e0a10eb6ab5016a3251987c6640c893005fc80 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 10:37:06 +0200 Subject: [PATCH 13/22] chore: improve testing for cloudflare IP lookup --- internal/enginelocate/cloudflare.go | 9 +- internal/enginelocate/cloudflare_test.go | 198 ++++++++++++++++++++--- internal/enginelocate/invalid_test.go | 3 +- internal/enginelocate/iplookup.go | 2 +- internal/enginelocate/stun.go | 5 +- internal/enginelocate/ubuntu.go | 3 +- 6 files changed, 192 insertions(+), 28 deletions(-) diff --git a/internal/enginelocate/cloudflare.go b/internal/enginelocate/cloudflare.go index 5f9257bb62..04b22d10d6 100644 --- a/internal/enginelocate/cloudflare.go +++ b/internal/enginelocate/cloudflare.go @@ -2,7 +2,7 @@ package enginelocate import ( "context" - "net/http" + "net" "regexp" "strings" @@ -12,7 +12,7 @@ import ( func cloudflareIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, @@ -34,6 +34,11 @@ func cloudflareIPLookup( r := regexp.MustCompile("(?:ip)=(.*)") ip := strings.Trim(string(r.Find(data)), "ip=") + // make sure the IP addr is valid + if net.ParseIP(ip) == nil { + return model.DefaultProbeIP, ErrInvalidIPAddress + } + // done! return ip, nil } diff --git a/internal/enginelocate/cloudflare_test.go b/internal/enginelocate/cloudflare_test.go index ff49ce9957..223207d410 100644 --- a/internal/enginelocate/cloudflare_test.go +++ b/internal/enginelocate/cloudflare_test.go @@ -2,32 +2,194 @@ package enginelocate import ( "context" + "errors" "net" "net/http" + "net/url" "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) +// cloudflareRealisticresponse is a realistic response returned by cloudflare +// with the IP address modified to belong to a public institution. +var cloudflareRealisticResponse = []byte(` +fl=270f47 +h=www.cloudflare.com +ip=130.192.91.211 +ts=1713946961.154 +visit_scheme=https +uag=Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:125.0) Gecko/20100101 Firefox/125.0 +colo=MXP +sliver=none +http=http/3 +loc=IT +tls=TLSv1.3 +sni=plaintext +warp=off +gateway=off +rbi=off +kex=X25519 +`) + func TestIPLookupWorksUsingcloudlflare(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - netx := &netxlite.Netx{} - ip, err := cloudflareIPLookup( - context.Background(), - http.DefaultClient, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err != nil { - t.Fatal(err) - } - if net.ParseIP(ip) == nil { - t.Fatalf("not an IP address: '%s'", ip) - } + + // We want to make sure the real server gives us an IP address. + t.Run("is working as intended when using the real server", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + http.DefaultClient, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + }) + + // But we also want to make sure everything is working as intended when using + // a local HTTP server, as well as that we can handle errors, so that we can run + // tests in short mode. This is done with the tests below. + + t.Run("is working as intended when using a fake server", func(t *testing.T) { + // create a fake server returning an hardcoded IP address. + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(cloudflareRealisticResponse) + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + + // we expect to see exactly the IP address that we want to see + if ip != "130.192.91.211" { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles errors", func(t *testing.T) { + // create a fake server resetting the connection for the client. + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see ECONNRESET here + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles the case where there's no IP addreess", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`ipx=130.192.91.211`)) // note: different key name + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see ECONNRESET here + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) } diff --git a/internal/enginelocate/invalid_test.go b/internal/enginelocate/invalid_test.go index a6ac6b7f97..d9c76c1fda 100644 --- a/internal/enginelocate/invalid_test.go +++ b/internal/enginelocate/invalid_test.go @@ -2,14 +2,13 @@ package enginelocate import ( "context" - "net/http" "github.com/ooni/probe-cli/v3/internal/model" ) func invalidIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, diff --git a/internal/enginelocate/iplookup.go b/internal/enginelocate/iplookup.go index b618a38d68..ca05b75908 100644 --- a/internal/enginelocate/iplookup.go +++ b/internal/enginelocate/iplookup.go @@ -25,7 +25,7 @@ var ( ) type lookupFunc func( - ctx context.Context, client *http.Client, + ctx context.Context, client model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, ) (string, error) diff --git a/internal/enginelocate/stun.go b/internal/enginelocate/stun.go index b001401a96..d659c9de84 100644 --- a/internal/enginelocate/stun.go +++ b/internal/enginelocate/stun.go @@ -3,7 +3,6 @@ package enginelocate import ( "context" "net" - "net/http" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" @@ -86,7 +85,7 @@ func stunIPLookup(ctx context.Context, config stunConfig) (string, error) { func stunEkigaIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, @@ -100,7 +99,7 @@ func stunEkigaIPLookup( func stunGoogleIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index aabdc59704..b753098d0b 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -3,7 +3,6 @@ package enginelocate import ( "context" "encoding/xml" - "net/http" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" @@ -16,7 +15,7 @@ type ubuntuResponse struct { func ubuntuIPLookup( ctx context.Context, - httpClient *http.Client, + httpClient model.HTTPClient, logger model.Logger, userAgent string, resolver model.Resolver, From a69d98133111802db81fd47281c232967fbf0748 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 11:02:08 +0200 Subject: [PATCH 14/22] chore: improve the ubuntu IP lookup tests --- internal/enginelocate/cloudflare_test.go | 46 +++- internal/enginelocate/fake_test.go | 28 --- internal/enginelocate/ubuntu.go | 6 + internal/enginelocate/ubuntu_test.go | 292 +++++++++++++++++++---- 4 files changed, 301 insertions(+), 71 deletions(-) delete mode 100644 internal/enginelocate/fake_test.go diff --git a/internal/enginelocate/cloudflare_test.go b/internal/enginelocate/cloudflare_test.go index 223207d410..30e5e0bcef 100644 --- a/internal/enginelocate/cloudflare_test.go +++ b/internal/enginelocate/cloudflare_test.go @@ -115,7 +115,7 @@ func TestIPLookupWorksUsingcloudlflare(t *testing.T) { } }) - t.Run("correctly handles errors", func(t *testing.T) { + t.Run("correctly handles network errors", func(t *testing.T) { // create a fake server resetting the connection for the client. srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) defer srv.Close() @@ -153,7 +153,7 @@ func TestIPLookupWorksUsingcloudlflare(t *testing.T) { } }) - t.Run("correctly handles the case where there's no IP addreess", func(t *testing.T) { + t.Run("correctly handles parsing errors", func(t *testing.T) { // create a fake server returnning different keys srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte(`ipx=130.192.91.211`)) // note: different key name @@ -182,7 +182,47 @@ func TestIPLookupWorksUsingcloudlflare(t *testing.T) { netx.NewStdlibResolver(model.DiscardLogger), ) - // we expect to see ECONNRESET here + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`ip=foobarbaz`)) // note: invalid IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using cloudflare + netx := &netxlite.Netx{} + ip, err := cloudflareIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response if !errors.Is(err, ErrInvalidIPAddress) { t.Fatal("unexpected error", err) } diff --git a/internal/enginelocate/fake_test.go b/internal/enginelocate/fake_test.go deleted file mode 100644 index 72933e9ab9..0000000000 --- a/internal/enginelocate/fake_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package enginelocate - -import ( - "net/http" - "time" - - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -type FakeTransport struct { - Err error - Resp *http.Response -} - -func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { - time.Sleep(10 * time.Microsecond) - if req.Body != nil { - netxlite.ReadAllContext(req.Context(), req.Body) - req.Body.Close() - } - if txp.Err != nil { - return nil, txp.Err - } - txp.Resp.Request = req // non thread safe but it doesn't matter - return txp.Resp, nil -} - -func (txp FakeTransport) CloseIdleConnections() {} diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index b753098d0b..6847d337e8 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -3,6 +3,7 @@ package enginelocate import ( "context" "encoding/xml" + "net" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" @@ -33,6 +34,11 @@ func ubuntuIPLookup( return model.DefaultProbeIP, err } + // make sure the IP addr is valid + if net.ParseIP(resp.IP) == nil { + return model.DefaultProbeIP, ErrInvalidIPAddress + } + // handle the success case return resp.IP, nil } diff --git a/internal/enginelocate/ubuntu_test.go b/internal/enginelocate/ubuntu_test.go index e4e313484b..4f305c7556 100644 --- a/internal/enginelocate/ubuntu_test.go +++ b/internal/enginelocate/ubuntu_test.go @@ -2,56 +2,268 @@ package enginelocate import ( "context" - "io" + "errors" "net" "net/http" + "net/url" "strings" "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) -func TestUbuntuParseError(t *testing.T) { - netx := &netxlite.Netx{} - ip, err := ubuntuIPLookup( - context.Background(), - &http.Client{Transport: FakeTransport{ - Resp: &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader("<")), - }, - }}, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") { - t.Fatalf("not the error we expected: %+v", err) - } - if ip != model.DefaultProbeIP { - t.Fatalf("not the expected IP address: %s", ip) - } -} +// ubuntuRealisticresponse is a realistic response returned by cloudflare +// with the IP address modified to belong to a public institution. +var ubuntuRealisticresponse = []byte(` + +130.192.91.211 +OK +IT +ITA +Italy +09 +Lombardia +Sesto San Giovanni +20099 +45.5349 +9.2295 +0 +Europe/Rome + +`) func TestIPLookupWorksUsingUbuntu(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - netx := &netxlite.Netx{} - ip, err := ubuntuIPLookup( - context.Background(), - http.DefaultClient, - log.Log, - model.HTTPHeaderUserAgent, - netx.NewStdlibResolver(model.DiscardLogger), - ) - if err != nil { - t.Fatal(err) - } - if net.ParseIP(ip) == nil { - t.Fatalf("not an IP address: '%s'", ip) - } + + // We want to make sure the real server gives us an IP address. + t.Run("is working as intended when using the real server", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + http.DefaultClient, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + if err != nil { + t.Fatal(err) + } + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + }) + + // But we also want to make sure everything is working as intended when using + // a local HTTP server, as well as that we can handle errors, so that we can run + // tests in short mode. This is done with the tests below. + + t.Run("is working as intended when using a fake server", func(t *testing.T) { + // create a fake server returning an hardcoded IP address. + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(ubuntuRealisticresponse) + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect this call to succeed + if err != nil { + t.Fatal(err) + } + + // we expect to get back a valid IPv4/IPv6 address + if net.ParseIP(ip) == nil { + t.Fatalf("not an IP address: '%s'", ip) + } + + // we expect to see exactly the IP address that we want to see + if ip != "130.192.91.211" { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles network errors", func(t *testing.T) { + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see ECONNRESET here + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles parsing errors", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`<`)) // note: invalid XML + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an XML parsing error here + if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") { + t.Fatalf("not the error we expected: %+v", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles missing IP address in a valid XML document", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(``)) // note: missing IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) + + t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) { + // create a fake server returnning different keys + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`foobarbaz`)) // note: not an IP address + })) + defer srv.Close() + + // create an HTTP client that uses the fake server. + client := &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + // rewrite the request URL to be the one of the fake server + req.URL = runtimex.Try1(url.Parse(srv.URL)) + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // figure out the IP address using ubuntu + netx := &netxlite.Netx{} + ip, err := ubuntuIPLookup( + context.Background(), + client, + log.Log, + model.HTTPHeaderUserAgent, + netx.NewStdlibResolver(model.DiscardLogger), + ) + + // we expect to see an error indicating there's no IP address in the response + if !errors.Is(err, ErrInvalidIPAddress) { + t.Fatal("unexpected error", err) + } + + // the returned IP address should be the default one + if ip != model.DefaultProbeIP { + t.Fatal("unexpected IP address", ip) + } + }) } From 642ae5c965049d944a5097dc53fbed9db2e2f6c4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 11:22:58 +0200 Subject: [PATCH 15/22] x --- internal/enginelocate/ubuntu.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/enginelocate/ubuntu.go b/internal/enginelocate/ubuntu.go index 6847d337e8..c005ee1a7f 100644 --- a/internal/enginelocate/ubuntu.go +++ b/internal/enginelocate/ubuntu.go @@ -22,7 +22,7 @@ func ubuntuIPLookup( resolver model.Resolver, ) (string, error) { // read the HTTP response and parse as XML - resp, err := httpclientx.GetXML[*ubuntuResponse](ctx, "https://geoip.ubuntu.com/lookup", &httpclientx.Config{ + v, err := httpclientx.GetXML[*ubuntuResponse](ctx, "https://geoip.ubuntu.com/lookup", &httpclientx.Config{ Authorization: "", // not needed Client: httpClient, Logger: logger, @@ -35,10 +35,10 @@ func ubuntuIPLookup( } // make sure the IP addr is valid - if net.ParseIP(resp.IP) == nil { + if net.ParseIP(v.IP) == nil { return model.DefaultProbeIP, ErrInvalidIPAddress } // handle the success case - return resp.IP, nil + return v.IP, nil } From 548e6bcba6e05a1a94ff9046049e3f59f9e9ad30 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 12:30:49 +0200 Subject: [PATCH 16/22] doc: document oonirun/v2_test.go tests --- internal/oonirun/v2.go | 50 ++++++++++++++-- internal/oonirun/v2_test.go | 111 +++++++++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 8 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 98c8fe7644..b8b75906fe 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -60,15 +60,12 @@ type V2Nettest struct { TestName string `json:"test_name"` } -// ErrHTTPRequestFailed indicates that an HTTP request failed. -var ErrHTTPRequestFailed = errors.New("oonirun: HTTP request failed") - // getV2DescriptorFromHTTPSURL GETs a v2Descriptor instance from // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, logger model.Logger, URL string) (*V2Descriptor, error) { return httpclientx.GetJSON[*V2Descriptor](ctx, URL, &httpclientx.Config{ - Authorization: "", + Authorization: "", // not needed Client: client, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, @@ -87,7 +84,11 @@ const v2DescriptorCacheKey = "oonirun-v2.state" // v2DescriptorCacheLoad loads the v2DescriptorCache. func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, error) { + // attempt to access the cache data, err := fsstore.Get(v2DescriptorCacheKey) + + // if there's a miss either create a new descriptor or return the + // error if it's something I/O related if err != nil { if errors.Is(err, kvstore.ErrNoSuchKey) { cache := &v2DescriptorCache{ @@ -97,13 +98,19 @@ func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, err } return nil, err } + + // transform the raw descriptor into a struct var cache v2DescriptorCache if err := json.Unmarshal(data, &cache); err != nil { return nil, err } + + // handle the case where there are no entries inside the on-disk cache + // by properly initializing to a non-nil map if cache.Entries == nil { cache.Entries = make(map[string]*V2Descriptor) } + return &cache, nil } @@ -159,13 +166,18 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri // more robust in terms of the implementation. return ErrNilDescriptor } + logger := config.Session.Logger() + for _, nettest := range desc.Nettests { + // early handling of the case where the test name is empty if nettest.TestName == "" { logger.Warn("oonirun: nettest name cannot be empty") v2CountEmptyNettestNames.Add(1) continue } + + // construct an experiment from the current nettest exp := &Experiment{ Annotations: config.Annotations, ExtraOptions: nettest.Options, @@ -184,12 +196,15 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri newSaverFn: nil, newInputProcessorFn: nil, } + + // actually run the experiment if err := exp.Run(ctx); err != nil { logger.Warnf("cannot run experiment: %s", err.Error()) v2CountFailedExperiments.Add(1) continue } } + return nil } @@ -200,14 +215,25 @@ var ErrNeedToAcceptChanges = errors.New("oonirun: need to accept changes") // v2DescriptorDiff shows what changed between the old and the new descriptors. func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string { + // JSON serialize old descriptor oldData, err := json.MarshalIndent(oldValue, "", " ") runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly") + + // JSON serialize new descriptor newData, err := json.MarshalIndent(newValue, "", " ") runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly") + + // make sure the serializations are newline-terminated oldString, newString := string(oldData)+"\n", string(newData)+"\n" + + // generate names for the final diff oldFile := "OLD " + URL newFile := "NEW " + URL + + // compute the edits to update from the old to the new descriptor edits := myers.ComputeEdits(span.URIFromPath(oldFile), oldString, newString) + + // transform the edits and obtain an unified diff return fmt.Sprint(gotextdiff.ToUnified(oldFile, newFile, oldString, edits)) } @@ -224,25 +250,39 @@ func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string { func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { logger := config.Session.Logger() logger.Infof("oonirun/v2: running %s", URL) + + // load the descriptor from the cache cache, err := v2DescriptorCacheLoad(config.KVStore) if err != nil { return err } + + // pull a possibly new descriptor without updating the old descriptor clnt := config.Session.DefaultHTTPClient() oldValue, newValue, err := cache.PullChangesWithoutSideEffects(ctx, clnt, logger, URL) if err != nil { return err } + + // compare the new descriptor to the old descriptor diff := v2DescriptorDiff(oldValue, newValue, URL) + + // possibly stop if configured to ask for permission when accepting changes if !config.AcceptChanges && diff != "" { logger.Warnf("oonirun: %s changed as follows:\n\n%s", URL, diff) logger.Warnf("oonirun: we are not going to run this link until you accept changes") return ErrNeedToAcceptChanges } + + // in case there are changes, update the descriptor if diff != "" { if err := cache.Update(config.KVStore, URL, newValue); err != nil { return err } } - return V2MeasureDescriptor(ctx, config, newValue) // handles nil newValue gracefully + + // measure using the possibly-new descriptor + // + // note: this function gracefully handles nil values + return V2MeasureDescriptor(ctx, config, newValue) } diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index 9ba06db5e0..4744a2c8ae 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -17,6 +17,7 @@ import ( ) func TestOONIRunV2LinkCommonCase(t *testing.T) { + // make a local server that returns a reasonable descriptor for the example experiment server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -34,8 +35,10 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -43,19 +46,24 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { }, KVStore: &kvstore.Memory{}, MaxRuntime: 0, - NoCollector: true, + NoCollector: true, // disable collector so we don't submit NoJSON: true, Random: false, ReportFile: "", Session: newMinimalFakeSession(), } + + // create a link runner for the local server URL r := NewLinkRunner(config, server.URL) + + // run and verify that we could run without getting errors if err := r.Run(ctx); err != nil { t.Fatal(err) } } func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { + // make a server that returns a minimal descriptor for the example experiment server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -73,8 +81,12 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create with a key value store that returns an empty cache and fails to update + // the cache afterwards such that we can see if we detect such an error expected := errors.New("mocked") config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error @@ -96,14 +108,21 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // create new runner for the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run the link err := r.Run(ctx) + + // make sure we exactly got the cache updating error if !errors.Is(err, expected) { t.Fatal("unexpected err", err) } } func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { + // make a local server that would return a reasonable descriptor server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -121,8 +140,11 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: false, // should see "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -136,19 +158,29 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // create a new runner for the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run the link err := r.Run(ctx) + + // make sure the error indicates we need to accept changes if !errors.Is(err, ErrNeedToAcceptChanges) { t.Fatal("unexpected err", err) } } func TestOONIRunV2LinkNilDescriptor(t *testing.T) { + // create a local server that returns a literal "null" as the JSON descriptor server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("null")) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -162,14 +194,23 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // attempt to run the link at the local server r := NewLinkRunner(config, server.URL) + + // make sure we correctly handled an invalid "null" descriptor if err := r.Run(ctx); !errors.Is(err, httpclientx.ErrIsNil) { t.Fatal("unexpected error", err) } } func TestOONIRunV2LinkEmptyTestName(t *testing.T) { + // load the count of the number of cases where the test name was empty so we can + // later on check whether this count has increased due to running this test emptyTestNamesPrev := v2CountEmptyNettestNames.Load() + + // create a local server that will respond with a minimal descriptor that + // actually contains an empty test name, which is what we want to test server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -187,8 +228,11 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -202,30 +246,51 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // construct a link runner relative to the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run and verify there's no error (the code only emits a warning in this case) if err := r.Run(ctx); err != nil { t.Fatal(err) } + + // make sure the loop for running nettests continued where we expected it to do so if v2CountEmptyNettestNames.Load() != emptyTestNamesPrev+1 { t.Fatal("expected to see 1 more instance of empty nettest names") } } func TestV2MeasureDescriptor(t *testing.T) { + t.Run("with nil descriptor", func(t *testing.T) { ctx := context.Background() config := &LinkConfig{} + + // invoke the function with a nil descriptor and make sure the code + // is correctly handling this specific case by returnning error err := V2MeasureDescriptor(ctx, config, nil) + if !errors.Is(err, ErrNilDescriptor) { t.Fatal("unexpected err", err) } }) t.Run("with failing experiment", func(t *testing.T) { + // load the previous count of failed experiments so we can check that it increased later previousFailedExperiments := v2CountFailedExperiments.Load() + expected := errors.New("mocked error") + ctx := context.Background() sess := newMinimalFakeSession() + + // create a mocked submitted that will panic in case we try to submit, such that + // this test fails with a panic if we go as far as attempting to submit + // + // Note: the convention is that we do not submit experiment results when the + // experiment measurement function returns a non-nil error, since such an error + // represents a fundamental failure in setting up the experiment sess.MockNewSubmitter = func(ctx context.Context) (model.Submitter, error) { subm := &mocks.Submitter{ MockSubmit: func(ctx context.Context, m *model.Measurement) error { @@ -234,6 +299,9 @@ func TestV2MeasureDescriptor(t *testing.T) { } return subm, nil } + + // mock an experiment builder where we have the measurement function fail by returning + // an error, which has the meaning indicated in the previous comment sess.MockNewExperimentBuilder = func(name string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ MockInputPolicy: func() model.InputPolicy { @@ -259,6 +327,8 @@ func TestV2MeasureDescriptor(t *testing.T) { } return eb, nil } + + // create a mostly empty config referring to the session config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -270,6 +340,8 @@ func TestV2MeasureDescriptor(t *testing.T) { ReportFile: "", Session: sess, } + + // create a mostly empty descriptor referring to the example experiment descr := &V2Descriptor{ Name: "", Description: "", @@ -280,10 +352,18 @@ func TestV2MeasureDescriptor(t *testing.T) { TestName: "example", }}, } + + // attempt to measure this descriptor err := V2MeasureDescriptor(ctx, config, descr) + + // here we do not expect to see an error because the implementation continues + // until it has run all experiments and just emits warning messages if err != nil { t.Fatal(err) } + + // however there's also a count of the number of times we failed to load + // an experiment and we use that to make sure the code failed where we expected if v2CountFailedExperiments.Load() != previousFailedExperiments+1 { t.Fatal("expected to see a failed experiment") } @@ -291,9 +371,13 @@ func TestV2MeasureDescriptor(t *testing.T) { } func TestV2MeasureHTTPS(t *testing.T) { + t.Run("when we cannot load from cache", func(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() + + // construct the link configuration with a key-value store that fails + // with a well-know error when attempting to load. config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -309,15 +393,22 @@ func TestV2MeasureHTTPS(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // attempt to measure with the given config (there's no need to pass an URL + // here because we should fail to load from the cache first) err := v2MeasureHTTPS(ctx, config, "") + + // verify that we've actually got the expected error if !errors.Is(err, expected) { t.Fatal("unexpected err", err) } }) t.Run("when we cannot pull changes", func(t *testing.T) { + // create and immediately cancel a context so that HTTP would fail ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately + config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -329,25 +420,39 @@ func TestV2MeasureHTTPS(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } - err := v2MeasureHTTPS(ctx, config, "https://example.com") // should not use URL + + // attempt to measure with a random URL (which is fine since we shouldn't use it) + err := v2MeasureHTTPS(ctx, config, "https://example.com") + + // make sure that we've actually go the expected error if !errors.Is(err, context.Canceled) { t.Fatal("unexpected err", err) } }) + } func TestV2DescriptorCacheLoad(t *testing.T) { - t.Run("cannot unmarshal cache content", func(t *testing.T) { + + t.Run("handle the case where we cannot unmarshal the cache content", func(t *testing.T) { + // write an invalid serialized JSON into the cache fsstore := &kvstore.Memory{} if err := fsstore.Set(v2DescriptorCacheKey, []byte("{")); err != nil { t.Fatal(err) } + + // attempt to load descriptors cache, err := v2DescriptorCacheLoad(fsstore) + + // make sure we cannot unmarshal if err == nil || err.Error() != "unexpected end of JSON input" { t.Fatal("unexpected err", err) } + + // make sure the returned cache is nil if cache != nil { t.Fatal("expected nil cache") } }) + } From 4cf3566847548738e88c81f4d4bb511901f0f95b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Apr 2024 15:54:18 +0200 Subject: [PATCH 17/22] start improving probeservices tests As before, here I am going to ensure there's redundancy. --- internal/probeservices/bouncer_test.go | 166 ++++++++++++++++- internal/probeservices/checkin_test.go | 241 +++++++++++++++++++------ 2 files changed, 346 insertions(+), 61 deletions(-) diff --git a/internal/probeservices/bouncer_test.go b/internal/probeservices/bouncer_test.go index 25f23a8519..1fb5c65925 100644 --- a/internal/probeservices/bouncer_test.go +++ b/internal/probeservices/bouncer_test.go @@ -2,18 +2,164 @@ package probeservices import ( "context" + "errors" + "net/http" + "net/url" "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) func TestGetTestHelpers(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - testhelpers, err := newclient().GetTestHelpers(context.Background()) - if err != nil { - t.Fatal(err) - } - if len(testhelpers) <= 1 { - t.Fatal("no returned test helpers?!") - } + + // First, let's check whether we can get a response from the real OONI backend. + t.Run("is working as intended with the real backend", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + // create client + client := newclient() + + // issue the request + testhelpers, err := client.GetTestHelpers(context.Background()) + + // we do not expect an error here + if err != nil { + t.Fatal(err) + } + + // we expect at least one TH + if len(testhelpers) <= 1 { + t.Fatal("no returned test helpers?!") + } + }) + + // Now let's construct a test server that returns a valid response and try + // to communicate with such a test server successfully and with errors + + t.Run("is working as intended with a local test server", func(t *testing.T) { + // this is what we expect to receive + expect := map[string][]model.OOAPIService{ + "web-connectivity": {{ + Address: "https://0.th.ooni.org/", + Type: "https", + }}, + } + + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + runtimex.Assert(r.Method == http.MethodGet, "invalid method") + runtimex.Assert(r.URL.Path == "/api/v1/test-helpers", "invalid URL path") + w.Write(must.MarshalJSON(expect)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the GET request + testhelpers, err := client.GetTestHelpers(context.Background()) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // we expect to see exactly what the server sent + if diff := cmp.Diff(expect, testhelpers); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("reports an error when the connection is reset", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the GET request + testhelpers, err := client.GetTestHelpers(context.Background()) + + // we do expect an error + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // we expect to see a zero-length / nil map + if len(testhelpers) != 0 { + t.Fatal("expected result lenght to be zero") + } + }) + + t.Run("reports an error when the response is not JSON parsable", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{`)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the GET request + testhelpers, err := client.GetTestHelpers(context.Background()) + + // we do expect an error + if err == nil || err.Error() != "unexpected end of JSON input" { + t.Fatal("unexpected error", err) + } + + // we expect to see a zero-length / nil map + if len(testhelpers) != 0 { + t.Fatal("expected result lenght to be zero") + } + }) } diff --git a/internal/probeservices/checkin_test.go b/internal/probeservices/checkin_test.go index b9ea1ace94..87b7571b85 100644 --- a/internal/probeservices/checkin_test.go +++ b/internal/probeservices/checkin_test.go @@ -2,19 +2,23 @@ package probeservices import ( "context" - "strings" + "errors" + "net/http" + "net/url" "testing" + "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) -func TestCheckInSuccess(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - client := newclient() - client.BaseURL = "https://ams-pg-test.ooni.org" +func TestCheckIn(t *testing.T) { + // define a common configuration to use across all tests config := model.OOAPICheckInConfig{ Charging: true, OnWiFi: true, @@ -28,49 +32,184 @@ func TestCheckInSuccess(t *testing.T) { CategoryCodes: []string{"NEWS", "CULTR"}, }, } - ctx := context.Background() - result, err := client.CheckIn(ctx, config) - if err != nil { - t.Fatal(err) - } - if result == nil || result.Tests.WebConnectivity == nil { - t.Fatal("got nil result or WebConnectivity") - } - if result.Tests.WebConnectivity.ReportID == "" { - t.Fatal("ReportID is empty") - } - if len(result.Tests.WebConnectivity.URLs) < 1 { - t.Fatal("unexpected number of URLs") - } - for _, entry := range result.Tests.WebConnectivity.URLs { - if entry.CategoryCode != "NEWS" && entry.CategoryCode != "CULTR" { - t.Fatalf("unexpected category code: %+v", entry) + + t.Run("with the real API server", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") } - } -} -func TestCheckInFailure(t *testing.T) { - client := newclient() - client.BaseURL = "https://\t\t\t/" // cause test to fail - config := model.OOAPICheckInConfig{ - Charging: true, - OnWiFi: true, - Platform: "android", - ProbeASN: "AS12353", - ProbeCC: "PT", - RunType: model.RunTypeTimed, - SoftwareName: "ooniprobe-android", - SoftwareVersion: "2.7.1", - WebConnectivity: model.OOAPICheckInConfigWebConnectivity{ - CategoryCodes: []string{"NEWS", "CULTR"}, - }, - } - ctx := context.Background() - result, err := client.CheckIn(ctx, config) - if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { - t.Fatal("not the error we expected") - } - if result != nil { - t.Fatal("results?!") - } + client := newclient() + client.BaseURL = "https://ams-pg-test.ooni.org" // use the test infra + + ctx := context.Background() + + // call the API + result, err := client.CheckIn(ctx, config) + + // we do not expect to see an error + if err != nil { + t.Fatal(err) + } + + // sanity check the returned response + if result == nil || result.Tests.WebConnectivity == nil { + t.Fatal("got nil result or nil WebConnectivity") + } + if result.Tests.WebConnectivity.ReportID == "" { + t.Fatal("ReportID is empty") + } + if len(result.Tests.WebConnectivity.URLs) < 1 { + t.Fatal("unexpected number of URLs") + } + + // ensure the category codes match our request + for _, entry := range result.Tests.WebConnectivity.URLs { + if entry.CategoryCode != "NEWS" && entry.CategoryCode != "CULTR" { + t.Fatalf("unexpected category code: %+v", entry) + } + } + }) + + t.Run("with a working-as-intended local server", func(t *testing.T) { + // define our expectations + expect := &model.OOAPICheckInResult{ + Conf: model.OOAPICheckInResultConfig{ + Features: map[string]bool{}, + TestHelpers: map[string][]model.OOAPIService{ + "web-connectivity": {{ + Address: "https://0.th.ooni.org/", + Type: "https", + }}, + }, + }, + ProbeASN: "AS30722", + ProbeCC: "US", + Tests: model.OOAPICheckInResultNettests{ + WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{ + ReportID: "20240424T134700Z_webconnectivity_IT_30722_n1_q5N5YSTWEqHYDo9v", + URLs: []model.OOAPIURLInfo{{ + CategoryCode: "NEWS", + CountryCode: "IT", + URL: "https://www.example.com/", + }}, + }, + }, + UTCTime: time.Date(2022, 11, 22, 1, 2, 3, 0, time.UTC), + V: 1, + } + + // create a local server that responds with the expectation + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + runtimex.Assert(r.Method == http.MethodPost, "invalid method") + runtimex.Assert(r.URL.Path == "/api/v1/check-in", "invalid URL path") + rawreqbody := runtimex.Try1(netxlite.ReadAllContext(r.Context(), r.Body)) + var gotrequest model.OOAPICheckInConfig + must.UnmarshalJSON(rawreqbody, &gotrequest) + diff := cmp.Diff(config, gotrequest) + runtimex.Assert(diff == "", "request mismatch:"+diff) + w.Write(must.MarshalJSON(expect)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // call the API + result, err := client.CheckIn(context.Background(), config) + + // we do not expect to see an error + if err != nil { + t.Fatal(err) + } + + // we expect to see exactly what the server sent + if diff := cmp.Diff(expect, result); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("reports an error when the connection is reset", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // call the API + result, err := client.CheckIn(context.Background(), config) + + // we do expect an error + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // we expect to see a nil pointer + if result != nil { + t.Fatal("expected result to be nil") + } + }) + + t.Run("reports an error when the response is not JSON parsable", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{`)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // call the API + result, err := client.CheckIn(context.Background(), config) + + // we do expect an error + if err == nil || err.Error() != "unexpected end of JSON input" { + t.Fatal("unexpected error", err) + } + + // we expect to see a nil pointer + if result != nil { + t.Fatal("expected result to be nil") + } + }) } From e8471c499e61159075ac50382e0d616fea7e5397 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 26 Apr 2024 11:05:08 +0200 Subject: [PATCH 18/22] x --- .../probeservices/measurementmeta_test.go | 301 ++++++++++++------ 1 file changed, 204 insertions(+), 97 deletions(-) diff --git a/internal/probeservices/measurementmeta_test.go b/internal/probeservices/measurementmeta_test.go index 63ed736d73..c2774c9f4c 100644 --- a/internal/probeservices/measurementmeta_test.go +++ b/internal/probeservices/measurementmeta_test.go @@ -2,116 +2,223 @@ package probeservices import ( "context" - "encoding/json" "errors" "net/http" - "sync/atomic" + "net/url" "testing" + "time" - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/httpx" - "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) -func TestGetMeasurementMetaWorkingAsIntended(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } +func TestGetMeasurementMeta(t *testing.T) { - client := Client{ - APIClientTemplate: httpx.APIClientTemplate{ - BaseURL: "https://api.ooni.io/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "miniooni/0.1.0-dev", - }, - KVStore: &kvstore.Memory{}, - LoginCalls: &atomic.Int64{}, - RegisterCalls: &atomic.Int64{}, - StateFile: NewStateFile(&kvstore.Memory{}), - } + // This is the configuration we use for both testing with the real API server + // and for testing with a local HTTP server for -short tests. config := model.OOAPIMeasurementMetaConfig{ ReportID: `20201209T052225Z_urlgetter_IT_30722_n1_E1VUhMz08SEkgYFU`, Full: true, Input: `https://www.example.org`, } - ctx := context.Background() - mmeta, err := client.GetMeasurementMeta(ctx, config) - if err != nil { - t.Fatal(err) - } - if mmeta.Anomaly != false { - t.Fatal("unexpected anomaly value") - } - if mmeta.CategoryCode != "" { - t.Fatal("unexpected category code value") - } - if mmeta.Confirmed != false { - t.Fatal("unexpected confirmed value") - } - if mmeta.Failure != true { - // TODO(bassosimone): this field seems wrong - t.Fatal("unexpected failure value") - } - if mmeta.Input == nil || *mmeta.Input != config.Input { - t.Fatal("unexpected input value") - } - if mmeta.MeasurementStartTime.String() != "2020-12-09 05:22:25 +0000 UTC" { - t.Fatal("unexpected measurement start time value") - } - if mmeta.ProbeASN != 30722 { - t.Fatal("unexpected probe asn value") - } - if mmeta.ProbeCC != "IT" { - t.Fatal("unexpected probe cc value") - } - if mmeta.ReportID != config.ReportID { - t.Fatal("unexpected report id value") - } - // TODO(bassosimone): we could better this check - var scores interface{} - if err := json.Unmarshal([]byte(mmeta.Scores), &scores); err != nil { - t.Fatalf("cannot parse scores value: %+v", err) - } - if mmeta.TestName != "urlgetter" { - t.Fatal("unexpected test name value") - } - if mmeta.TestStartTime.String() != "2020-12-09 05:22:25 +0000 UTC" { - t.Fatal("unexpected test start time value") - } - // TODO(bassosimone): we could better this check - var rawmeas interface{} - if err := json.Unmarshal([]byte(mmeta.RawMeasurement), &rawmeas); err != nil { - t.Fatalf("cannot parse raw measurement: %+v", err) - } -} -func TestGetMeasurementMetaWorkingWithCancelledContext(t *testing.T) { - client := Client{ - APIClientTemplate: httpx.APIClientTemplate{ - BaseURL: "https://api.ooni.io/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "miniooni/0.1.0-dev", - }, - KVStore: &kvstore.Memory{}, - LoginCalls: &atomic.Int64{}, - RegisterCalls: &atomic.Int64{}, - StateFile: NewStateFile(&kvstore.Memory{}), - } - config := model.OOAPIMeasurementMetaConfig{ - ReportID: `20201209T052225Z_urlgetter_IT_30722_n1_E1VUhMz08SEkgYFU`, - Full: true, - Input: `https://www.example.org`, - } - ctx, cancel := context.WithCancel(context.Background()) - cancel() // fail immediately - mmeta, err := client.GetMeasurementMeta(ctx, config) - if !errors.Is(err, context.Canceled) { - t.Fatalf("not the error we expected: %+v", err) - } - if mmeta != nil { - t.Fatal("we expected a nil mmeta here") + // This is what we expectMmeta the API to send us. We share this struct + // because we're using it for testing with the real backend as well as + // for testing with a local test server. + // + // The measurement is marked as "failed". This feels wrong but it may + // be that the fastpah marks all urlgetter measurements as failed. + // + // We are not including the raw measurement for simplicity (also, there are + // not tests for the ooni/backend API anyway, and so it's fine). + expectMmeta := &model.OOAPIMeasurementMeta{ + Anomaly: false, + CategoryCode: "", + Confirmed: false, + Failure: true, + Input: &config.Input, + MeasurementStartTime: time.Date(2020, 12, 9, 5, 22, 25, 0, time.UTC), + ProbeASN: 30722, + ProbeCC: "IT", + ReportID: "20201209T052225Z_urlgetter_IT_30722_n1_E1VUhMz08SEkgYFU", + Scores: `{"blocking_general":0.0,"blocking_global":0.0,"blocking_country":0.0,"blocking_isp":0.0,"blocking_local":0.0,"accuracy":0.0}`, + TestName: "urlgetter", + TestStartTime: time.Date(2020, 12, 9, 5, 22, 25, 0, time.UTC), + RawMeasurement: "", } + + // First, let's check whether we can get a response from the real OONI backend. + t.Run("is working as intended with the real backend", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + // construct a client and override the URL to be the production backend + // instead of the testing backend, so we have stable measurements + client := newclient() + client.BaseURL = "https://api.ooni.io/" + + // issue the API call proper + mmeta, err := client.GetMeasurementMeta(context.Background(), config) + + // we do not expect to see errors obviously + if err != nil { + t.Fatal(err) + } + + // the raw measurement must not be empty and must parse as JSON + // + // once we know that, clear it for the subsequent cmp.Diff + if mmeta.RawMeasurement == "" { + t.Fatal("mmeta.RawMeasurement should not be empty") + } + var rawmeas any + must.UnmarshalJSON([]byte(mmeta.RawMeasurement), &rawmeas) + mmeta.RawMeasurement = "" + + // compare with the expectation + if diff := cmp.Diff(expectMmeta, mmeta); diff != "" { + t.Fatal(diff) + } + }) + + // Now let's construct a test server that returns a valid response and try + // to communicate with such a test server successfully and with errors + + t.Run("is working as intended with a local test server", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + runtimex.Assert(r.Method == http.MethodGet, "invalid method") + runtimex.Assert(r.URL.Path == "/api/v1/measurement_meta", "invalid URL path") + w.Write(must.MarshalJSON(expectMmeta)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the API call proper + mmeta, err := client.GetMeasurementMeta(context.Background(), config) + + // we do not expect to see errors obviously + if err != nil { + t.Fatal(err) + } + + // compare with the expectation + if diff := cmp.Diff(expectMmeta, mmeta); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("reports an error when the connection is reset", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the API call proper + mmeta, err := client.GetMeasurementMeta(context.Background(), config) + + // we do expect an error + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // we expect mmeta to be nil + if mmeta != nil { + t.Fatal("expected nil meta") + } + }) + + t.Run("reports an error when the response is not JSON parsable", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{`)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // issue the API call proper + mmeta, err := client.GetMeasurementMeta(context.Background(), config) + + // we do expect an error + if err == nil || err.Error() != "unexpected end of JSON input" { + t.Fatal("unexpected error", err) + } + + // we expect mmeta to be nil + if mmeta != nil { + t.Fatal("expected nil meta") + } + }) + + t.Run("correctly handles the case where the URL is unparseable", func(t *testing.T) { + // create a probeservices client + client := newclient() + + // override the URL to be unparseable + client.BaseURL = "\t\t\t" + + // issue the API call proper + mmeta, err := client.GetMeasurementMeta(context.Background(), config) + + // we do expect an error + if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` { + t.Fatal("unexpected error", err) + } + + // we expect mmeta to be nil + if mmeta != nil { + t.Fatal("expected nil meta") + } + }) } From 08e81a9cc2490789e57032e6690afceb22ebd86a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 26 Apr 2024 14:10:42 +0200 Subject: [PATCH 19/22] x --- internal/probeservices/psiphon_test.go | 132 +++-- internal/testingx/oonibackendwithlogin.go | 289 ++++++++++ .../testingx/oonibackendwithlogin_test.go | 504 ++++++++++++++++++ 3 files changed, 890 insertions(+), 35 deletions(-) create mode 100644 internal/testingx/oonibackendwithlogin.go create mode 100644 internal/testingx/oonibackendwithlogin_test.go diff --git a/internal/probeservices/psiphon_test.go b/internal/probeservices/psiphon_test.go index 3bfacd2de2..331233f712 100644 --- a/internal/probeservices/psiphon_test.go +++ b/internal/probeservices/psiphon_test.go @@ -4,44 +4,106 @@ import ( "context" "encoding/json" "errors" + "net/http" + "net/url" "testing" + + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) func TestFetchPsiphonConfig(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - clnt := newclient() - if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { - t.Fatal(err) - } - if err := clnt.MaybeLogin(context.Background()); err != nil { - t.Fatal(err) - } - data, err := clnt.FetchPsiphonConfig(context.Background()) - if err != nil { - t.Fatal(err) - } - var config interface{} - if err := json.Unmarshal(data, &config); err != nil { - t.Fatal(err) - } -} -func TestFetchPsiphonConfigNotRegistered(t *testing.T) { - clnt := newclient() - state := State{ - // Explicitly empty so the test is more clear - } - if err := clnt.StateFile.Set(state); err != nil { - t.Fatal(err) - } - data, err := clnt.FetchPsiphonConfig(context.Background()) - if !errors.Is(err, ErrNotRegistered) { - t.Fatal("expected an error here") - } - if data != nil { - t.Fatal("expected nil data here") - } + // First, let's check whether we can get a response from the real OONI backend. + t.Run("is working as intended with the real backend", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + + clnt := newclient() + + // preconditions: to fetch the psiphon config we need to be register and login + if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { + t.Fatal(err) + } + if err := clnt.MaybeLogin(context.Background()); err != nil { + t.Fatal(err) + } + + // then we can try to fetch the config + data, err := clnt.FetchPsiphonConfig(context.Background()) + + // we do not expect an error here + if err != nil { + t.Fatal(err) + } + + // the config is bytes but we want to make sure we can parse it + var config interface{} + if err := json.Unmarshal(data, &config); err != nil { + t.Fatal(err) + } + }) + + // Now let's construct a test server that returns a valid response and try + // to communicate with such a test server successfully and with errors + + t.Run("is working as intended with a local test server", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + runtimex.Assert(r.Method == http.MethodGet, "invalid method") + runtimex.Assert(r.URL.Path == "/api/v1/test-list/psiphon-config", "invalid URL path") + w.Write(must.MarshalJSON(`{}`)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // then we can try to fetch the config + data, err := client.FetchPsiphonConfig(context.Background()) + + _, _ = data, err + t.Fatal("this test is too simplistic") + }) + + t.Run("when we're not registered", func(t *testing.T) { + clnt := newclient() + + // With explicitly empty state so it's pretty obvioust there's no state + state := State{} + + // force the state to be empty + if err := clnt.StateFile.Set(state); err != nil { + t.Fatal(err) + } + + // attempt to fetch the config + data, err := clnt.FetchPsiphonConfig(context.Background()) + + // ensure that the error says we're not registered + if !errors.Is(err, ErrNotRegistered) { + t.Fatal("expected an error here") + } + + // obviously the data should be empty as well + if len(data) != 0 { + t.Fatal("expected nil data here") + } + }) } diff --git a/internal/testingx/oonibackendwithlogin.go b/internal/testingx/oonibackendwithlogin.go new file mode 100644 index 0000000000..74f60baf50 --- /dev/null +++ b/internal/testingx/oonibackendwithlogin.go @@ -0,0 +1,289 @@ +package testingx + +// +// Code for testing the OONI backend login flow. +// + +import ( + "errors" + "fmt" + "io" + "net/http" + "sync" + "time" + + "github.com/google/uuid" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// OONIBackendWithLoginFlowUserRecord is a user record used by [OONIBackendWithLoginFlow]. +type OONIBackendWithLoginFlowUserRecord struct { + Expire time.Time + Password string + Token string +} + +// OONIBackendWithLoginFlow is an [http.Handler] that implements the register and +// loging workflow and serves psiphon and tor config. +// +// The zero value is ready to use. +// +// This struct methods panics for several errors. Only use for testing purposes! +type OONIBackendWithLoginFlow struct { + // logins maps the existing login names to their password. + logins map[string]*OONIBackendWithLoginFlowUserRecord + + // mu provides mutual exclusion. + mu sync.Mutex + + // psiphonConfig is the serialized psiphon config to send to authenticated clients. + psiphonConfig []byte + + // tokens maps a token to a user record. + tokens map[string]*OONIBackendWithLoginFlowUserRecord + + // torTargets is the serialized tor config to send to authenticated clients. + torTargets []byte +} + +// SetPsiphonConfig sets psiphon configuration to use. +// +// This method is safe to call concurrently with incoming HTTP requests. +func (h *OONIBackendWithLoginFlow) SetPsiphonConfig(config []byte) { + defer h.mu.Unlock() + h.mu.Lock() + h.psiphonConfig = config +} + +// SetTorTargets sets tor targets to use. +// +// This method is safe to call concurrently with incoming HTTP requests. +func (h *OONIBackendWithLoginFlow) SetTorTargets(config []byte) { + defer h.mu.Unlock() + h.mu.Lock() + h.torTargets = config +} + +// DoWithLockedUserRecord performs an action with the given user record. The action will +// run while we're holding the [*OONIBackendWithLoginFlow] mutex. +func (h *OONIBackendWithLoginFlow) DoWithLockedUserRecord( + username string, fx func(rec *OONIBackendWithLoginFlowUserRecord) error) error { + defer h.mu.Unlock() + h.mu.Lock() + rec := h.logins[username] + if rec == nil { + return errors.New("no such record") + } + return fx(rec) +} + +// NewMux constructs an [*http.ServeMux] configured with the correct routing. +func (h *OONIBackendWithLoginFlow) NewMux() *http.ServeMux { + mux := http.NewServeMux() + mux.Handle("/api/v1/register", h.handleRegister()) + mux.Handle("/api/v1/login", h.handleLogin()) + mux.Handle("/api/v1/test-list/psiphon-config", h.withAuthentication(h.handlePsiphonConfig())) + mux.Handle("/api/v1/test-list/tor-targets", h.withAuthentication(h.handleTorTargets())) + return mux +} + +func (h *OONIBackendWithLoginFlow) handleRegister() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // make sure the method is OK + if r.Method != http.MethodPost { + w.WriteHeader(501) + return + } + + // read the raw request body + rawreqbody := runtimex.Try1(io.ReadAll(r.Body)) + + // unmarshal the request + var request model.OOAPIRegisterRequest + must.UnmarshalJSON(rawreqbody, &request) + + // lock the users database + h.mu.Lock() + + // make sure the map is usable + if h.logins == nil { + h.logins = make(map[string]*OONIBackendWithLoginFlowUserRecord) + } + + // create new login + userID := uuid.Must(uuid.NewRandom()).String() + + // save login + h.logins[userID] = &OONIBackendWithLoginFlowUserRecord{ + Expire: time.Time{}, + Password: request.Password, + Token: "", + } + + // unlock the users database + h.mu.Unlock() + + // prepare response + response := &model.OOAPIRegisterResponse{ + ClientID: userID, + } + + // send response + w.Write(must.MarshalJSON(response)) + }) +} + +func (h *OONIBackendWithLoginFlow) handleLogin() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // make sure the method is OK + if r.Method != http.MethodPost { + w.WriteHeader(501) + return + } + + // read the raw request body + rawreqbody := runtimex.Try1(io.ReadAll(r.Body)) + + // unmarshal the request + var request model.OOAPILoginCredentials + must.UnmarshalJSON(rawreqbody, &request) + + // lock the users database + h.mu.Lock() + + // attempt to access user record + record := h.logins[request.Username] + + // handle the case where the user does not exist + if record == nil { + // unlock the users database + h.mu.Unlock() + + // return 401 + w.WriteHeader(http.StatusUnauthorized) + return + } + + // handle the case where the password is invalid + if request.Password != record.Password { + // unlock the users database + h.mu.Unlock() + + // return 401 + w.WriteHeader(http.StatusUnauthorized) + return + } + + // create token + token := uuid.Must(uuid.NewRandom()).String() + + // create expiry date + expirydate := time.Now().Add(10 * time.Minute) + + // update record + record.Token = token + record.Expire = expirydate + + // create the token bearer header + bearer := fmt.Sprintf("Bearer %s", token) + + // make sure the tokens map is okay + if h.tokens == nil { + h.tokens = make(map[string]*OONIBackendWithLoginFlowUserRecord) + } + + // update the tokens map + h.tokens[bearer] = record + + // unlock the users database + h.mu.Unlock() + + // prepare response + response := &model.OOAPILoginAuth{ + Expire: expirydate, + Token: token, + } + + // send response + w.Write(must.MarshalJSON(response)) + }) +} + +func (h *OONIBackendWithLoginFlow) handlePsiphonConfig() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // make sure the method is OK + if r.Method != http.MethodGet { + w.WriteHeader(501) + return + } + + // we must lock because of SetPsiphonConfig + h.mu.Lock() + w.Write(h.psiphonConfig) + h.mu.Unlock() + }) +} + +func (h *OONIBackendWithLoginFlow) handleTorTargets() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // make sure the method is OK + if r.Method != http.MethodGet { + w.WriteHeader(501) + return + } + + // make sure the client has provided the right query string + cc := r.URL.Query().Get("country_code") + if cc == "" { + w.WriteHeader(http.StatusBadRequest) + return + } + + // we must lock because of SetTorTargets + h.mu.Lock() + w.Write(h.torTargets) + h.mu.Unlock() + }) + +} + +func (h *OONIBackendWithLoginFlow) withAuthentication(child http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // get the authorization header + authorization := r.Header.Get("Authorization") + + // lock the users database + h.mu.Lock() + + // check whether we have state + record := h.tokens[authorization] + + // handle the case of nonexisting state + if record == nil { + // unlock the users database + h.mu.Unlock() + + // return 401 + w.WriteHeader(http.StatusUnauthorized) + return + } + + // handle the case of expired state + if time.Until(record.Expire) <= 0 { + // unlock the users database + h.mu.Unlock() + + // return 401 + w.WriteHeader(http.StatusUnauthorized) + return + } + + // unlock the users database + h.mu.Unlock() + + // defer to the child handler + child.ServeHTTP(w, r) + }) +} diff --git a/internal/testingx/oonibackendwithlogin_test.go b/internal/testingx/oonibackendwithlogin_test.go new file mode 100644 index 0000000000..a9e6f749cc --- /dev/null +++ b/internal/testingx/oonibackendwithlogin_test.go @@ -0,0 +1,504 @@ +package testingx + +import ( + "bytes" + "fmt" + "io" + "net/http" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/google/uuid" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/urlx" +) + +func TestOONIBackendWithLoginFlow(t *testing.T) { + // create state + state := &OONIBackendWithLoginFlow{} + + // create local testing server + server := MustNewHTTPServer(state.NewMux()) + defer server.Close() + + // create a fake filler + ff := &FakeFiller{} + + t.Run("it may be that there's no user record", func(t *testing.T) { + err := state.DoWithLockedUserRecord("foobar", func(rec *OONIBackendWithLoginFlowUserRecord) error { + panic("should not be called") + }) + if err == nil || err.Error() != "no such record" { + t.Fatal("unexpected error", err) + } + }) + + t.Run("attempt login with invalid method", func(t *testing.T) { + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "GET", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/login", "")), + nil, + )) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusNotImplemented { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("attempt login with invalid credentials", func(t *testing.T) { + // create fake login request + request := &model.OOAPILoginCredentials{} + + // fill it with random data + ff.Fill(&request) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "POST", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/login", "")), + bytes.NewReader(must.MarshalJSON(request)), + )) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to be unauthorized + if resp.StatusCode != http.StatusUnauthorized { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("attempt register with invalid method", func(t *testing.T) { + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "GET", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/register", "")), + nil, + )) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusNotImplemented { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + // registerflow attempts to register and returns the username and password + registerflow := func(t *testing.T) (string, string) { + // create fake register request + // + // we ignore the metadata because we're testing + request := &model.OOAPIRegisterRequest{ + OOAPIProbeMetadata: model.OOAPIProbeMetadata{}, + Password: uuid.Must(uuid.NewRandom()).String(), + } + + // fill it with random data + ff.Fill(&request) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "POST", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/register", "")), + bytes.NewReader(must.MarshalJSON(request)), + )) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to be unauthorized + if resp.StatusCode != http.StatusOK { + t.Fatal("unexpected status code", resp.StatusCode) + } + + // read response body + rawrespbody := runtimex.Try1(io.ReadAll(resp.Body)) + + // parse the response body + var response model.OOAPIRegisterResponse + must.UnmarshalJSON(rawrespbody, &response) + + // return username and password + return response.ClientID, request.Password + } + + t.Run("successful register", func(t *testing.T) { + _, _ = registerflow(t) + }) + + loginrequest := func(username, password string) *http.Response { + // create fake login request + request := &model.OOAPILoginCredentials{ + Username: username, + Password: password, + } + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "POST", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/login", "")), + bytes.NewReader(must.MarshalJSON(request)), + )) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + return resp + } + + loginflow := func(username, password string) (string, time.Time) { + // get the response + resp := loginrequest(username, password) + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to be unauthorized + if resp.StatusCode != http.StatusOK { + t.Fatal("unexpected status code", resp.StatusCode) + } + + // read response body + rawrespbody := runtimex.Try1(io.ReadAll(resp.Body)) + + // parse the response body + var response model.OOAPILoginAuth + must.UnmarshalJSON(rawrespbody, &response) + + // return username and password + return response.Token, response.Expire + } + + t.Run("successful login", func(t *testing.T) { + _, _ = loginflow(registerflow(t)) + }) + + t.Run("login with invalid password", func(t *testing.T) { + // obtain the credentials + username, _ := registerflow(t) + + // obtain the response + resp := loginrequest(username, "antani") + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusUnauthorized { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("get psiphon config with invalid method", func(t *testing.T) { + // obtain the token + token, _ := loginflow(registerflow(t)) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "DELETE", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/psiphon-config", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusNotImplemented { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("get tor targets with invalid method", func(t *testing.T) { + // obtain the token + token, _ := loginflow(registerflow(t)) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "DELETE", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/tor-targets", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusNotImplemented { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("get psiphon config with invalid token", func(t *testing.T) { + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "DELETE", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/psiphon-config", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", "antani")) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusUnauthorized { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("get psiphon config with expired token", func(t *testing.T) { + // obtain the credentials + username, password := registerflow(t) + + // obtain the token + token, _ := loginflow(username, password) + + // modify the token expiry time so that it's expired + state.DoWithLockedUserRecord(username, func(rec *OONIBackendWithLoginFlowUserRecord) error { + rec.Expire = time.Now().Add(-1 * time.Hour) + return nil + }) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "DELETE", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/psiphon-config", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusUnauthorized { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) + + t.Run("we can get psiphon config", func(t *testing.T) { + // define the expected body + expectedbody := []byte(`bonsoir elliot`) + + // set the config + state.SetPsiphonConfig(expectedbody) + + // obtain the credentials + username, password := registerflow(t) + + // obtain the token + token, _ := loginflow(username, password) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "GET", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/psiphon-config", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusOK { + t.Fatal("unexpected status code", resp.StatusCode) + } + + // read the full body + rawrespbody := runtimex.Try1(io.ReadAll(resp.Body)) + + // make sure we've got the expected body + if diff := cmp.Diff(expectedbody, rawrespbody); err != nil { + t.Fatal(diff) + } + }) + + t.Run("we can get tor targets", func(t *testing.T) { + // define the expected body + expectedbody := []byte(`bonsoir elliot`) + + // set the targets + state.SetTorTargets(expectedbody) + + // obtain the credentials + username, password := registerflow(t) + + // obtain the token + token, _ := loginflow(username, password) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "GET", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/tor-targets", "country_code=IT")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusOK { + t.Fatal("unexpected status code", resp.StatusCode) + } + + // read the full body + rawrespbody := runtimex.Try1(io.ReadAll(resp.Body)) + + // make sure we've got the expected body + if diff := cmp.Diff(expectedbody, rawrespbody); err != nil { + t.Fatal(diff) + } + }) + + t.Run("we need query string to get tor targets", func(t *testing.T) { + // define the expected body + expectedbody := []byte(`bonsoir elliot`) + + // set the targets + state.SetTorTargets(expectedbody) + + // obtain the credentials + username, password := registerflow(t) + + // obtain the token + token, _ := loginflow(username, password) + + // create HTTP request + req := runtimex.Try1(http.NewRequest( + "GET", + runtimex.Try1(urlx.ResolveReference(server.URL, "/api/v1/test-list/tor-targets", "")), + nil, + )) + + // create the authorization token + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + // perform the round trip + resp, err := http.DefaultClient.Do(req) + + // we do not expect an error + if err != nil { + t.Fatal(err) + } + + // make sure we eventually close the body + defer resp.Body.Close() + + // we expect to see not implemented + if resp.StatusCode != http.StatusBadRequest { + t.Fatal("unexpected status code", resp.StatusCode) + } + }) +} From a7e748fb7f8883facad158e8f589650ae46c5ef8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 26 Apr 2024 15:47:09 +0200 Subject: [PATCH 20/22] x --- internal/probeservices/psiphon_test.go | 126 +++++++++++++++++++++---- 1 file changed, 109 insertions(+), 17 deletions(-) diff --git a/internal/probeservices/psiphon_test.go b/internal/probeservices/psiphon_test.go index 331233f712..3f8edef684 100644 --- a/internal/probeservices/psiphon_test.go +++ b/internal/probeservices/psiphon_test.go @@ -7,15 +7,30 @@ import ( "net/http" "net/url" "testing" + "time" "github.com/ooni/probe-cli/v3/internal/mocks" - "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/ooni/probe-cli/v3/internal/testingx" ) func TestFetchPsiphonConfig(t *testing.T) { + // psiphonflow is the flow with which we invoke the psiphon API + psiphonflow := func(t *testing.T, client *Client) ([]byte, error) { + // we need to make sure we're registered and logged in + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { + t.Fatal(err) + } + if err := client.MaybeLogin(context.Background()); err != nil { + t.Fatal(err) + } + + // then we can try to fetch the config + return client.FetchPsiphonConfig(context.Background()) + } + // First, let's check whether we can get a response from the real OONI backend. t.Run("is working as intended with the real backend", func(t *testing.T) { if testing.Short() { @@ -24,16 +39,53 @@ func TestFetchPsiphonConfig(t *testing.T) { clnt := newclient() - // preconditions: to fetch the psiphon config we need to be register and login - if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { + // run the psiphon flow + data, err := psiphonflow(t, clnt) + + // we do not expect an error here + if err != nil { t.Fatal(err) } - if err := clnt.MaybeLogin(context.Background()); err != nil { + + // the config is bytes but we want to make sure we can parse it + var config interface{} + if err := json.Unmarshal(data, &config); err != nil { t.Fatal(err) } + }) + + // Now let's construct a test server that returns a valid response and try + // to communicate with such a test server successfully and with errors + + t.Run("is working as intended with a local test server", func(t *testing.T) { + // create state for emulating the OONI backend + state := &testingx.OONIBackendWithLoginFlow{} + + // make sure we return something that is JSON parseable + state.SetPsiphonConfig([]byte(`{}`)) + + // expose the state via HTTP + srv := testingx.MustNewHTTPServer(state.NewMux()) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client so we speak with out local server rather than the true backend + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } // then we can try to fetch the config - data, err := clnt.FetchPsiphonConfig(context.Background()) + data, err := psiphonflow(t, client) // we do not expect an error here if err != nil { @@ -47,16 +99,9 @@ func TestFetchPsiphonConfig(t *testing.T) { } }) - // Now let's construct a test server that returns a valid response and try - // to communicate with such a test server successfully and with errors - - t.Run("is working as intended with a local test server", func(t *testing.T) { + t.Run("reports an error when the connection is reset", func(t *testing.T) { // create quick and dirty server to serve the response - srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - runtimex.Assert(r.Method == http.MethodGet, "invalid method") - runtimex.Assert(r.URL.Path == "/api/v1/test-list/psiphon-config", "invalid URL path") - w.Write(must.MarshalJSON(`{}`)) - })) + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) defer srv.Close() // create a probeservices client @@ -75,11 +120,28 @@ func TestFetchPsiphonConfig(t *testing.T) { }, } - // then we can try to fetch the config + // we need to convince the client that we're logged in first otherwise it will + // refuse to send a request to the server and we won't be testing networking + runtimex.Try0(client.StateFile.Set(State{ + ClientID: "ttt-uuu-iii", + Expire: time.Now().Add(30 * time.Hour), + Password: "xxx-xxx-xxx", + Token: "abc-yyy-zzz", + })) + + // issue the call directly: no register or login otherwise we're testing + // the register or login implementation LOL data, err := client.FetchPsiphonConfig(context.Background()) - _, _ = data, err - t.Fatal("this test is too simplistic") + // we do expect an error + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // we expect to see zero-length data + if len(data) != 0 { + t.Fatal("expected result lenght to be zero") + } }) t.Run("when we're not registered", func(t *testing.T) { @@ -106,4 +168,34 @@ func TestFetchPsiphonConfig(t *testing.T) { t.Fatal("expected nil data here") } }) + + t.Run("correctly handles the case where the URL is unparseable", func(t *testing.T) { + // create a probeservices client + client := newclient() + + // override the URL to be unparseable + client.BaseURL = "\t\t\t" + + // we need to convince the client that we're logged in first otherwise it will + // refuse to send a request to the server and we won't be testing networking + runtimex.Try0(client.StateFile.Set(State{ + ClientID: "ttt-uuu-iii", + Expire: time.Now().Add(30 * time.Hour), + Password: "xxx-xxx-xxx", + Token: "abc-yyy-zzz", + })) + + // issue the API call proper + data, err := client.FetchPsiphonConfig(context.Background()) + + // we do expect an error + if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` { + t.Fatal("unexpected error", err) + } + + // we expect data to be zero length + if len(data) != 0 { + t.Fatal("expected zero length data") + } + }) } From 87146ccb75ca33046adbda4800418b0f8525824b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 26 Apr 2024 16:01:27 +0200 Subject: [PATCH 21/22] x --- internal/probeservices/tor_test.go | 257 +++++++++++++++++++++-------- 1 file changed, 187 insertions(+), 70 deletions(-) diff --git a/internal/probeservices/tor_test.go b/internal/probeservices/tor_test.go index 62f7065143..2d3ede152b 100644 --- a/internal/probeservices/tor_test.go +++ b/internal/probeservices/tor_test.go @@ -2,85 +2,202 @@ package probeservices import ( "context" + "errors" "net/http" + "net/url" "testing" + "time" + + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) func TestFetchTorTargets(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - clnt := newclient() - if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { - t.Fatal(err) - } - if err := clnt.MaybeLogin(context.Background()); err != nil { - t.Fatal(err) - } - data, err := clnt.FetchTorTargets(context.Background(), "ZZ") - if err != nil { - t.Fatal(err) - } - if data == nil || len(data) <= 0 { - t.Fatal("invalid data") - } -} + // Implementation note: OONIBackendWithLoginFlow ensures that tor sends a query + // string and there are also tests making sure of that. We use to have a test that + // checked for the query string here, but now it seems unnecessary. -func TestFetchTorTargetsNotRegistered(t *testing.T) { - clnt := newclient() - state := State{ - // Explicitly empty so the test is more clear - } - if err := clnt.StateFile.Set(state); err != nil { - t.Fatal(err) - } - data, err := clnt.FetchTorTargets(context.Background(), "ZZ") - if err == nil { - t.Fatal("expected an error here") - } - if data != nil { - t.Fatal("expected nil data here") + // torflow is the flow with which we invoke the tor API + torflow := func(t *testing.T, client *Client) (map[string]model.OOAPITorTarget, error) { + // we need to make sure we're registered and logged in + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { + t.Fatal(err) + } + if err := client.MaybeLogin(context.Background()); err != nil { + t.Fatal(err) + } + + // then we can try to fetch the config + return client.FetchTorTargets(context.Background(), "ZZ") } -} -type FetchTorTargetsHTTPTransport struct { - Response *http.Response -} + // First, let's check whether we can get a response from the real OONI backend. + t.Run("is working as intended with the real backend", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } -func (clnt *FetchTorTargetsHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := http.DefaultTransport.RoundTrip(req) - if err != nil { - return nil, err - } - if req.URL.Path == "/api/v1/test-list/tor-targets" { - clnt.Response = resp - } - return resp, err -} + clnt := newclient() -func TestFetchTorTargetsSetsQueryString(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } + // run the tor flow + targets, err := torflow(t, clnt) + + // we do not expect an error here + if err != nil { + t.Fatal(err) + } + + // we expect non-zero length targets + if len(targets) <= 0 { + t.Fatal("expected non-zero-length targets") + } + }) + + t.Run("reports an error when the connection is reset", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // we need to convince the client that we're logged in first otherwise it will + // refuse to send a request to the server and we won't be testing networking + runtimex.Try0(client.StateFile.Set(State{ + ClientID: "ttt-uuu-iii", + Expire: time.Now().Add(30 * time.Hour), + Password: "xxx-xxx-xxx", + Token: "abc-yyy-zzz", + })) + + // run the tor flow + targets, err := torflow(t, client) + + // we do expect an error + if !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } + + // we expect to see zero-length targets + if len(targets) != 0 { + t.Fatal("expected targets to be zero length") + } + }) + + t.Run("reports an error when the response is not JSON parsable", func(t *testing.T) { + // create quick and dirty server to serve the response + srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{`)) + })) + defer srv.Close() + + // create a probeservices client + client := newclient() + + // override the HTTP client + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + URL := runtimex.Try1(url.Parse(srv.URL)) + req.URL.Scheme = URL.Scheme + req.URL.Host = URL.Host + return http.DefaultClient.Do(req) + }, + MockCloseIdleConnections: func() { + http.DefaultClient.CloseIdleConnections() + }, + } + + // we need to convince the client that we're logged in first otherwise it will + // refuse to send a request to the server and we won't be testing networking + runtimex.Try0(client.StateFile.Set(State{ + ClientID: "ttt-uuu-iii", + Expire: time.Now().Add(30 * time.Hour), + Password: "xxx-xxx-xxx", + Token: "abc-yyy-zzz", + })) + + // run the tor flow + targets, err := torflow(t, client) + + // we do expect an error + if err == nil || err.Error() != "unexpected end of JSON input" { + t.Fatal("unexpected error", err) + } + + // we expect to see zero-length targets + if len(targets) != 0 { + t.Fatal("expected targets to be zero length") + } + }) + + t.Run("when we're not registered", func(t *testing.T) { + clnt := newclient() + + // With explicitly empty state so it's pretty obvioust there's no state + state := State{} + + // force the state to be empty + if err := clnt.StateFile.Set(state); err != nil { + t.Fatal(err) + } + + // run the tor flow + targets, err := clnt.FetchTorTargets(context.Background(), "ZZ") + + // ensure that the error says we're not registered + if !errors.Is(err, ErrNotRegistered) { + t.Fatal("expected an error here") + } + + // we expect zero length targets + if len(targets) != 0 { + t.Fatal("expected zero-length targets") + } + }) + + t.Run("correctly handles the case where the URL is unparseable", func(t *testing.T) { + // create a probeservices client + client := newclient() + + // override the URL to be unparseable + client.BaseURL = "\t\t\t" + + // we need to convince the client that we're logged in first otherwise it will + // refuse to send a request to the server and we won't be testing networking + runtimex.Try0(client.StateFile.Set(State{ + ClientID: "ttt-uuu-iii", + Expire: time.Now().Add(30 * time.Hour), + Password: "xxx-xxx-xxx", + Token: "abc-yyy-zzz", + })) + + targets, err := client.FetchTorTargets(context.Background(), "ZZ") + + // we do expect an error + if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` { + t.Fatal("unexpected error", err) + } + + // we expect zero length targets + if len(targets) != 0 { + t.Fatal("expected zero-length targets") + } + }) - clnt := newclient() - txp := new(FetchTorTargetsHTTPTransport) - clnt.HTTPClient = &http.Client{Transport: txp} - if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { - t.Fatal(err) - } - if err := clnt.MaybeLogin(context.Background()); err != nil { - t.Fatal(err) - } - data, err := clnt.FetchTorTargets(context.Background(), "ZZ") - if err != nil { - t.Fatal(err) - } - if data == nil || len(data) <= 0 { - t.Fatal("invalid data") - } - requestURL := txp.Response.Request.URL - if requestURL.Query().Get("country_code") != "ZZ" { - t.Fatal("invalid country code") - } } From 45bd046953d3968b8f8c769e01386236c98c981f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 29 Apr 2024 09:51:23 +0200 Subject: [PATCH 22/22] x --- internal/oonirun/v2.go | 19 +++++++--- internal/oonirun/v2_test.go | 4 +- internal/probeservices/bouncer.go | 25 +++---------- internal/probeservices/collector.go | 45 ++++------------------- internal/probeservices/login.go | 21 ++--------- internal/probeservices/measurementmeta.go | 26 +++++-------- internal/probeservices/psiphon.go | 22 +---------- internal/probeservices/register.go | 25 ++----------- internal/probeservices/tor.go | 27 +++----------- 9 files changed, 51 insertions(+), 163 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index b8b75906fe..54b583bf34 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -14,7 +14,7 @@ import ( "github.com/hexops/gotextdiff" "github.com/hexops/gotextdiff/myers" "github.com/hexops/gotextdiff/span" - "github.com/ooni/probe-cli/v3/internal/httpclientx" + "github.com/ooni/probe-cli/v3/internal/httpx" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" @@ -64,12 +64,21 @@ type V2Nettest struct { // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, logger model.Logger, URL string) (*V2Descriptor, error) { - return httpclientx.GetJSON[*V2Descriptor](ctx, URL, &httpclientx.Config{ - Authorization: "", // not needed - Client: client, + template := httpx.APIClientTemplate{ + Accept: "", + Authorization: "", + BaseURL: URL, + HTTPClient: client, + Host: "", + LogBody: true, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, - }) + } + var desc V2Descriptor + if err := template.Build().GetJSON(ctx, "", &desc); err != nil { + return nil, err + } + return &desc, nil } // v2DescriptorCache contains all the known v2Descriptor entries. diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index 8f2260f680..9314287370 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" @@ -201,7 +200,8 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) { r := NewLinkRunner(config, server.URL) // make sure we correctly handled an invalid "null" descriptor - if err := r.Run(ctx); !errors.Is(err, httpclientx.ErrIsNil) { + if err := r.Run(ctx); err != nil { + t.Fatal(err) t.Fatal("unexpected error", err) } } diff --git a/internal/probeservices/bouncer.go b/internal/probeservices/bouncer.go index 4320f83efe..7ddc3fd941 100644 --- a/internal/probeservices/bouncer.go +++ b/internal/probeservices/bouncer.go @@ -1,29 +1,14 @@ package probeservices -// -// bouncer.go - GET /api/v1/test-helpers -// - import ( "context" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/urlx" ) -// GetTestHelpers queries the /api/v1/test-helpers API. -func (c *Client) GetTestHelpers(ctx context.Context) (map[string][]model.OOAPIService, error) { - // construct the URL to use - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-helpers", "") - if err != nil { - return nil, err - } - - // get the response - return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL, &httpclientx.Config{ - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }) +// GetTestHelpers is like GetCollectors but for test helpers. +func (c Client) GetTestHelpers( + ctx context.Context) (output map[string][]model.OOAPIService, err error) { + err = c.APIClientTemplate.WithBodyLogging().Build().GetJSON(ctx, "/api/v1/test-helpers", &output) + return } diff --git a/internal/probeservices/collector.go b/internal/probeservices/collector.go index a566eaef3b..df25570fec 100644 --- a/internal/probeservices/collector.go +++ b/internal/probeservices/collector.go @@ -8,9 +8,7 @@ import ( "reflect" "sync" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/urlx" ) var ( @@ -60,24 +58,10 @@ func (c Client) OpenReport(ctx context.Context, rt model.OOAPIReportTemplate) (R if rt.Format != model.OOAPIReportDefaultFormat { return nil, ErrUnsupportedFormat } - - URL, err := urlx.ResolveReference(c.BaseURL, "/report", "") - if err != nil { + var cor model.OOAPICollectorOpenResponse + if err := c.APIClientTemplate.WithBodyLogging().Build().PostJSON(ctx, "/report", rt, &cor); err != nil { return nil, err } - - cor, err := httpclientx.PostJSON[model.OOAPIReportTemplate, model.OOAPICollectorOpenResponse]( - ctx, URL, rt, &httpclientx.Config{ - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }, - ) - - if err != nil { - return nil, err - } - for _, format := range cor.SupportedFormats { if format == "json" { return &reportChan{ID: cor.ReportID, client: c, tmpl: rt}, nil @@ -99,27 +83,14 @@ func (r reportChan) CanSubmit(m *model.Measurement) bool { // submitted. Otherwise, we'll set the report ID to the empty // string, so that you know which measurements weren't submitted. func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) error { + var updateResponse model.OOAPICollectorUpdateResponse m.ReportID = r.ID - - URL, err := urlx.ResolveReference(r.client.BaseURL, fmt.Sprintf("/report/%s", r.ID), "") - if err != nil { - return err - } - - apiReq := model.OOAPICollectorUpdateRequest{ - Format: "json", - Content: m, - } - - updateResponse, err := httpclientx.PostJSON[ - model.OOAPICollectorUpdateRequest, model.OOAPICollectorUpdateResponse]( - ctx, URL, apiReq, &httpclientx.Config{ - Client: r.client.HTTPClient, - Logger: r.client.Logger, - UserAgent: r.client.UserAgent, - }, + err := r.client.APIClientTemplate.WithBodyLogging().Build().PostJSON( + ctx, fmt.Sprintf("/report/%s", r.ID), model.OOAPICollectorUpdateRequest{ + Format: "json", + Content: m, + }, &updateResponse, ) - if err != nil { m.ReportID = "" return err diff --git a/internal/probeservices/login.go b/internal/probeservices/login.go index 460b602589..d6bd4c1c93 100644 --- a/internal/probeservices/login.go +++ b/internal/probeservices/login.go @@ -3,9 +3,7 @@ package probeservices import ( "context" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/urlx" ) // MaybeLogin performs login if necessary @@ -19,24 +17,11 @@ func (c Client) MaybeLogin(ctx context.Context) error { return ErrNotRegistered } c.LoginCalls.Add(1) - - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/login", "") - if err != nil { - return err - } - - auth, err := httpclientx.PostJSON[*model.OOAPILoginCredentials, model.OOAPILoginAuth]( - ctx, URL, creds, &httpclientx.Config{ - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }, - ) - - if err != nil { + var auth model.OOAPILoginAuth + if err := c.APIClientTemplate.Build().PostJSON( + ctx, "/api/v1/login", *creds, &auth); err != nil { return err } - state.Expire = auth.Expire state.Token = auth.Token return c.StateFile.Set(state) diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go index a2646657e1..46c00fdaf6 100644 --- a/internal/probeservices/measurementmeta.go +++ b/internal/probeservices/measurementmeta.go @@ -1,22 +1,16 @@ package probeservices -// -// measurementmeta.go - GET /api/v1/measurement_meta -// - import ( "context" "net/url" - "github.com/ooni/probe-cli/v3/internal/httpclientx" + "github.com/ooni/probe-cli/v3/internal/httpx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/urlx" ) // GetMeasurementMeta returns meta information about a measurement. func (c Client) GetMeasurementMeta( ctx context.Context, config model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) { - // construct the query to use query := url.Values{} query.Add("report_id", config.ReportID) if config.Input != "" { @@ -25,17 +19,15 @@ func (c Client) GetMeasurementMeta( if config.Full { query.Add("full", "true") } - - // construct the URL to use - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/measurement_meta", query.Encode()) + var response model.OOAPIMeasurementMeta + err := (&httpx.APIClientTemplate{ + BaseURL: c.BaseURL, + HTTPClient: c.HTTPClient, + Logger: c.Logger, + UserAgent: c.UserAgent, + }).WithBodyLogging().Build().GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response) if err != nil { return nil, err } - - // get the response - return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL, &httpclientx.Config{ - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }) + return &response, nil } diff --git a/internal/probeservices/psiphon.go b/internal/probeservices/psiphon.go index 41ec925f14..a01421d051 100644 --- a/internal/probeservices/psiphon.go +++ b/internal/probeservices/psiphon.go @@ -3,33 +3,15 @@ package probeservices import ( "context" "fmt" - - "github.com/ooni/probe-cli/v3/internal/httpclientx" - "github.com/ooni/probe-cli/v3/internal/urlx" ) // FetchPsiphonConfig fetches psiphon config from authenticated OONI orchestra. func (c Client) FetchPsiphonConfig(ctx context.Context) ([]byte, error) { - // get credentials and authentication token _, auth, err := c.GetCredsAndAuth() if err != nil { return nil, err } - - // format Authorization header value s := fmt.Sprintf("Bearer %s", auth.Token) - - // construct the URL to use - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/psiphon-config", "") - if err != nil { - return nil, err - } - - // get response - return httpclientx.GetRaw(ctx, URL, &httpclientx.Config{ - Authorization: s, - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }) + client := c.APIClientTemplate.BuildWithAuthorization(s) + return client.FetchResource(ctx, "/api/v1/test-list/psiphon-config") } diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go index 43dbf96a47..9c75c9558f 100644 --- a/internal/probeservices/register.go +++ b/internal/probeservices/register.go @@ -1,16 +1,10 @@ package probeservices -// -// register.go - POST /api/v1/register -// - import ( "context" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/randx" - "github.com/ooni/probe-cli/v3/internal/urlx" ) // MaybeRegister registers this client if not already registered @@ -30,24 +24,11 @@ func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMeta OOAPIProbeMetadata: metadata, Password: pwd, } - - // construct the URL to use - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/register", "") - if err != nil { - return err - } - - resp, err := httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( - ctx, URL, req, &httpclientx.Config{ - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }, - ) - if err != nil { + var resp model.OOAPIRegisterResponse + if err := c.APIClientTemplate.Build().PostJSON( + ctx, "/api/v1/register", req, &resp); err != nil { return err } - state.ClientID = resp.ClientID state.Password = pwd return c.StateFile.Set(state) diff --git a/internal/probeservices/tor.go b/internal/probeservices/tor.go index 5ce50e6962..8c5cef7395 100644 --- a/internal/probeservices/tor.go +++ b/internal/probeservices/tor.go @@ -5,37 +5,20 @@ import ( "fmt" "net/url" - "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/urlx" ) // FetchTorTargets returns the targets for the tor experiment. -func (c Client) FetchTorTargets(ctx context.Context, cc string) (map[string]model.OOAPITorTarget, error) { - // get credentials and authentication token +func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[string]model.OOAPITorTarget, err error) { _, auth, err := c.GetCredsAndAuth() if err != nil { return nil, err } - - // format Authorization header value s := fmt.Sprintf("Bearer %s", auth.Token) - - // create query string + client := c.APIClientTemplate.BuildWithAuthorization(s) query := url.Values{} query.Add("country_code", cc) - - // construct the URL to use - URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/tor-targets", query.Encode()) - if err != nil { - return nil, err - } - - // get response - return httpclientx.GetJSON[map[string]model.OOAPITorTarget](ctx, URL, &httpclientx.Config{ - Authorization: s, - Client: c.HTTPClient, - Logger: c.Logger, - UserAgent: c.UserAgent, - }) + err = client.GetJSONWithQuery( + ctx, "/api/v1/test-list/tor-targets", query, &result) + return }