Skip to content

Conversation

@dunkmann00
Copy link
Contributor

This pull request adds the ability to pass in a datetime.timedelta object for the fresh kwarg, instead of a boolean, when creating an access token. When using the fresh_jwt_required decorator, if the fresh value is old, the endpoint will be inaccessible. But the access token is still valid for endpoints that use jwt_required.

@dunkmann00
Copy link
Contributor Author

#107

@dunkmann00
Copy link
Contributor Author

Not sure whats going on here. Something related to cookies is failing and I didn't change anything with that. Also, when I clone the master branch and run tox without any changes to the project I also get these errors. Seems strange.

@dunkmann00
Copy link
Contributor Author

Still not sure why this is happening but after further examination it seems that the problem has to do with the parse_cookie function which is in _get_cookie_from_response function of the test_cookies.py file. After passing the cookie into this method it returns a dictionary that leaves off the 'HttpOnly; Path' value.

@vimalloc
Copy link
Owner

vimalloc commented Jan 1, 2018

Looks like changes in the newest werkzeug release: http://werkzeug.pocoo.org/docs/0.14/changes/#version-0-14

I'll get this updated in master soon (today or tomorrow hopefully), then you can rebase this pr and we can get it merged and push out a new release.

Cheers

vimalloc added a commit that referenced this pull request Jan 3, 2018
@vimalloc
Copy link
Owner

vimalloc commented Jan 3, 2018

Ok, assuming all goes well that fix should be in place. If you can rebase this, I would be happy to get it merged in. Cheers 👍

Copy link
Owner

@vimalloc vimalloc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test for successful case

with app.test_request_context():
access_token = create_access_token('username')
fresh_access_token = create_access_token('username', fresh=True)
fresh_timed_access_token = create_access_token('username', fresh=timedelta(minutes=-1))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add one more unit test somewhere? You have the not fresh test case here, I would like to have a test added that sets the time delta in the future and successfully accesses a fresh jwt required endpoint while using this new feature. Thanks!

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling f1b427f on dunkmann00:master into 92de911 on vimalloc:master.

dunkmann00 pushed a commit to dunkmann00/flask-jwt-extended that referenced this pull request Jan 6, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 47086e6 on dunkmann00:master into 92de911 on vimalloc:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 47086e6 on dunkmann00:master into 92de911 on vimalloc:master.

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling f29650f on dunkmann00:master into 92de911 on vimalloc:master.

@dunkmann00
Copy link
Contributor Author

Okay so I think I have my branch correct now. This is my first pull request and first time doing a rebase so it took me a little bit but I think its now in good order. I also added the unit test for the successful case and rebased that into my original commit. I hope that was okay to do. Let me know if there is anything else I need to do!

@vimalloc
Copy link
Owner

vimalloc commented Jan 7, 2018

Thanks for working on this!

The rebase against master looks good. I'm not seeing that success unit test in the changes for this pull request though. You can push that into this PR with a separate commit, that doesn't have to be rebased.

Besides that, everything looks great. Once you get that in this PR I'll get this merged.

Thanks for contributing! 👍

@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5947e47 on dunkmann00:master into 92de911 on vimalloc:master.

@dunkmann00
Copy link
Contributor Author

My mistake! Thought I had that in here already. Just added it now.

Thanks for having started this repo, this is an awesome extension for Flask!

@vimalloc vimalloc merged commit 6483592 into vimalloc:master Jan 8, 2018
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.

3 participants