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

Revise how to change the password for external logins #4975

Merged
merged 5 commits into from Aug 10, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -25,33 +25,41 @@
<abp-card>
<abp-card-body>
<abp-tabs tab-style="PillVertical">
<abp-tab title="@L["ChangePassword"].Value">
<h4>@L["ChangePassword"].Value</h4><hr />
<abp-dynamic-form abp-model="@Model.ChangePasswordInfoModel" id="ChangePasswordForm">
<abp-form-content />
<abp-button type="submit" button-type="Primary" text="@L["Submit"].Value" />
</abp-dynamic-form>
</abp-tab>
@if (!Model.DisablePasswordChange)
{
<abp-tab title="@L["ChangePassword"].Value">
<h4>@L["ChangePassword"].Value</h4><hr/>
<form id="ChangePasswordForm">
@if (!Model.HideOldPasswordInput)
{
<abp-input asp-for="ChangePasswordInfoModel.CurrentPassword"/>
}
<abp-input asp-for="ChangePasswordInfoModel.NewPassword"/>
<abp-input asp-for="ChangePasswordInfoModel.NewPasswordConfirm"/>
<abp-button type="submit" button-type="Primary" text="@L["Submit"].Value"/>
</form>
</abp-tab>
}
<abp-tab title="@L["PersonalSettings"].Value">
<h4>@L["PersonalSettings"].Value</h4><hr />
<h4>@L["PersonalSettings"].Value</h4><hr/>
<form method="post" id="PersonalSettingsForm">

<abp-input asp-for="PersonalSettingsInfoModel.UserName" readonly="!isUserNameUpdateEnabled" />
<abp-input asp-for="PersonalSettingsInfoModel.UserName" readonly="!isUserNameUpdateEnabled"/>

<abp-row>
<abp-column size-md="_6">
<abp-input asp-for="PersonalSettingsInfoModel.Name" />
<abp-input asp-for="PersonalSettingsInfoModel.Name"/>
</abp-column>
<abp-column size-md="_6">
<abp-input asp-for="PersonalSettingsInfoModel.Surname" />
<abp-input asp-for="PersonalSettingsInfoModel.Surname"/>
</abp-column>
</abp-row>

<abp-input asp-for="PersonalSettingsInfoModel.Email" readonly="!isEmailUpdateEnabled" />
<abp-input asp-for="PersonalSettingsInfoModel.Email" readonly="!isEmailUpdateEnabled"/>

<abp-input asp-for="PersonalSettingsInfoModel.PhoneNumber" />
<abp-input asp-for="PersonalSettingsInfoModel.PhoneNumber"/>

<abp-button type="submit" button-type="Primary" text="@L["Submit"].Value" />
<abp-button type="submit" button-type="Primary" text="@L["Submit"].Value"/>
</form>
</abp-tab>
</abp-tabs>
Expand Down
Expand Up @@ -12,6 +12,10 @@ public class ManageModel : AccountPageModel

public PersonalSettingsInfoModel PersonalSettingsInfoModel { get; set; }

public bool DisablePasswordChange { get; set; }

public bool HideOldPasswordInput { get; set; }

protected IProfileAppService ProfileAppService { get; }

public ManageModel(IProfileAppService profileAppService)
Expand All @@ -25,6 +29,9 @@ public virtual async Task<IActionResult> OnGetAsync()

PersonalSettingsInfoModel = ObjectMapper.Map<ProfileDto, PersonalSettingsInfoModel>(user);

DisablePasswordChange = user.IsExternalLoggedIn;
HideOldPasswordInput = !user.HasPassword;

return Page();
}

Expand Down Expand Up @@ -54,7 +61,7 @@ public class ChangePasswordInfoModel
[DataType(DataType.Password)]
public string NewPasswordConfirm { get; set; }
}

public class PersonalSettingsInfoModel
{
[Required]
Expand Down
Expand Up @@ -15,13 +15,13 @@

if (
input.newPassword != input.newPasswordConfirm ||
input.currentPassword == ''
input.newPassword == ''
) {
abp.message.error(l('NewPasswordConfirmFailed'));
return;
}

if (input.currentPassword == '') {
if (input.currentPassword && input.currentPassword == ''){
return;
}

Expand Down
@@ -1,9 +1,18 @@
namespace Volo.Abp.Identity
using System.ComponentModel.DataAnnotations;
using Volo.Abp.Auditing;
using Volo.Abp.Validation;

namespace Volo.Abp.Identity
{
public class ChangePasswordInput
{
[DisableAuditing]
[DynamicStringLength(typeof(IdentityUserConsts), nameof(IdentityUserConsts.MaxPasswordLength))]
public string CurrentPassword { get; set; }

[Required]
[DisableAuditing]
[DynamicStringLength(typeof(IdentityUserConsts), nameof(IdentityUserConsts.MaxPasswordLength))]
public string NewPassword { get; set; }
}
}
Expand Up @@ -13,5 +13,9 @@ public class ProfileDto : ExtensibleObject
public string Surname { get; set; }

public string PhoneNumber { get; set; }

public bool IsExternalLoggedIn { get; set; }

public bool HasPassword { get; set; }
}
}
}
@@ -1,4 +1,5 @@
using AutoMapper;
using Volo.Abp.AutoMapper;

namespace Volo.Abp.Identity
{
Expand All @@ -11,9 +12,11 @@ public AbpIdentityApplicationModuleAutoMapperProfile()

CreateMap<IdentityRole, IdentityRoleDto>()
.MapExtraProperties();

CreateMap<IdentityUser, ProfileDto>()
.Ignore(x=>x.IsExternalLoggedIn)
.Ignore(x=>x.HasPassword)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of ignoring HasPassword, you can map it with a custom lambda expression that checks PasswordHash != null. In this way, you don't need to manual mapping later.

.MapExtraProperties();
}
}
}
}
@@ -1,4 +1,5 @@
using System.Threading.Tasks;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Identity;
using Volo.Abp.Identity.Settings;
Expand All @@ -20,9 +21,13 @@ public ProfileAppService(IdentityUserManager userManager)

public virtual async Task<ProfileDto> GetAsync()
{
return ObjectMapper.Map<IdentityUser, ProfileDto>(
await UserManager.GetByIdAsync(CurrentUser.GetId())
);
var currentUser = await UserManager.GetByIdAsync(CurrentUser.GetId());

var profile = ObjectMapper.Map<IdentityUser, ProfileDto>(currentUser);
profile.IsExternalLoggedIn = currentUser.IsExternal;
Copy link
Member

Choose a reason for hiding this comment

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

If you rename profile.IsExternalLoggedIn to currentUser.IsExternal then you don't need to manually map.

profile.HasPassword = currentUser.PasswordHash != null;

return profile;
Copy link
Member

Choose a reason for hiding this comment

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

After changing the mapping to fully use automapper, you can rollback changes in this method.

}

public virtual async Task<ProfileDto> UpdateAsync(UpdateProfileDto input)
Expand Down Expand Up @@ -56,6 +61,20 @@ public virtual async Task<ProfileDto> UpdateAsync(UpdateProfileDto input)
public virtual async Task ChangePasswordAsync(ChangePasswordInput input)
{
var currentUser = await UserManager.GetByIdAsync(CurrentUser.GetId());

if (currentUser.IsExternal)
{
throw new BusinessException(code: IdentityErrorCodes.ExternalUserPasswordChange);
}

if (currentUser.PasswordHash == null)
{
(await UserManager.RemovePasswordAsync(currentUser)).CheckErrors();
Copy link
Member

Choose a reason for hiding this comment

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

Is it really need to UserManager.RemovePasswordAsync. I suppose it is not needed because you are already checking if currentUser.PasswordHash == null.

(await UserManager.AddPasswordAsync(currentUser, input.NewPassword)).CheckErrors();

return;
}

(await UserManager.ChangePasswordAsync(currentUser, input.CurrentPassword, input.NewPassword)).CheckErrors();
}
}
Expand Down
Expand Up @@ -4,5 +4,6 @@ public static class IdentityErrorCodes
{
public const string UserSelfDeletion = "Volo.Abp.Identity:010001";
public const string MaxAllowedOuMembership = "Volo.Abp.Identity:010002";
public const string ExternalUserPasswordChange = "Volo.Abp.Identity:010003";
}
}
}
Expand Up @@ -102,6 +102,7 @@
"Description:Abp.Identity.SignIn.RequireConfirmedPhoneNumber": "Whether a confirmed telephone number is required to sign in.",
"Description:Abp.Identity.User.IsUserNameUpdateEnabled": "Whether the username can be updated by the user.",
"Description:Abp.Identity.User.IsEmailUpdateEnabled": "Whether the email can be updated by the user.",
"Volo.Abp.Identity:010002": "Can not set more than {MaxUserMembershipCount} organization unit for a user!"
"Volo.Abp.Identity:010002": "Can not set more than {MaxUserMembershipCount} organization unit for a user!",
"Volo.Abp.Identity:010003": "Can not change password of an externally logged in user!"
}
}