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

Add Code Password Reset to the Bootstrapper #343

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

eliasbiagioninc
Copy link
Contributor

@eliasbiagioninc eliasbiagioninc commented Aug 20, 2024

What this does

Switch from password reset email links to 7-digit codes for resetting passwords. Discussion Add Code Password Reset to the Bootstrapper

Checklist

  • Remove token management for reset password
  • Add models to store OTP codes
  • Update reset password endpoint to generate new codes
  • Update reset password confirmation to allow code, password and email
  • Add screens - Mobile
  • Add screens - Web
    • React
    • Vue
  • Tests

How to test

Add user steps to achieve desired functionality for this feature.

@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 20, 2024 19:56 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 20, 2024 20:38 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 12:30 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 12:36 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 12:44 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 12:59 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 13:08 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 13:23 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 13:28 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 13:32 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 21, 2024 13:57 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 30, 2024 12:11 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 30, 2024 19:44 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 August 30, 2024 20:07 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 September 2, 2024 20:29 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 September 2, 2024 20:35 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 September 2, 2024 20:40 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 November 29, 2024 20:55 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 November 29, 2024 21:12 Inactive
Copy link
Member

Choose a reason for hiding this comment

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

return (
<MultiPlatformSafeAreaView safeAreaClassName="h-full mt-5">
<View className="w-full content-center mx-auto py-10 bg-slate-200 rounded-lg items-center px-4">
<Text textClassName="text-black text-3xl" variant="bold">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Text textClassName="text-black text-3xl" variant="bold">
<Text className="text-primary-bold text-black text-3xl">

@@ -0,0 +1,64 @@
import { MultiPlatformSafeAreaView } from '@components/multi-platform-safe-area-view'
import { BounceableWind } from '@components/styled'
import { Text } from '@components/text'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { Text } from '@components/text'
import { Text } from 'react-native'

@@ -14,3 +14,30 @@ export const customFonts = {
[`${baseFamily}-MediumItalic` as const]: require(`../../assets/fonts/${baseFamily}-MediumItalic.${fontFormat}`),
[`${baseFamily}-Regular` as const]: require(`../../assets/fonts/${baseFamily}-Regular.${fontFormat}`),
}

Copy link
Member

Choose a reason for hiding this comment

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

We removed this in a previous PR.

@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 December 6, 2024 15:26 Inactive
@oudeismetis oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-343 December 6, 2024 18:18 Inactive

@property
def is_valid(self):
return not (self.is_used | (self.created > now() + timedelta(minutes=settings.RESET_PASSWORD_CODE_VALIDITY_MINUTES)))
Copy link
Member

Choose a reason for hiding this comment

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

if a code is used, wouldn't we just delete the model instance?

self.context.get("user")
.reset_password_codes.filter(created__gte=(timezone.now() - timedelta(minutes=settings.RESET_PASSWORD_CODE_VALIDITY_MINUTES)))
.first()
)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't they only ever have a single code at any moment? If it expires, then it fails validation.
If they request a new one, the old one should be deleted.

from .dispatchers import new_reset_password_code_created_ds

# Logger
logger = logging.getLogger(__name__)
Copy link
Member

Choose a reason for hiding this comment

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

we know it's a logger. No need for a code comment

@eliasbiagioninc eliasbiagioninc temporarily deployed to tn-spa-bootstrapper-pr-343 December 6, 2024 20:28 Inactive
email = kwargs.get("email")
user = User.objects.filter(email=email).first()
if not user:
raise ValidationError(detail={"non_field_errors": ["User not found with that email"]})
Copy link
Member

Choose a reason for hiding this comment

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

Security issue. Someone could use this to determine what users are in the DB.

I would just fail validation with a generic error. The error should be the same regardless of the reason (ex: bad code vs bad email)

@eliasbiagioninc eliasbiagioninc temporarily deployed to tn-spa-bootstrapper-pr-343 December 6, 2024 20:46 Inactive
@eliasbiagioninc eliasbiagioninc temporarily deployed to tn-spa-bootstrapper-pr-343 December 6, 2024 20:51 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants