-
Notifications
You must be signed in to change notification settings - Fork 318
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
Force New Password - Add Support For Required Attributes #1176
Conversation
🦋 Changeset detectedLatest commit: e9e17f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
{ | ||
"ChallengeName": "NEW_PASSWORD_REQUIRED", | ||
"ChallengeParameters": { | ||
"requiredAttributes": "[\"userAttributes.gender\",\"userAttributes.given_name\",\"userAttributes.nickname\",\"userAttributes.middle_name\",\"userAttributes.name\",\"userAttributes.zoneinfo\",\"userAttributes.profile\",\"userAttributes.family_name\",\"userAttributes.phone_number\",\"userAttributes.website\",\"userAttributes.preferred_username\",\"userAttributes.locale\",\"userAttributes.picture\",\"userAttributes.address\"]", | ||
"userAttributes": "{\"email_verified\":\"true\",\"phone_number_verified\":\"true\",\"phone_number\":\"+1••••••4444\",\"email\":\"••••••+FORCE_CHANGE_PASSWORD@••••••.com\"}" | ||
}, | ||
"Session": "••••••" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mocked out the response we see when a new password is required with attributes.
import { | ||
getActorContext, | ||
getActorState, | ||
SignInContext, | ||
SignInState, | ||
SignUpContext, | ||
translate, | ||
} from '@aws-amplify/ui'; | ||
|
||
import { useAuthenticator } from '..'; | ||
import { PhoneNumberField, TextField } from '../../..'; | ||
|
||
export function FormFields() { | ||
const { _state } = useAuthenticator(); | ||
const { country_code } = getActorContext(_state) as SignUpContext; | ||
|
||
const actorState = getActorState(_state) as SignInState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new components are modified heavily from their source Sign Up Form Fields
This pull request introduces 5 alerts when merging 6d14b7a into 3e88dfb - view on LGTM.com new alerts:
|
<ng-template amplifySlot="force-new-form-fields"> | ||
<amplify-force-new-password-form-fields></amplify-force-new-password-form-fields> | ||
<amplify-text-field | ||
label="Zone Info" | ||
id="12233" | ||
autocomplete="false" | ||
placeholder="Zone Info" | ||
name="zoneinfo" | ||
type="text" | ||
></amplify-text-field> | ||
</ng-template> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late review. QQ, is there a zero-config value we can use so that customer does not have to make this mapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a handful of attributes that we decided that we aren't going to have default mappings for because they are app specific, including zoneinfo. As mentioned here https://ui.docs.amplify.aws/components/authenticator#signupattributes
The zoneInfo
is in the zero config, you are right. We just don't have a mapping for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, I was lost yesterday xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, got two non-blocking questions
This pull request introduces 5 alerts when merging c289ea0 into 13e3637 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request introduces 5 alerts when merging e9e17f3 into 5f7969a - view on LGTM.com new alerts:
|
Issue #, if available:
#1155
Description of changes:
If an Admin creates a user manually, the first time a user logs in they are brought to the
Force New Password
page. In previous versions of the Authenticator this would display the password, and confirm password as well as any required attributes the user needed.Without these required attributes the sign in cannot continue and the user will see an error.
This PR adds in a check to see if the attributes are needed, and adds them.
We also will use the same logic we used in the sign up page. Only common attributes that are defined internally will display. If a user has a custom attribute, they can override with slots the
Force New Password Fields
to add it in.This has been added to the docs as well.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.