[fix] Fixed FreeRADIUS PostAuthView returning 500 error #513
Conversation
Set the max_length attribute to be equal to 50 on the called_station_id field, so that it doesn't give a 500 error(400 error instead) every time it exceeds 50. Fixes openwisp#467
the test shall be added in |
There was a problem hiding this comment.
@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
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.
Signed-off-by: Shiva953 <b22070@students.iitmandi.ac.in>
thanks for the clarification, just added the required failing test. |
Signed-off-by: Shiva953 <b22070@students.iitmandi.ac.in>
There was a problem hiding this comment.
@Shiva953 don't forget to run ./run-qa-checks locally before pushing, the build will fail because of QA checks.
| reverse('radius:postauth'), payload, HTTP_AUTHORIZATION=self.auth_header | ||
| ) | ||
| self.assertEqual(RadiusPostAuth.objects.all().count(), 0) | ||
| self.assertEqual(response.status_code, 400) |
There was a problem hiding this comment.
Have you double checked that this test fails if the fix is removed?
@nemesifier yes, I have confirmed the test by running 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): |
|
@Shiva953 in the second output, you ran the QA checks script and not the Django test suite. To run the complete test suite use QA checks are for the formatting of the code, Django tests for the functionality of the code. |
There was a problem hiding this comment.
@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.
The QA check was failing earlier because the latest commit wasn't following the conventional commit guidelines, this commit fixes it. Ran both the openwisp-qa-format and openwisp-qa-check commands. Fixes openwisp#467
Apologies for the oversight. I ran the |
There was a problem hiding this comment.
@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.
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 attributeto be 50 on thecalled_station_idfield, such that it returns HTTP Status Code 400 oncalled_station_idexceeding 50.Fixes #467