Closed grantmcconnaughey closed 7 years ago
Hmm, not sure why this has the renamed setting commit in there since that has already been merged into develop
...
It looks good so far. @ebpetway and will still need to integrate it with the warrant.django.views
but I don't see anything else glaring at me. @ebpetway thoughts?
Tested and seems to work fine.
There's only one possible issue from the perspective of using the Cognito
object.
Example:
Say I'm using this in a Django app.
A user inputs their credentials into a login form.
That input is used to instantiate a Cognito
object, and then the authenticate()
method is used.
If the NEW_PASSWORD_REQUIRED
challenge comes up, then this call raises a key error when Cognito
tries to pull tokens from the response.
Two ways of handling this could be either to catch the exception, and then call the new_password_challenge method
. Or call the admin_get_user
boto3 client method and check for a'FORCE_CHANGE_PASSWORD
status on the user, so you would know in advance to use the new_password_challenge
method.
With that said, I'm ok with getting this initial functionality in.
That's a really good point, and I'm working on this in a Django app so it is something to consider for sure.
What would you think about raising the challenges as exceptions? Like raise a ForceChangePasswordException
if that challenge comes up. Then users can catch that exception and handle redirecting the user to a change password page instead.
@ebpetway Are you talking about calling admin_get_user before the authenticate?
Okay, I've changed the flow a bit and I think it's better now. Now users will handle the flow like this:
cog = Cognito()
try:
cog.authenticate('oldpassword')
except ForceChangePasswordException:
cog.new_password_challenge('oldpassword', 'newpassword')
Also, @bjinwright, if I need to do something to cleanup the commits on this PR then I can. Not sure why it says the PR has so many commits. 😕
That's because we had some merge conflicts that I cleaned up yesterday
@grantmcconnaughey @ebpetway Here is the workflow I think we should go with.
admin_get_user
before attempting to authenticate and based on the users status we either call authenticate
because the user's status is CONFIRMED or return a redirect response based on the user's status.Makes sense to me! The only downside I can think of there is that most of the time unnecessary calls to admin_get_user
will occur because (in theory) most of the time they are confirmed users attempting to login.
With the exception-based approach most of the time the authenticate
method is the only thing that will be called. The Challenges would be raised with what they are -- exceptions to the normal flow.
I decided to merge this as is
Cool, thanks Brian!
This PR adds a method to respond to the new password challenge. It is mostly based off of @stephenoneal's work at https://github.com/stephenoneal/warrant.
Fixes #22