Ignoring OPTIONS method in jwt_required (fix #119) #120

ludusrusso · 2018-02-09T00:59:25Z

This shoud fix #119!

All test passes!

$ python -m pytest
=================================================== test session starts ===================================================
platform darwin -- Python 3.6.1, pytest-3.4.0, py-1.5.2, pluggy-0.6.0
rootdir: /Users/ludus/develop/playground/flask-jwt-extended, inifile:
collected 91 items

tests/test_asymmetric_crypto.py .                                                                                   [  1%]
tests/test_blacklist.py ...........                                                                                 [ 13%]
tests/test_claims_verification.py ............                                                                      [ 26%]
tests/test_config.py ..........                                                                                     [ 37%]
tests/test_cookies.py ..................                                                                            [ 57%]
tests/test_decode_tokens.py ............                                                                            [ 70%]
tests/test_headers.py ...                                                                                           [ 73%]
tests/test_headers_and_cookies.py ...                                                                               [ 76%]
tests/test_options_method.py ..                                                                                     [ 79%]
tests/test_user_claims_loader.py ....                                                                               [ 83%]
tests/test_user_loader.py ......                                                                                    [ 90%]
tests/test_view_decorators.py .........                                                                             [100%]

coveralls · 2018-02-09T01:01:03Z

Coverage remained the same at 100.0% when pulling ea5fa12 on ludusrusso:master into 32a6604 on vimalloc:master.

coveralls · 2018-02-09T01:01:03Z

Coverage remained the same at 100.0% when pulling 4ad8d64 on ludusrusso:master into 32a6604 on vimalloc:master.

vimalloc

Got some minor nit-picks here, but all in all this looks great! Most of these relate to the fact that flask allows you to create your own OPTIONS method, and even though I'm sure no one actually does that, because it is something supported by flask I want to make sure this extension doesn't break that.

Thanks for contributing! 👍

vimalloc · 2018-02-09T01:24:52Z

flask_jwt_extended/view_decorators.py

+            if not verify_token_claims(jwt_data[config.user_claims_key]):
+                raise UserClaimsVerificationError('User claims verification failed')
+            _load_user(jwt_data[config.identity_claim_key])
+            return fn(*args, **kwargs)


If we don't return fn(*args, **kwargs) in an else block here, this will break if someone is creating their own OPTIONS endpoint via someting like @app.route('/foo', methods=['GET', 'OPTIONS']). I doubt anyone is doing that, but because flask allows this to happen, I would rather get that fixed up anyways, just in case :)

You're right! I didn't mean to indent also return fn(*args, **kwargs)!

vimalloc · 2018-02-09T01:25:33Z

flask_jwt_extended/view_decorators.py

-            raise UserClaimsVerificationError('User claims verification failed')
-        _load_user(jwt_data[config.identity_claim_key])
-        return fn(*args, **kwargs)
+        if request.method not in config.exempt_methods:


Could we add this same logic to the other view decorators in this file as well, for completeness sake? I'm alright with having the unit tests be only for the @jwt_required decorator (unless you feel like adding unit tests for the other decorators as well, in which case 👍 👍)

Yea! I'll add it also in fresh_jwt_required and jwt_refresh_token_required! I'm not sure if we need to implement it in jwt_optional, since it should works well!

I'll wrote more tests anyway :D

vimalloc · 2018-02-09T01:26:34Z

tests/test_options_method.py

+    app.config['JWT_SECRET_KEY'] = 'secret'
+    JWTManager(app)
+
+    protected_bp = Blueprint('protected', __name__)


I don't think you need to check this with a blueprint here. Testing it with just an app should be sufficient and a bit cleaner to read.

vimalloc · 2018-02-09T01:29:51Z

tests/test_options_method.py

+
+def test_access_protected_enpoint_options(app):
+    client = app.test_client()
+    assert client.options('/protected').status_code == 200 # test fails


Could we add one more test here for creating a custom OPTIONS endpiont, and verifying that the data returned is the returned value of that endpoint? Not likely that this is happening in real codebases, but I want to make sure we don't break flask functionality with this extension, and that is something flask allows you to do.

vimalloc · 2018-02-09T01:40:01Z

flask_jwt_extended/config.py

-
+    @property
+    def exempt_methods(self):
+        return set(["OPTIONS"])


We don't need to support python2.6, so you could write this as the set literal (ex: {'foo'}) instead.

`jwt_refresh_token_required`, improve and add tests

ludusrusso · 2018-02-09T09:15:08Z

Ok! I've improved the code according to the review!
I've also rewrite tests in order to only check the options ignoring feature, since protecting enpoints are already tested.

vimalloc · 2018-02-09T16:20:01Z

Great, thanks! I'll get a new released pushed out with these changes later today 👍

ludusrusso added 2 commits February 9, 2018 01:50

add test to test options method (vimalloc#119)

ecb18f7

fix issue vimalloc#119

ea5fa12

ludusrusso changed the title ~~Fix Issue #119 (ignoring OPTIONS method in jwt_required)~~ Ignoring OPTIONS method in jwt_required (fix #119) Feb 9, 2018

vimalloc requested changes Feb 9, 2018

View reviewed changes

vimalloc mentioned this pull request Feb 9, 2018

jwt_refresh_token_required on OPTION request #116

Closed

Ignore options also in fresh_jwt_required and

fe5e5bf

`jwt_refresh_token_required`, improve and add tests

this should solve Non-ASCII character '\xc2' error

4ad8d64

vimalloc approved these changes Feb 9, 2018

View reviewed changes

vimalloc merged commit c78d37a into vimalloc:master Feb 9, 2018

Uh oh!

Ignoring OPTIONS method in jwt_required (fix #119) #120

Ignoring OPTIONS method in jwt_required (fix #119) #120

Uh oh!

Conversation

ludusrusso commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vimalloc left a comment

Choose a reason for hiding this comment

Uh oh!

vimalloc Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

ludusrusso Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

vimalloc Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

ludusrusso Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimalloc Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

vimalloc Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

vimalloc Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

ludusrusso commented Feb 9, 2018

Uh oh!

vimalloc commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ludusrusso commented Feb 9, 2018 •

edited

Loading

coveralls commented Feb 9, 2018 •

edited

Loading

coveralls commented Feb 9, 2018 •

edited

Loading

ludusrusso Feb 9, 2018 •

edited

Loading