fix: Internal Server Error on Empty PUT /identities/id body

[VeenaInd]

Signed-off-by: Veena Saini veena.code.assignment@gmail.com

Description of the changes

This PR fixes the issue #2307.

Currently if a user sends HTTP PUT request with empty json data or incorrect json path (means the file does not exist), then that results into "internal server error". This error message is a generic and creates confusion.

Empty json (or we can say http request without body) triggers io.EOF error. This PR provides the user a better error message when io.EOF error is encountered.

Affected Module: Identity Update

There can be other scenarios of malformed user requests that may trigger "internal server error" during identity update, but those are not handled in this PR.

Related issue(s)

Checklist

• [x ] I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• [x ] I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[CLAassistant]

All committers have signed the CLA.

[VeenaInd]

Hi @zepatrik and @Benehiko ,

Just a quick check here. I was trying to reproduce the issue and got "ERR_DNS_FAIL" when attempting to connect to http://kratos:4434/identities . Also, if instead I do curl to http://127.0.0.1:4434/identities/ then that attempt results into "Temporary Redirect".

So, before proceeding wanted to confirm if there are any proxy settings in the source code that needs to be taken care.

Thanks,
Veena

[VeenaInd]

Also, I started with the quickstart and I am able to follow along with the demo. Only issue is when trying to reach to http://kratos:4434/identities. Please do not review the fix now, I created the PR with minimal changes to clarify some doubts.

[zepatrik]

I was trying to reproduce the issue and got "ERR_DNS_FAIL" when attempting to connect to http://kratos:4434/identities . Also, if instead I do curl to http://127.0.0.1:4434/identities/ then that attempt results into "Temporary Redirect".

This depends a bit on your test setup. If you run everything using docker-compose on localhost, you want to use http://127.0.0.1:4434. The OP used this setup:

I execed into the Postgres image, installed curl, ad called curl -s http://kratos:4434/identities/c8d98079-c6c0-49e1-a034-92fa2269724b

With docker networking that will be resolved properly, but you can also just use the loopback interface from outside of docker.

Now the redirect is happening because we recently added the path prefix /admin for admin endpoints. Therefore, you can test your fix using curl -v -X PUT http://127.0.0.1:4434/admin/identities/c8d98079-c6c0-49e1-a034-92fa2269724b -H "Accept: application/json".

In the end, it would be great to also have a go unit test for this case. There are already other tests where you can just add one more subtest, without the need to setup everything.

[Benehiko]

Hi @VeenaInd

Thank you for taking the time to work on this :)

You don't need to compile and run Kratos with the whole configuration setup manually to test this. I would suggest adding a test case to the identity/handler_test.go file and then running that.

You can find an example of such a test here

kratos/identity/handler_test.go

Lines 428 to 459 in 562e340

	t.Run("case=should update an identity and persist the changes", func(t *testing.T) {
	i := &identity.Identity{Traits: identity.Traits(fmt.Sprintf(`{"subject":"%s"}`, x.NewUUID().String()))}
	require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i))
	for name, ts := range map[string]*httptest.Server{"public": publicTS, "admin": adminTS} {
	t.Run("endpoint="+name, func(t *testing.T) {
	ur := identity.AdminUpdateIdentityBody{
	Traits: []byte(`{"bar":"baz","foo":"baz"}`),
	SchemaID: i.SchemaID,
	State: identity.StateInactive,
	MetadataPublic: []byte(`{"public":"metadata"}`),
	MetadataAdmin: []byte(`{"admin":"metadata"}`),
	}
	res := send(t, ts, "PUT", "/identities/"+i.ID.String(), http.StatusOK, &ur)
	assert.EqualValues(t, "baz", res.Get("traits.bar").String(), "%s", res.Raw)
	assert.EqualValues(t, "baz", res.Get("traits.foo").String(), "%s", res.Raw)
	assert.EqualValues(t, "metadata", res.Get("metadata_admin.admin").String(), "%s", res.Raw)
	assert.EqualValues(t, "metadata", res.Get("metadata_public.public").String(), "%s", res.Raw)
	assert.EqualValues(t, identity.StateInactive, res.Get("state").String(), "%s", res.Raw)
	assert.NotEqualValues(t, i.StateChangedAt, sqlxx.NullTime(res.Get("state_changed_at").Time()), "%s", res.Raw)
	res = get(t, ts, "/identities/"+i.ID.String(), http.StatusOK)
	assert.EqualValues(t, i.ID.String(), res.Get("id").String(), "%s", res.Raw)
	assert.EqualValues(t, "baz", res.Get("traits.bar").String(), "%s", res.Raw)
	assert.EqualValues(t, "metadata", res.Get("metadata_admin.admin").String(), "%s", res.Raw)
	assert.EqualValues(t, "metadata", res.Get("metadata_public.public").String(), "%s", res.Raw)
	assert.EqualValues(t, identity.StateInactive, res.Get("state").String(), "%s", res.Raw)
	assert.NotEqualValues(t, i.StateChangedAt, sqlxx.NullTime(res.Get("state_changed_at").Time()), "%s", res.Raw)
	})
	}
	})

Also please sign the CLA agreement so that the CI can run the tests.
#2417 (comment)

[VeenaInd]

Thanks @zepatrik and @Benehiko for this guidance.

curl -v -X PUT http://127.0.0.1:4434/admin/identities/c8d98079-c6c0-49e1-a034-92fa2269724b -H "Accept: application/json". is good one to replicate the scenario. I am able to get the exact error with this.

[Benehiko]

This is looking good, we would just need some handler tests to verify the fix

[Benehiko]

Also please rebase with origin/master as it seems this branch is out of date.

[VeenaInd]

Thanks @zepatrik and @Benehiko for your feedback. I have pushed working code and corresponding unit test. If I can think of a better code, I will push by tomorrow morning.

[Benehiko]

Hi @VeenaInd

Thank you for working on this, it's looking great so far :)

There are just a few minor changes that need to be made. I also see the CI doesn't run all the tests due to linting problems, you can resolve this by doing make format in the root of the Kratos project. It uses goimports and npm so you would need to install that for this to work.

kratos/Makefile

Lines 134 to 138 in a42a0f7

	# Formats the code
	.PHONY: format
	format: .bin/goimports node_modules
	goimports -w -local github.com/ory .
	npm run format

[zepatrik]

Thank you, this looks very good now from my side 👍

[Benehiko]

Good job! LGTM :)

[VeenaInd]

Thanks @zepatrik and @Benehiko for your feedback. Though I did "make format", before pushing the code, but still some test cases are failing here at CI pipeline. Anything I can do to make those test cases pass ?

[Benehiko]

Thanks @zepatrik and @Benehiko for your feedback. Though I did "make format", before pushing the code, but still some test cases are failing here at CI pipeline. Anything I can do to make those test cases pass ?

I think github packages are down at the moment which the docker image scanner relies on afaik.

The other failures might be flakes https://github.com/ory/kratos/runs/6154892204?check_suite_focus=true#step:16:1398

We can also just wait a bit until Github is fully operational and re-run everything later.

[VeenaInd]

Thanks @zepatrik and @Benehiko for your feedback. Though I did "make format", before pushing the code, but still some test cases are failing here at CI pipeline. Anything I can do to make those test cases pass ?

I think github packages are down at the moment which the docker image scanner relies on afaik.

The other failures might be flakes https://github.com/ory/kratos/runs/6154892204?check_suite_focus=true#step:16:1398

We can also just wait a bit until Github is fully operational and re-run everything later.

ok, thank you.

[vinckr]

Hello @VeenaInd
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

[VeenaInd]

Hello @VeenaInd Congrats on merging your first PR in Ory 🎉 ! Your contribution will soon be helping secure millions of identities around the globe 🌏. As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community. Please drop me an email and I will forward you the form to claim your Ory swag!

Thank you @vinckr. I am glad, I got this opportunity to work on ory project :) .