Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default REST_FRAMEWORK permission classes could break api views (#499) #500

Closed

Conversation

bmihelac
Copy link
Contributor

@bmihelac bmihelac commented Jan 5, 2022

Fixes #499

Unfortunately, it seems there is no easy way to test this. DRF api_views decorator apply default permission classes early, and settings_override has no effect. I have added REST_FRAMEWORK settings that are known to break wagtail_localize in test app settings.

@zerolab
Copy link
Collaborator

zerolab commented Jan 5, 2022

@bmihelac according to https://github.com/encode/django-rest-framework/blob/master/tests/test_settings.py#L29-L44 you should be able to use override_settings

@codecov-commenter
Copy link

Codecov Report

Merging #500 (ebe4e91) into main (08272a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #500   +/-   ##
=======================================
  Coverage   92.92%   92.93%           
=======================================
  Files          36       36           
  Lines        3025     3029    +4     
  Branches      478      478           
=======================================
+ Hits         2811     2815    +4     
  Misses        114      114           
  Partials      100      100           
Impacted Files Coverage Δ
wagtail_localize/test/settings.py 100.00% <100.00%> (ø)
wagtail_localize/views/edit_translation.py 88.22% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08272a9...ebe4e91. Read the comment docs.

@bmihelac
Copy link
Contributor Author

bmihelac commented Jan 5, 2022

@zerolab I have tried and it does not work.

I have added test code in d6ec08b comiit.

It can be checked with

python testmanage.py test wagtail_localize.tests.test_edit_translation.TestEditOverrideAPIView.test_update_override_with_default_permissions_classes

api_settings.DEFAULT_PERMISSION_CLASSES is updated and it correctly shows [<class 'rest_framework.permissions.DjangoModelPermissionsOrAnonReadOnly'>] but view still have rest_framework.permissions.AllowAny. I guess that is because view is decorated early in:

https://github.com/encode/django-rest-framework/blob/a780e80debac0d12bb62fbb0ed98635e601e76de/rest_framework/decorators.py#L70-L71

@bmihelac
Copy link
Contributor Author

bmihelac commented Jan 7, 2022

@zerolab if there is any other idea how tests for this could be added, I would be happy to update this PR

@zerolab
Copy link
Collaborator

zerolab commented Jan 7, 2022

@bmihelac will have a look over the weekend and get back to you

@zerolab
Copy link
Collaborator

zerolab commented Jan 27, 2022

@bmihelac this is on my list for tomorrow. Sorry for the delay

@bmihelac
Copy link
Contributor Author

No problems @zerolab
iI you think there is anything that I could try let me know, I am out of ideas other than opening issue on DRF.

@zerolab
Copy link
Collaborator

zerolab commented Feb 1, 2022

@bmihelac opened #513 which fixes the tests as well as addresses a related issue (having to do with default permission classes not including SessionAuthentication)

Will close this. Thank you very much for your PR!

@zerolab zerolab closed this Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

edit_string_translation view should not use REST_FRAMEWORK default permissions and authentication classes
3 participants