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

Support new password challenge #25

Conversation

grantmcconnaughey
Copy link
Contributor

@grantmcconnaughey grantmcconnaughey commented Apr 20, 2017

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

@grantmcconnaughey
Copy link
Contributor Author

Hmm, not sure why this has the renamed setting commit in there since that has already been merged into develop...

@bjinwright
Copy link
Member

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?

@bjinwright bjinwright changed the base branch from develop to new_password_challenge April 20, 2017 17:26
@ebpetway
Copy link
Contributor

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.

@grantmcconnaughey
Copy link
Contributor Author

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.

@bjinwright
Copy link
Member

@ebpetway Are you talking about calling admin_get_user before the authenticate?

@grantmcconnaughey
Copy link
Contributor Author

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. 😕

@bjinwright
Copy link
Member

That's because we had some merge conflicts that I cleaned up yesterday

@bjinwright
Copy link
Member

@grantmcconnaughey @ebpetway Here is the workflow I think we should go with.

  1. User attempts to log in
  2. The view from the framework calls 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.

What's Needed to Make it Work

  1. Different methods for each scenario
  2. Different views for each user status scenario (UNCONFIRMED | CONFIRMED | ARCHIVED | COMPROMISED | UNKNOWN | RESET_REQUIRED | FORCE_CHANGE_PASSWORD)

@grantmcconnaughey
Copy link
Contributor Author

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.

@bjinwright bjinwright merged commit 5086b25 into capless:new_password_challenge Apr 21, 2017
@bjinwright
Copy link
Member

I decided to merge this as is

@grantmcconnaughey
Copy link
Contributor Author

Cool, thanks Brian!

macpijan added a commit to 3mdeb/warrant that referenced this pull request Sep 4, 2018
This problem was raised in a number of issues such as this one:
capless#29

The `set_new_password_challenge()` was introduced in the:
capless#25 and is not yet documented.

This should help the newcomers to get started with this module.

Signed-off-by: Maciej Pijanowski <maciej.pijanowski@3mdeb.com>
alastairmccormack pushed a commit to alastairmccormack/pycognito that referenced this pull request Feb 2, 2022
Bumps [coverage](https://github.com/nedbat/coveragepy) from 5.0.3 to 5.3.1.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@coverage-5.0.3...coverage-5.3.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

3 participants