[fix] Fixed FreeRADIUS PostAuthView returning 500 error

[Shiva953]

Addresses the issue of PostAuthView returning HTTP status code 500, and sets a restriction on the server side so that it gives a client side error(400) instead.

Updates the PostAuthSerializer and sets the max_length attribute to be 50 on the called_station_id field, such that it returns HTTP Status Code 400 on called_station_id exceeding 50.

Fixes #467

[nemesifier]

A failing test is missing here.

[Shiva953]

A failing test is missing here.

the test shall be added in test_freeradius_api.py, as a TestFreeradiusApi class method right?

[pandafy]

@Shiva953 yes, you are right! I will add the test after this test case

openwisp-radius/openwisp_radius/tests/test_api/test_freeradius_api.py

Lines 586 to 591 in 67fb112

	def test_postauth_400(self):
	response = self.client.post(
	reverse('radius:postauth'), {}, HTTP_AUTHORIZATION=self.auth_header
	)
	self.assertEqual(RadiusPostAuth.objects.all().count(), 0)
	self.assertEqual(response.status_code, 400)

I would name the test something like test_postauth_called_station_id_validation. The test should ensure that upon sending a request payload which has called_station_id with more than 50 characters, a HTTP 400 response should be returned.

[Shiva953]

@Shiva953 yes, you are right! I will add the test after this test case

openwisp-radius/openwisp_radius/tests/test_api/test_freeradius_api.py

Lines 586 to 591 in 67fb112

	def test_postauth_400(self):
	response = self.client.post(
	reverse('radius:postauth'), {}, HTTP_AUTHORIZATION=self.auth_header
	)
	self.assertEqual(RadiusPostAuth.objects.all().count(), 0)
	self.assertEqual(response.status_code, 400)

I would name the test something like test_postauth_called_station_id_validation. The test should ensure that upon sending a request payload which has called_station_id with more than 50 characters, a HTTP 400 response should be returned.

thanks for the clarification, just added the required failing test.

[nemesifier]

@Shiva953 don't forget to run ./run-qa-checks locally before pushing, the build will fail because of QA checks.

[nemesifier]

Have you double checked that this test fails if the fix is removed?

[Shiva953]

@Shiva953 don't forget to run ./run-qa-checks locally before pushing, the build will fail because of QA checks.

@nemesifier yes, I have confirmed the test by running ./run-qa-checks locally:

Also, I double checked the test and re-ran it after removing the fix, and the test case failed(which is indeed the desired scenario):

[pandafy]

@Shiva953 in the second output, you ran the QA checks script and not the Django test suite. To run the complete test suite use ./runtests.py.

QA checks are for the formatting of the code, Django tests for the functionality of the code.

[nemesifier]

@Shiva953 the build is failing because of QA checks. You need to run:

openwisp-qa-check

Then run:

./run-qa-checks

If the exit code (echo $? in a linux shell) of the last command is not zero and there's any error output, don't push, fix it first.

Please read the build output, it also provides a link to our guidelines which explain all these aspects in detail, please read that page carefully.

[Shiva953]

@Shiva953 the build is failing because of QA checks. You need to run:

openwisp-qa-check

Then run:

./run-qa-checks

If the exit code (echo $? in a linux shell) of the last command is not zero and there's any error output, don't push, fix it first.

Please read the build output, it also provides a link to our guidelines which explain all these aspects in detail, please read that page carefully.

Apologies for the oversight. I ran the openwisp-qa-format command check now and fixed the formatting/linting errors. After that, ran openwisp-qa-check and found out the build was failing due to formatting errors and also the commit not following the conventional guidelines, both of which are successful now.

[coveralls]

coverage: 98.723%. remained the same
when pulling db05e03 on Shiva953:issues/467-postauthview-500-error
into 67fb112 on openwisp:master.

[nemesifier]

@Shiva953 I ran the tests without the fix and the test passes, the response from the API is:

{'username': ['This field is required.'], 'reply': ['This field is required.']}

Please make sure the test replicates the bug first, and make sure the assertions check different aspects of the expected results to ensure the result is really what we expect.

Here you're checking the status code as 400, which means error on the client side, but you're not checking for specific aspects of the error response, so any error could make the test pass, which generates a false sense of security.