search

pallets-eco / flask-security

Quick and simple security for Flask applications
MIT License
1.63k stars 513 forks source link

Add Two Factor Authentication #842

Open TillerBurr opened 5 years ago

TillerBurr commented 5 years ago

Rebased https://github.com/mattupstate/flask-security/pull/773 and https://github.com/mattupstate/flask-security/pull/522 This PR solves: https://github.com/mattupstate/flask-security/issues/496

Much of the CI Failures were due to formatting issues. Also contains the following:

ghost commented 5 years ago

Thanks for this awesome PR.

Do you consider making this pull to the Flask-Security-Too fork at https://github.com/jwag956/flask-security?

The fork is more active. See #822.

TillerBurr commented 5 years ago

The main reason that I did not make a pull request to https://github.com/jwag956/flask-security was that in https://github.com/mattupstate/flask-security/issues/822 @lnielsen was added as an admin/maintainter on the various sites. I can definitely make a pull request to Flask-Security-Too.

jwag956 commented 5 years ago

I would be happy to help get your change into flask-security-too. I have been trying to work with current flask-security maintainers but so far to no avail - so I have decided at least for now to go off and formalize my fork. I am working on a 3.2.0 release that will contain a few feature improvements - in particular support for Single-Page-Applications (more complete json support) as well as documentation improvements, and a contributed authentication token cache. Adding 2-factor to 3.2.0 should be doable and I hope to get that release out in the next couple weeks.

Please be aware that on my fork - I need PRs against MASTER (not develop). Hopefully most of non-feature related code changes won't be necessary.

I have a rc1 for 3.2.0 on pypi:

https://pypi.org/project/Flask-Security-Too/#history

which currently is based off of master branch.

TillerBurr commented 5 years ago

Thank you for your help. This is not my initial code and I am kind of jumping into it without understanding what the original author was trying to do.

jwag956 commented 5 years ago

Let me know when you believe you are done - and I will cherry-pick that over to my fork. It will be a bit of a mess due to all the non-2FA changes - but shouldn't be too hard. I assume you have an app that actually uses this so you can test that it actually works?

TillerBurr commented 5 years ago

@jwag956 I've got it passing all of the CI tests and I've tested it in an app that isn't a SPA. The force push from earlier was just to squash a bunch of smaller commits that I was working through with the CI.

I was trying to locally merge your fork with my current branch, but at this point there is too much of a difference to try to merge without stepping on your toes. If this gets merged into your fork, I will try to keep an eye on it. There is much that can be improved on the backend, e.g. hashing the totp_secret, not storing it on the user model etc. but I think this is a good start.

jwag956 commented 5 years ago

That was painful - doing a rebase rather than cherry-pick - in any case - I have a PR up - made minor changes, and had to fix some doc stuff since my CI actually verifies that the docs build!

In my PR I added a bunch of questions that I could use your help on. I am mostly concerned about things that if we change later would not be backwards compatible.

Also - please look at: https://coveralls.io/builds/23685029

looks like we need some more unit tests!

TillerBurr commented 5 years ago

I can do this on your fork as well. As far as syntax checking, I am currently using vscode and it sometimes throws a bunch of unresolved imports that aren't true. I try to catch them, but apparently I seem to enjoy missing them.

jwag956 commented 5 years ago

Thanks - that would be great. I use Pycharm and it also sometimes makes mistakes - but if you run the code - doesn't it work/throw an error? I guess I am just asking why commit/push just to check if it runs?

I both run from PyCharm - but also, to check pep8 etc - I run python setup.py test before committing...

let me know if you run into any issues on my fork. Just work on the same branch (baurt) - I think it will let you - I just added you as a collaborator.

On Thu, May 30, 2019 at 7:10 AM Tyler Baur notifications@github.com wrote:

I can do this on your fork as well. As far as syntax checking, I am currently using vscode and it sometimes throws a bunch of unresolved imports that aren't true. I try to catch them, but apparently I seem to enjoy missing them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mattupstate/flask-security/pull/842?email_source=notifications&email_token=AAHU2T6OROK777ETCC7VHZDPX7N6DA5CNFSM4HODXFR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWSNWEY#issuecomment-497343251, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHU2T5P6RQP7IHIVJVKL2DPX7N6DANCNFSM4HODXFRQ .

--

Chris Wagner

The land embracing Point Lobos was bought for inclusion in the State Park System (a) because the very peculiar physical characteristics of the locality have enabled many people to obtain here personal satisfactions of peculiar kinds which they have valued very highly, and of which the most highly valued are unobtainable elsewhere in like degree if at all, ...

     Point Lobos Reserve Master Plan Report - Olmsted Brothers,

November, 1935

posix4e commented 4 years ago

what's going on here?

TillerBurr commented 4 years ago

Project is pretty much abandoned. An updated fork is here: https://github.com/jwag956/flask-security. It contains the above pull request and many other improvements.

katesinclair91 commented 3 years ago

Project is pretty much abandoned. An updated fork is here: https://github.com/jwag956/flask-security. It contains the above pull request and many other improvements.

Luckily. i read the pull requests and found this comment. Else i will be stuck on old. :D