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

SignInManager Checks IdentityOptions in AccountController Login #19745

Conversation

alihdev
Copy link
Contributor

@alihdev alihdev commented May 8, 2024

Hi,
we need to use settings to set "IdentitySettingNames.SignIn.RequireConfirmedEmail" to true.
these are our settings:

context.Add(new SettingDefinition(
    name: IdentitySettingNames.SignIn.RequireConfirmedEmail,
    defaultValue: true.ToString(),
    isVisibleToClients: true
));

If the user tries to login and the email is not confirmed, the result should be failed.

  • Everything is good in the admin (/Account/Login) view. If the user is not confirmed, give me the message "You are not allowed to login! Your account is inactive or needs to confirm your email/phone number."
    image

  • The issue is in Swagger (/api/account/login) endpoint. If the user is not confirmed, the login succeeds!
    image

What we found is that the issue lies in the AccountController.Login where the SignInManager does not read the IdentityOptions. we should add code to read the IdentityOptions in SignInManager:

await IdentityOptions.SetAsync();

Please check the pull request to see if there is an issue or if we need to follow a step to bypass this issue.

@CLAassistant
Copy link

CLAassistant commented May 8, 2024

CLA assistant check
All committers have signed the CLA.

@maliming maliming added this to the 8.3-preview milestone May 9, 2024
@maliming maliming merged commit b528a51 into abpframework:dev May 9, 2024
2 checks passed
@maliming
Copy link
Member

maliming commented May 9, 2024

Thanks @alihdev

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