[management] improve zitadel idp error response detail by decoding errors

[adasauce]

Describe your changes

Adds a zitadelErrorResponse struct to decode errors returned from zitadel rather than just returning a status code, so more information can be learned about potential configuration issues. Helped to diagnose #2616

I have some other small changes and improvements to compatibility later, but this was a pretty clear cut and standalone improvement.

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)
• Extended the README / documentation, if necessary

[CLAassistant]

All committers have signed the CLA.

[bcmmbaga]

Could you also include handling for the Zitadel error when requesting JWT token?

netbird/management/server/idp/zitadel.go

Lines 178 to 180 in 765aba2

	if resp.StatusCode != http.StatusOK {
	return nil, fmt.Errorf("unable to get zitadel token, statusCode %d", resp.StatusCode)
	}

[adasauce]

Could you also include handling for the Zitadel error when requesting JWT token?

netbird/management/server/idp/zitadel.go

Lines 178 to 180 in 765aba2

	if resp.StatusCode != http.StatusOK {
	return nil, fmt.Errorf("unable to get zitadel token, statusCode %d", resp.StatusCode)
	}

yeah, i think since the only thing the method references internally is the helper, and they're two different structs it makes more sense to pull the parser out as a standalone utility function within the package and pass the bound helper of zm/zc + the io.readcloser of resp.Body.

I have the changes laid out like that locally here, but lmk if you'd like to see it go in that direction vs. something else.

[bcmmbaga]

Could you also include handling for the Zitadel error when requesting JWT token?

netbird/management/server/idp/zitadel.go

Lines 178 to 180 in 765aba2

	if resp.StatusCode != http.StatusOK {
	return nil, fmt.Errorf("unable to get zitadel token, statusCode %d", resp.StatusCode)
	}

yeah, i think since the only thing the method references internally is the helper, and they're two different structs it makes more sense to pull the parser out as a standalone utility function within the package and pass the bound helper of zm/zc + the io.readcloser of resp.Body.

I have the changes laid out like that locally here, but lmk if you'd like to see it go in that direction vs. something else.

Yes, you can extract the parser as a standalone function and initialize the helper within that function, as it's an instance of JsonParser

[adasauce]

ah, have some tests to clean up to expect a zitadel message in the output. sorry didnt check first.

[bcmmbaga]

I ran the tests and found an issue when the JWT request fails. It seems they're using a different error response format

2024-09-27T20:01:23+03:00 WARN [context: SYSTEM] management/server/account.go:1017: failed warming up cache due to error: unable to get zitadel token, statusCode 400, zitadel: error code: 0 message:

Error format:{"error":"invalid_request","error_description":"Errors.User.Machine.Secret.Invalid"}

To make it generic lets return the string(bodyBytes) as the new error. This way, we don't need to worry about the error structure.

// readZitadelError parses errors returned by the zitadel APIs from a response.
func readZitadelError(body io.ReadCloser) error {
	bodyBytes, err := io.ReadAll(body)
	if err != nil {
		return fmt.Errorf("failed to read response body: %w", err)
	}

	return errors.New(string(bodyBytes))
}

Also, the Zitadel tests are failing. Could you take a look, or would you like some help with that?

[adasauce]

yeah, I was just going to follow up to mention that it's a different error format with error and error_description fields with an actual failure. we could parse it, or just dump it as is.

I'm on board with just stringifying the body and returning that. the only downside I think is that with the other error response types you get extra data that isn't really important to the error, like encoded type info about the message field being a string.

[bcmmbaga]

I'm on board with just stringifying the body and returning that. the only downside I think is that with the other error response types you get extra data that isn't really important to the error, like encoded type info about the message field being a string.

If there's a possibility of extra fields in the error response (which I couldn't reproduce), we can extend the zitadelErrorResponse to include these new fields. When constructing the error message, we should ensure to omit any fields with empty values

[adasauce]

I'm on board with just stringifying the body and returning that. the only downside I think is that with the other error response types you get extra data that isn't really important to the error, like encoded type info about the message field being a string.

If there's a possibility of extra fields in the error response (which I couldn't reproduce), we can extend the zitadelErrorResponse to include these new fields. When constructing the error message, we should ensure to omit any fields with empty values

before pusing it up, how about this:

// readZitadelError parses errors returned by the zitadel APIs from a response.
func readZitadelError(body io.ReadCloser) error {
	bodyBytes, err := io.ReadAll(body)
	if err != nil {
		return fmt.Errorf("failed to read response body: %w", err)
	}

	helper := JsonParser{}
	var target map[string]interface{}
	err = helper.Unmarshal(bodyBytes, &target)
	if err != nil {
		return fmt.Errorf("error unparsable body: %s", string(bodyBytes))
	}

	// ensure keys are ordered for consistent logging behaviour.
	errorKeys := make([]string, 0, len(target))
	for k := range target {
		errorKeys = append(errorKeys, k)
	}
	slices.Sort(errorKeys)

	var errsOut []string
	for _, k := range errorKeys {
		if _, isEmbedded := target[k].(map[string]interface{}); isEmbedded {
			continue
		}
		errsOut = append(errsOut, fmt.Sprintf("[%v: %v]", k, target[k]))
	}

	if len(errsOut) == 0 {
		return fmt.Errorf("[data missing]")
	}

	return fmt.Errorf(strings.Join(errsOut, " "))
}

omits any extra embedded fields, gets rid of the extra data structures and returns the top level elements that we care about.

I updated the test cases like this to demonstrate:

	requestJWTTokenTestCase2 := requestJWTTokenTest{
		name:                    "Request Bad Status Code",
		inputCode:               400,
		inputRespBody:           "{\"error\": \"invalid_scope\", \"error_description\":\"openid missing\"}",
		helper:                  JsonParser{},
		expectedFuncExitErrDiff: fmt.Errorf("unable to get zitadel token, statusCode 400, zitadel: [error: invalid_scope] [error_description: openid missing]"),
		expectedToken:           "",
	}

[bcmmbaga]

This looks good. Just a minor change, remove the brackets around the error message and we’ll be good to go.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud