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

Custom admin/pass implementation for new tenant #3107

Merged
merged 7 commits into from
Mar 16, 2020
Merged

Conversation

cotur
Copy link
Contributor

@cotur cotur commented Mar 13, 2020

Closes: #3088

@cotur cotur added this to the 2.3 milestone Mar 13, 2020
@cotur cotur requested review from hikalkan and maliming March 13, 2020 10:24
@cotur cotur self-assigned this Mar 13, 2020
Copy link
Member

@maliming maliming left a comment

Choose a reason for hiding this comment

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

hi @cotur

The password will be checked by Identity. Will the web prompt when the password does not meet the requirements?

options.Password.RequiredLength = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequiredLength, options.Password.RequiredLength);
options.Password.RequiredUniqueChars = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequiredUniqueChars, options.Password.RequiredUniqueChars);
options.Password.RequireNonAlphanumeric = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequireNonAlphanumeric, options.Password.RequireNonAlphanumeric);
options.Password.RequireLowercase = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequireLowercase, options.Password.RequireLowercase);
options.Password.RequireUppercase = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequireUppercase, options.Password.RequireUppercase);
options.Password.RequireDigit = await _settingProvider.GetAsync(IdentitySettingNames.Password.RequireDigit, options.Password.RequireDigit);

Comment on lines 20 to 21
var isValidEmail =
Regex.IsMatch(AdminEmailAddress, TenantConsts.EmailRegex, RegexOptions.IgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use ValidationHelper.IsValidEmailAddress().

Copy link
Member

Choose a reason for hiding this comment

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

Use [EmailAddress] and [Required] attributes for the AdminEmailAddress. Also, add [Required] attribute for AdminPassword.
Completely remove custom Validate method.

@@ -51,8 +51,7 @@ public virtual async Task<TenantDto> CreateAsync(TenantCreateDto input)
{
//TODO: Handle database creation?

//TODO: Set admin email & password..?
await DataSeeder.SeedAsync(tenant.Id);
await DataSeeder.SeedAsync(tenant.Id, input.AdminEmailAddress, input.AdminPassword);
Copy link
Member

Choose a reason for hiding this comment

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

We can use:

await DataSeeder.SeedAsync(new DataSeedContext()
	.WithProperty("AdminEmail", input.AdminEmailAddress)
	.WithProperty("AdminPassword", input.AdminPassword));

Comment on lines 12 to 20

public static Task SeedAsync(this IDataSeeder seeder, Guid? tenantId, string adminEmailAddress, string adminPassword)
{
var context = new DataSeedContext(tenantId)
.WithProperty("AdminEmail", adminEmailAddress)
.WithProperty("AdminPassword", adminPassword);

return seeder.SeedAsync(context);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove it, because this code is not related to the Volo.Abp.Data module.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please remove this. adminEmailAddress and adminPassword is a concept of the Identity module.
However, you can add WithAdminEmail(string emailAddress) and WithAdminPassword(string password) extension methods to the DataSeedContext, but inside the Identity module!

var isValidEmail =
Regex.IsMatch(AdminEmailAddress, TenantConsts.EmailRegex, RegexOptions.IgnoreCase);

if (string.IsNullOrWhiteSpace(AdminEmailAddress) || !isValidEmail)
Copy link
Member

Choose a reason for hiding this comment

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

We can use if (!isValidEmail)

Comment on lines 12 to 20

public static Task SeedAsync(this IDataSeeder seeder, Guid? tenantId, string adminEmailAddress, string adminPassword)
{
var context = new DataSeedContext(tenantId)
.WithProperty("AdminEmail", adminEmailAddress)
.WithProperty("AdminPassword", adminPassword);

return seeder.SeedAsync(context);
}
Copy link
Member

Choose a reason for hiding this comment

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

Yes, please remove this. adminEmailAddress and adminPassword is a concept of the Identity module.
However, you can add WithAdminEmail(string emailAddress) and WithAdminPassword(string password) extension methods to the DataSeedContext, but inside the Identity module!

Comment on lines 20 to 21
var isValidEmail =
Regex.IsMatch(AdminEmailAddress, TenantConsts.EmailRegex, RegexOptions.IgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

Use [EmailAddress] and [Required] attributes for the AdminEmailAddress. Also, add [Required] attribute for AdminPassword.
Completely remove custom Validate method.

@hikalkan
Copy link
Member

@maliming

The password will be checked by Identity. Will the web prompt when the password does not meet the requirements?

Yes, UI should validate if possible (check how it was implemented for the user registeration).

@hikalkan hikalkan merged commit 7b26309 into dev Mar 16, 2020
@cotur cotur deleted the Cotur-TenantManagement branch March 19, 2020 06:29
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.

Get password & email address of the admin while creating a new tenant
3 participants