Ntuthuko/consolidate authentication settings #4330
Conversation
…sha-framework into ntuthuko/consolidate-authentication-settings
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
|
Warning Rate limit exceeded@ntuthukomsane-boxfusion has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughConsolidates frontend authentication and user-management configuration into a unified IUserManagementSettings surface (DefaultAuthentication + GeneralFrontendSecuritySettings + UserManagementSettings), replaces legacy settings accessors across services, rewrites UserManagementAppService for dynamic person handling and creation modes, adds frontend redirect settings, and includes a DB migration to merge existing JSON settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserSvc as UserManagementAppService
participant EntityCfg as EntityTypeConfigurationStore
participant DynRepo as DynamicRepository
participant UserMgr as UserManager
participant RoleSvc as ShaRoleAppointedPersonAppService
participant AuditRepo as RegistrationAuditRepo
participant DB as Database
Client->>UserSvc: CreateAsync(RegisterDto)
UserSvc->>EntityCfg: Resolve PersonEntityType
EntityCfg-->>UserSvc: Person type info
UserSvc->>DynRepo: FindExistingPerson(criteria)
DynRepo->>DB: Query person
DB-->>DynRepo: result
DynRepo-->>UserSvc: existing/null
alt CreationMode = MustAlreadyExist
UserSvc->>UserSvc: Validate existence / throw if not found
else
UserSvc->>DynRepo: CreateNewPerson(data)
DynRepo->>DB: Insert person
DB-->>DynRepo: created person
DynRepo-->>UserSvc: person
end
UserSvc->>UserMgr: CreateOrLinkUserAsync(userDto)
UserMgr->>DB: Create/Update User
DB-->>UserMgr: user record
UserMgr-->>UserSvc: user
UserSvc->>DynRepo: LinkUserToPerson(personId, userId)
DynRepo->>DB: Update person.user reference
DB-->>DynRepo: updated person
DynRepo-->>UserSvc: updated person
UserSvc->>RoleSvc: AssignDefaultRoles(user, person)
RoleSvc->>DB: Insert role-appointments
DB-->>RoleSvc: inserted
RoleSvc-->>UserSvc: done
UserSvc->>AuditRepo: SaveUserRegistrationAudit(user, person)
AuditRepo->>DB: Insert audit
DB-->>AuditRepo: saved
AuditRepo-->>UserSvc: done
UserSvc-->>Client: PersonAccountDto (with UserId / redirect)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shesha-core/src/Shesha.Application/Otp/OtpAppService.cs (1)
49-61: Same data loss risk: OTP update overwrites all other authentication settings.Similar to
AuthorizationSettingsAppService, this method creates a newDefaultAuthenticationSettingswith only OTP-related fields, overwriting any existing lockout, password complexity, and password reset settings configured elsewhere.This is especially problematic since both
OtpAppService.UpdateSettingsAsyncandAuthorizationSettingsAppService.UpdateSettingsAsyncwrite to the sameDefaultAuthenticationsettings object—whichever runs last will overwrite the other's fields.Apply the same fix: fetch existing settings first and preserve unrelated fields:
+ var existingSettings = await _userManagementSettings.DefaultAuthentication.GetValueAsync(); + await _userManagementSettings.DefaultAuthentication.SetValueAsync(new DefaultAuthenticationSettings { + // Preserve existing lockout, password complexity, and password reset settings + UserLockOutEnabled = existingSettings.UserLockOutEnabled, + MaxFailedAccessAttemptsBeforeLockout = existingSettings.MaxFailedAccessAttemptsBeforeLockout, + DefaultAccountLockoutSeconds = existingSettings.DefaultAccountLockoutSeconds, + RequireDigit = existingSettings.RequireDigit, + RequireLowercase = existingSettings.RequireLowercase, + RequireNonAlphanumeric = existingSettings.RequireNonAlphanumeric, + RequireUppercase = existingSettings.RequireUppercase, + RequiredLength = existingSettings.RequiredLength, + UseResetPasswordViaEmailLink = existingSettings.UseResetPasswordViaEmailLink, + ResetPasswordEmailLinkLifetime = existingSettings.ResetPasswordEmailLinkLifetime, + UseResetPasswordViaSmsOtp = existingSettings.UseResetPasswordViaSmsOtp, + ResetPasswordSmsOtpLifetime = existingSettings.ResetPasswordSmsOtpLifetime, + UseResetPasswordViaSecurityQuestions = existingSettings.UseResetPasswordViaSecurityQuestions, + ResetPasswordViaSecurityQuestionsNumQuestionsAllowed = existingSettings.ResetPasswordViaSecurityQuestionsNumQuestionsAllowed, + // ... other fields to preserve + PasswordLength = input.PasswordLength, Alphabet = input.Alphabet, ... });shesha-core/src/Shesha.Application/UserManagements/UserManagementAppService.cs (2)
77-205: Email is now effectively mandatory and domain validation throws non‑user‑friendly exceptionsWithin
CreateAsync,HandleAllowedDomainsAsyncis called unconditionally withinput.EmailAddress(line 153), and that helper throwsArgumentExceptionif the email is missing or malformed, andInvalidOperationExceptionif the domain is not allowed. This has a few implications:
- Behaviour change: even when
defaultAuthenticationSettings.UserEmailAsUsernameisfalse, email becomes mandatory becauseHandleAllowedDomainsAsyncwill throw for null/empty emails beforevalidationResults.ThrowValidationExceptionIfAny(L)runs.- Inconsistent UX: other validation errors are aggregated into
validationResultsand surfaced as proper validation exceptions, but domain issues surface asArgumentException/InvalidOperationException, which will typically present as 500s rather than user‑friendly messages.If email is meant to be optional when username is used, the domain handler should short‑circuit when
string.IsNullOrWhiteSpace(input.EmailAddress)andAllowedEmailDomainsis empty/whitespace. If email is meant to be required in all cases, consider:
- Expressing that via the
validationResultslist rather thanArgumentException.- Using
UserFriendlyException(or the same validation mechanism) for invalid format and disallowed domain to keep responses consistent.A possible direction (adjust to your conventions):
- await HandleAllowedDomainsAsync( - input.EmailAddress ?? throw new UserFriendlyException(nameof(input.EmailAddress), "Email address cannot be null."), - allowedEmailDomains ?? string.Empty - ); + if (!string.IsNullOrWhiteSpace(input.EmailAddress) || !string.IsNullOrWhiteSpace(allowedEmailDomains)) + { + await HandleAllowedDomainsAsync( + input.EmailAddress ?? throw new UserFriendlyException(nameof(input.EmailAddress), "Email address cannot be null."), + allowedEmailDomains ?? string.Empty + ); + }and inside
HandleAllowedDomainsAsync(see separate comment) change the thrown exception types to your user‑facing ones.
162-318: CreationMode handling is inconsistent (null CreationMode and MustAlreadyExist with user‑less Person will currently blow up)The combination of
HandleCreationModeAsync(lines 243–270) andCreateOrLinkUserAsync(lines 272–318) has a couple of problematic edge cases:
creationMode == null
HandleCreationModeAsynchits thedefaultbranch and throwsNotImplementedExceptioneven though the comment says “Default behavior (no creation mode specified) – create new person.”CreateOrLinkUserAsyncalso throwsArgumentNullExceptionwhencreationModeis null (line 280), whileCreateOrLinkPersonEntityAsync’sdefaultcase happily creates a new person (lines 357–360).
This will break existing tenants whereCreationModehasn’t been configured yet and also contradicts the documented “default” behaviour.
RefListCreationMode.MustAlreadyExistwith an existingPersonbut noPerson.User
HandleCreationModeAsynconly requiresexistingPerson != null(lines 257–260), so validation passes.- In
CreateOrLinkUserAsync,foundUserbecomes((Person)existingPerson).User. If that property is null, there is no matching switch arm forMustAlreadyExist, so thedefaultarm runs and throwsInvalidOperationException("Unsupported creation mode: ...")(line 316).- Business‑wise,
MustAlreadyExistusually means “person record must already exist”; it does not require a pre‑existingUser. You almost certainly want to allow creating a new user and then linking it.To make behaviour consistent and keep backwards compatibility, I suggest:
- Treat
nullCreationModeas the legacy “create new person + user” behaviour.- For
MustAlreadyExist, allow the case where the person exists but has no user: create a new user and then link.One possible refactor:
- private static async Task HandleCreationModeAsync(RefListCreationMode? creationMode, object? existingPerson) - { - await Task.CompletedTask; // Make it async compatible since we no longer need to call database - - switch (creationMode) - { - case RefListCreationMode.Always: - if (existingPerson != null) - throw new UserFriendlyException($"A person with the same identifier already exists. Creation mode is set to 'Always' which requires no existing person."); - break; - - case RefListCreationMode.MustAlreadyExist: - if (existingPerson == null) - throw new UserFriendlyException($"No existing person found with the specified identifier. Creation mode is set to 'Must already exist' which requires a pre-existing person."); - break; - - case RefListCreationMode.CreateNewButLinkIfExist: - // No validation needed - handled in CreateOrLinkPersonAsync - break; - - default: - // Default behavior (no creation mode specified) - create new person - throw new NotImplementedException(); - } - } + private static Task HandleCreationModeAsync(RefListCreationMode? creationMode, object? existingPerson) + { + // Default behaviour when not configured: no extra validation (equivalent to "create new") + if (creationMode == null) + return Task.CompletedTask; + + switch (creationMode) + { + case RefListCreationMode.Always: + if (existingPerson != null) + throw new UserFriendlyException("A person with the same identifier already exists. Creation mode is set to 'Always' which requires no existing person."); + break; + + case RefListCreationMode.MustAlreadyExist: + if (existingPerson == null) + throw new UserFriendlyException("No existing person found with the specified identifier. Creation mode is set to 'Must already exist' which requires a pre-existing person."); + break; + + case RefListCreationMode.CreateNewButLinkIfExist: + // No additional validation – linking logic handles this mode. + break; + + default: + throw new InvalidOperationException($"Unsupported creation mode: {creationMode}"); + } + + return Task.CompletedTask; + }and in
CreateOrLinkUserAsync:- private async Task<User> CreateOrLinkUserAsync( + private async Task<User> CreateOrLinkUserAsync( RefListCreationMode? creationMode, object? existingPerson, CreatePersonAccountDto input, string? userName, long? supportedPasswordResetMethods ) { - if (creationMode is null) - throw new ArgumentNullException(nameof(creationMode)); - var isInternalAccount = input.TypeOfAccount?.ItemValue == (long)RefListTypeOfAccount.Internal; var username = userName.NotNull(); var foundUser = existingPerson != null ? ((Person)existingPerson).User : null; @@ - return creationMode switch - { - RefListCreationMode.Always when foundUser is null => await CreateUserAsync(), - RefListCreationMode.MustAlreadyExist when foundUser is not null => await LinkUserAsync(), - RefListCreationMode.CreateNewButLinkIfExist when foundUser is not null => await LinkUserAsync(), - RefListCreationMode.CreateNewButLinkIfExist => await CreateUserAsync(), - _ => throw new InvalidOperationException($"Unsupported creation mode: {creationMode}") - }; + if (creationMode is null) + { + // Legacy/default behaviour: always create a new user + return await CreateUserAsync(); + } + + return creationMode switch + { + RefListCreationMode.Always => await CreateUserAsync(), + + // Person must exist by now; if they already have a user, link, otherwise create a new one. + RefListCreationMode.MustAlreadyExist when foundUser is not null => await LinkUserAsync(), + RefListCreationMode.MustAlreadyExist => await CreateUserAsync(), + + RefListCreationMode.CreateNewButLinkIfExist when foundUser is not null => await LinkUserAsync(), + RefListCreationMode.CreateNewButLinkIfExist => await CreateUserAsync(), + + _ => throw new InvalidOperationException($"Unsupported creation mode: {creationMode}") + }; }This keeps
CreateOrLinkPersonEntityAsync’s default behaviour (“create new person”) aligned with the user creation logic and preventsNotImplementedException/InvalidOperationExceptionfrom leaking to callers in normal configuration scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
shesha-core/test/Shesha.Tests/Otp/OtpManager_Tests.csis excluded by none and included by noneshesha-core/test/Shesha.Tests/Otp/PasswordGenerator_Tests.csis excluded by none and included by none
📒 Files selected for processing (33)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs(1 hunks)shesha-core/src/Shesha.Application/Authorization/Settings/Dto/AuthorizationSettingsDto.cs(3 hunks)shesha-core/src/Shesha.Application/Notifications/NotificationTemplateProcessor.cs(1 hunks)shesha-core/src/Shesha.Application/Otp/Configuration/IOtpSettings.cs(0 hunks)shesha-core/src/Shesha.Application/Otp/Configuration/OtpSettings.cs(0 hunks)shesha-core/src/Shesha.Application/Otp/OtpAppService.cs(3 hunks)shesha-core/src/Shesha.Application/Otp/OtpGenerator.cs(1 hunks)shesha-core/src/Shesha.Application/Otp/OtpManager.cs(6 hunks)shesha-core/src/Shesha.Application/Persons/PersonAccountDto.cs(0 hunks)shesha-core/src/Shesha.Application/SecurityQuestions/QuestionAnswersAppService.cs(3 hunks)shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs(1 hunks)shesha-core/src/Shesha.Application/Shesha.Application.csproj(2 hunks)shesha-core/src/Shesha.Application/SheshaApplicationModule.cs(4 hunks)shesha-core/src/Shesha.Application/UserManagements/Configurations/IUserManagementSettings.cs(0 hunks)shesha-core/src/Shesha.Application/UserManagements/Configurations/UserManagementSettingNames.cs(0 hunks)shesha-core/src/Shesha.Application/UserManagements/Configurations/UserManagementSettings.cs(0 hunks)shesha-core/src/Shesha.Application/UserManagements/UserManagementAppService.cs(3 hunks)shesha-core/src/Shesha.Application/Users/UserAppService.cs(11 hunks)shesha-core/src/Shesha.Framework/Authorization/Users/UserManager.cs(6 hunks)shesha-core/src/Shesha.Framework/Configuration/FrontendApplicationRedirectsSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/IFrontendSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/IPasswordComplexitySettings.cs(0 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/DefaultAuthenticationSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/GeneralFrontendSecuritySettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/IUserManagementSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/RefListCreationMode.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettingNames.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/ISecuritySettings.cs(0 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/SecuritySettings.cs(0 hunks)shesha-core/src/Shesha.Framework/Configuration/SheshaSettingNames.cs(1 hunks)shesha-core/src/Shesha.Framework/Domain/Enums/SupportedRegistrationMethods.cs(1 hunks)shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs(3 hunks)
💤 Files with no reviewable changes (9)
- shesha-core/src/Shesha.Application/UserManagements/Configurations/UserManagementSettings.cs
- shesha-core/src/Shesha.Framework/Configuration/IPasswordComplexitySettings.cs
- shesha-core/src/Shesha.Application/Persons/PersonAccountDto.cs
- shesha-core/src/Shesha.Application/UserManagements/Configurations/IUserManagementSettings.cs
- shesha-core/src/Shesha.Application/UserManagements/Configurations/UserManagementSettingNames.cs
- shesha-core/src/Shesha.Framework/Configuration/Security/ISecuritySettings.cs
- shesha-core/src/Shesha.Framework/Configuration/Security/SecuritySettings.cs
- shesha-core/src/Shesha.Application/Otp/Configuration/IOtpSettings.cs
- shesha-core/src/Shesha.Application/Otp/Configuration/OtpSettings.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T17:11:17.267Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3678
File: shesha-core/src/Shesha.Application/Otp/Configuration/OtpDefaults.cs:14-15
Timestamp: 2025-08-05T17:11:17.267Z
Learning: In the Shesha framework, the DefaultEmailBodyTemplate in OtpDefaults.cs contains a hard-coded localhost URL by design. This serves as a default that gets updated/overridden in production environments through their existing configuration management process.
Applied to files:
shesha-core/src/Shesha.Application/Notifications/NotificationTemplateProcessor.csshesha-core/src/Shesha.Application/Otp/OtpGenerator.csshesha-core/src/Shesha.Application/Otp/OtpManager.csshesha-core/src/Shesha.Application/Otp/OtpAppService.csshesha-core/src/Shesha.Framework/Configuration/SheshaSettingNames.csshesha-core/src/Shesha.Framework/SheshaFrameworkModule.csshesha-core/src/Shesha.Framework/Configuration/Security/Frontend/DefaultAuthenticationSettings.csshesha-core/src/Shesha.Application/SheshaApplicationModule.cs
📚 Learning: 2025-10-25T09:24:59.417Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4063
File: shesha-core/src/Shesha.Web.Host/appsettings.Docker.json:3-3
Timestamp: 2025-10-25T09:24:59.417Z
Learning: In the shesha-framework repository, for Docker-specific configuration files like appsettings.Docker.json used for local development, hardcoded credentials are acceptable for plug-and-play developer experience.
Applied to files:
shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs
🔇 Additional comments (26)
shesha-core/src/Shesha.Application/Authorization/Settings/Dto/AuthorizationSettingsDto.cs (1)
67-70: LGTM!The new
AutoLogoffAfterInactivityboolean property complements the existingAutoLogoffTimeoutinteger, providing a clear toggle for the auto-logoff feature.shesha-core/src/Shesha.Application/SheshaApplicationModule.cs (1)
154-154: LGTM!The service registration follows the existing patterns and uses the appropriate transient lifecycle.
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/RefListCreationMode.cs (1)
12-38: LGTM!The enum is well-designed with clear documentation for each creation mode. The
ReferenceListattribute properly registers it for framework use.shesha-core/src/Shesha.Framework/Domain/Enums/SupportedRegistrationMethods.cs (3)
17-17: LGTM - Clarified display name.The display name change from "Email Address" to "Email OTP" better clarifies the registration method involves OTP verification, improving user understanding.
22-22: LGTM - Clarified display name.The display name change from "Mobile Number" to "Mobile OTP" better clarifies the registration method involves OTP verification, improving user understanding.
29-33: LGTM - New registration method added.The addition of
EmailLink = 5provides an alternative email-based registration method using verification links instead of OTP codes, expanding the available authentication options.shesha-core/src/Shesha.Application/Otp/OtpGenerator.cs (1)
10-15: LGTM - Dependency updated to consolidated settings.The constructor correctly migrates from
IOtpSettingstoIUserManagementSettings, aligning with the PR's consolidation objective.shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettingNames.cs (1)
1-9: LGTM - Clean constants definition.The
UserManagementSettingNamesclass provides standardized setting name keys for the consolidated user management configuration surface. The implementation is clean and follows established patterns.shesha-core/src/Shesha.Application/Otp/OtpManager.cs (5)
21-30: LGTM - Dependency updated to consolidated settings.The constructor correctly migrates from
IOtpSettingstoIUserManagementSettings, maintaining the same functionality while aligning with the PR's consolidation objective.
40-40: LGTM - Settings retrieval path updated.The settings retrieval correctly uses
_userManagementSettings.DefaultAuthentication.GetValueAsync(), consistent with the configuration consolidation.
93-93: LGTM - Settings retrieval path updated.The settings retrieval correctly uses
_userManagementSettings.DefaultAuthentication.GetValueAsync(), consistent with the configuration consolidation.
165-165: LGTM - Settings retrieval path updated.The settings retrieval correctly uses
_userManagementSettings.DefaultAuthentication.GetValueAsync(), consistent with the configuration consolidation.
189-189: LGTM - Settings retrieval path updated.The settings retrieval correctly uses
_userManagementSettings.DefaultAuthentication.GetValueAsync(), consistent with the configuration consolidation.shesha-core/src/Shesha.Application/SecurityQuestions/QuestionAnswersAppService.cs (2)
38-38: [rewritten comment]
[classification tag]
18-26: LGTM - Dependency updated to consolidated settings.The service shows correct injection of
IUserManagementSettingsin the constructor. However, verification of theResetPasswordViaSecurityQuestionsNumQuestionsAllowedproperty availability in the migrated settings could not be completed due to repository access limitations.shesha-core/src/Shesha.Application/Notifications/NotificationTemplateProcessor.cs (1)
97-104: Breaking change: Property type changed from string to complex object.The
PublicUrlproperty now returnsFrontendApplicationRedirectsSettings?instead ofstring?, which is a breaking change for any mustache templates or code that accesses this property. Template expressions like{{frontEndApps.myApp.PublicUrl}}will now render an object representation instead of a URL string.Consider either:
- Renaming the property to
ApplicationRedirectsSettingsto reflect its actual type, OR- Adding a separate
BaseUrlproperty that returnsstring?for backward compatibilityVerify that all mustache templates and notification templates using
PublicUrlare updated to access the nested properties (e.g.,{{frontEndApps.myApp.PublicUrl.BaseUrl}}), or confirm that backward compatibility measures were implemented.shesha-core/src/Shesha.Framework/Configuration/IFrontendSettings.cs (1)
23-27: Unable to verify breaking change claim due to repository access limitations.The breaking change assertion about removal of
DefaultUrlandPublicUrlproperties cannot be verified without access to the repository history and codebase search capabilities. To properly assess this issue, provide:
- Git commit or diff showing the removed properties from
IFrontendSettings.cs- Confirmation that these properties existed in the previous version
- Evidence of where references to these properties have been migrated or updated
shesha-core/src/Shesha.Framework/Configuration/SheshaSettingNames.cs (2)
25-29: LGTM! New setting name constants added.The new constants
SuccessLoginRedirectPath,ApplicationRedirects, andDefaultAuthenticationSettingsfollow the established naming conventions and support the authentication settings consolidation.
19-21: Verify breaking change: renamed setting name constants.Renaming
DefaultUrl→BaseUrlandPublicUrl→DefaultPathchanges the underlying string values from"Shesha.DefaultUrl"to"Shesha.BaseUrl"and"Shesha.PublicUrl"to"Shesha.DefaultPath". This may affect persisted settings or external consumers referencing the old names. Ensure any config migrations or deprecation handling is in place.shesha-core/src/Shesha.Application/Users/UserAppService.cs (2)
56-83: LGTM! Dependency injection properly wired.The
IUserManagementSettingsis correctly injected and stored in a private field, following the established pattern.
289-304: LGTM! Settings access migrated correctly.The retrieval of authentication settings from
_userManagementSettings.DefaultAuthenticationand subsequent use for feature toggles is correctly implemented.shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs (2)
131-136: LGTM! Security settings simplified.The
SecuritySettingsdefault now only configuresDefaultEndpointAccess, which aligns with moving password/lockout settings to the consolidatedIUserManagementSettingsinterface.
144-149: Consider addingDefaultPathto the default initialization.
FrontendApplicationRedirectsSettingshas four properties, but only three are initialized here. TheDefaultPathproperty is missing from the defaults. If it's intentionally omitted, this is fine; otherwise, consider adding a sensible default.shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/IUserManagementSettings.cs (1)
7-30: Well-structured settings interface consolidation.The interface cleanly consolidates authentication-related settings into a single access point. The use of
IsClientSpecific = trueon all settings is appropriate for tenant-specific configurations, and the editor form names follow a consistent naming convention.shesha-core/src/Shesha.Framework/Authorization/Users/UserManager.cs (1)
351-368: LGTM for async initialization.The async version correctly reads settings from
DefaultAuthenticationand applies appropriate null-coalescing defaults for lockout and password complexity options.shesha-core/src/Shesha.Application/UserManagements/UserManagementAppService.cs (1)
487-523: Dynamic default role assignment logic looks sound; just be mindful of potential duplicate role assignmentsThe
AssignDefaultRolesAsynchelper correctly:
- Extracts the person
Idvia reflection, which works for anyPerson‑derived dynamic type.- Skips null/empty role ids.
- Checks
_roleAppointedPersonRepositoryfor an existing appointment before calling_roleAppointedPersonService.CreateAsync, avoiding duplicates.This is generally solid. Two minor considerations:
- If
_roleAppointedPersonService.CreateAsynchas side effects or heavy logic, you might want to refactor to a batch‑oriented service in future, but that’s an optimisation, not a blocker here.- If there is a unique constraint at the database level on
(RoleId, PersonId), this pattern is safe from race conditions in single‑request flows but can still theoretically race under concurrent registrations for the same person; the DB constraint would be the final guard.No changes strictly required now; just keep these aspects in mind.
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs
Outdated
Show resolved
Hide resolved
shesha-core/src/Shesha.Application/Authorization/Settings/Dto/AuthorizationSettingsDto.cs
Outdated
Show resolved
Hide resolved
...-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs
Outdated
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/RefListCreationMode.cs
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs
Outdated
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs
Outdated
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs
Outdated
Show resolved
Hide resolved
…nd/RefListCreationMode.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nd/UserManagementSettings.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nd/UserManagementSettings.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…AuthorizationSettingsDto.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…sha-framework into ntuthuko/consolidate-authentication-settings
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
…sha-framework into ntuthuko/consolidate-authentication-settings
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/DefaultAuthenticationSettings.cs (2)
30-34: Empty XML documentation comment forUserEmailAsUsername.The
<summary>tag is empty. Add a meaningful description./// <summary> - /// + /// Specifies whether the user's email address should be used as their username. /// </summary> [Display(Name = "Use email as username")] public bool UserEmailAsUsername { get; set; }
106-137: Non-nullable string properties may causeNullReferenceException.
Alphabet,DefaultSubjectTemplate,DefaultBodyTemplate,DefaultEmailSubjectTemplate, andDefaultEmailBodyTemplateare declared as non-nullablestringbut have no default values. When settings are deserialized without these values, they will benull, potentially causing NREs.Either make them nullable or provide default values:
- public string Alphabet { get; set; } + public string Alphabet { get; set; } = string.Empty; ... - public string DefaultSubjectTemplate { get; set; } + public string DefaultSubjectTemplate { get; set; } = string.Empty; ... - public string DefaultBodyTemplate { get; set; } + public string DefaultBodyTemplate { get; set; } = string.Empty; ... - public string DefaultEmailSubjectTemplate { get; set; } + public string DefaultEmailSubjectTemplate { get; set; } = string.Empty; ... - public string DefaultEmailBodyTemplate { get; set; } + public string DefaultEmailBodyTemplate { get; set; } = string.Empty;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs(1 hunks)shesha-core/src/Shesha.Application/Authorization/Settings/Dto/AuthorizationSettingsDto.cs(3 hunks)shesha-core/src/Shesha.Application/Otp/OtpGenerator.cs(1 hunks)shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/DefaultAuthenticationSettings.cs(1 hunks)shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T17:11:17.267Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3678
File: shesha-core/src/Shesha.Application/Otp/Configuration/OtpDefaults.cs:14-15
Timestamp: 2025-08-05T17:11:17.267Z
Learning: In the Shesha framework, the DefaultEmailBodyTemplate in OtpDefaults.cs contains a hard-coded localhost URL by design. This serves as a default that gets updated/overridden in production environments through their existing configuration management process.
Applied to files:
shesha-core/src/Shesha.Application/Otp/OtpGenerator.csshesha-core/src/Shesha.Framework/Configuration/Security/Frontend/DefaultAuthenticationSettings.cs
🔇 Additional comments (4)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs (1)
13-17: RoleId may no longer be explicitly mapped on the DTOThe current
CreateMap<ShaRoleAppointedPerson, ShaRoleAppointedPersonDto>()only configuresPerson. If the previous version explicitly mappedRoleIdfrome.Role.Id, this mapping is no longer present. Verify whether:
ShaRoleAppointedPersonDtohas aRoleIdproperty and whether it is still being populatedShaRoleAppointedPersonexposesRolenavigation (so AutoMapper flattening could apply) or only a scalarRoleId- Any code depends on
RoleIdbeing populated on the DTOIf consumers require
RoleIdand it is not being auto-mapped via conventions, restore the mapping alongside thePersonconfiguration.shesha-core/src/Shesha.Framework/Configuration/Security/Frontend/UserManagementSettings.cs (1)
9-48: LGTM! All previous review comments have been addressed.The class is well-structured with proper XML documentation, nullable annotations where appropriate, and default initialization for
DefaultRoles. Good use ofDisplayattributes for UI binding.shesha-core/src/Shesha.Application/Otp/OtpGenerator.cs (1)
17-31: LGTM! Previous optimization feedback addressed.The settings are now retrieved once and cached in
authSettings. Good use ofStringBuilderfor efficient PIN generation.Note: Consider adding a null/empty guard for
alphabetto preventIndexOutOfRangeExceptionif the setting is not configured:var authSettings = _userManagementSettings.DefaultAuthentication.GetValue(); var alphabet = authSettings.Alphabet; var passwordLength = authSettings.PasswordLength; + + if (string.IsNullOrEmpty(alphabet)) + throw new InvalidOperationException("OTP alphabet is not configured."); for (int i = 0; i < passwordLength; i++)shesha-core/src/Shesha.Application/Authorization/Settings/Dto/AuthorizationSettingsDto.cs (1)
1-96: LGTM!The DTO is well-structured with proper XML documentation. The new
AutoLogoffAfterInactivityproperty andDisplayattributes are appropriately added. Previous feedback about unused imports has been addressed.
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs(1 hunks)shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs(1 hunks)
🔇 Additional comments (2)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs (1)
23-27: LGTM!The Entity→DTO mapping correctly transforms the
Personentity to anEntityReferenceDto<Guid?>with display name and type information. The null check is appropriate.shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs (1)
64-68: Verify whetherGeneralFrontendSecuritySettingshas other properties that should be preserved.This code creates a new
GeneralFrontendSecuritySettingsobject with only two properties from the DTO (AutoLogoffAfterInactivityandAutoLogoffTimeout). IfGeneralFrontendSecuritySettingshas additional properties not included here, they will be lost when this object is saved. Check the class definition to confirm all properties are being set, or consider using a merge/update pattern similar to the fix applied toDefaultAuthenticationSettings.
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs
Show resolved
Hide resolved
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs
Show resolved
Hide resolved
...-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs
Outdated
Show resolved
Hide resolved
...-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs
Outdated
Show resolved
Hide resolved
…orizationSettingsAppService.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs (1)
77-104: Add null‑safety for settings returned byGetValueAsync
defaultAuthSettingsandgeneralFrontendSettingsare used immediately without null checks (e.g.,defaultAuthSettings.UserLockOutEnabled). IfGetValueAsync()returns null (fresh install, misconfig, or failed migration), this will throw aNullReferenceExceptionand break settings retrieval.You already handle the null case for
DefaultAuthenticationinUpdateSettingsAsync; recommend doing the same here:- var defaultAuthSettings = await _userManagementSettings.DefaultAuthentication.GetValueAsync(); - var generalFrontendSettings = await _userManagementSettings.GeneralFrontendSecuritySettings.GetValueAsync(); + var defaultAuthSettings = await _userManagementSettings.DefaultAuthentication.GetValueAsync() + ?? new DefaultAuthenticationSettings(); + var generalFrontendSettings = await _userManagementSettings.GeneralFrontendSecuritySettings.GetValueAsync() + ?? new GeneralFrontendSecuritySettings();This keeps
GetSettingsAsyncrobust even when settings haven’t been saved yet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs(1 hunks)
🔇 Additional comments (1)
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs (1)
12-17: Constructor consolidation toIUserManagementSettingslooks goodSwitching this app service to depend only on
IUserManagementSettingskeeps the settings surface cohesive and aligns with the new consolidated configuration model. No issues from a correctness or maintainability perspective.
shesha-core/src/Shesha.Application/Authorization/Settings/AuthorizationSettingsAppService.cs
Outdated
Show resolved
Hide resolved
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @ntuthukomsane-boxfusion. Please do the following:
- Specify url of the GitHub issue these changes described in
- Remove all commented out code and console logging from scripts on attached packages
- Consolidate all packages into a single one
- Debug forms on empty DB frtom starter project and make sure new settings work properly. You lost at least some default values and login form throws an exception.
- Values of old settings should be migrated to a new structure or GitHub issue should clearly define that it's a breaking change and requires reconfiguration after update
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
…orizationSettingsAppService.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shesha-core/src/Shesha.Application/SheshaApplicationModule.cs (1)
140-143: Default minimum password length of 3 is too weak.
RequiredLength = 3does not meet industry standards. NIST guidelines recommend a minimum of 8 characters. This provides negligible security against brute-force attacks.// Password Complexity RequireDigit = true, - RequiredLength = 3, + RequiredLength = 8, RequireLowercase = true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-core/src/Shesha.Application/SheshaApplicationModule.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T17:11:17.267Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3678
File: shesha-core/src/Shesha.Application/Otp/Configuration/OtpDefaults.cs:14-15
Timestamp: 2025-08-05T17:11:17.267Z
Learning: In the Shesha framework, the DefaultEmailBodyTemplate in OtpDefaults.cs contains a hard-coded localhost URL by design. This serves as a default that gets updated/overridden in production environments through their existing configuration management process.
Applied to files:
shesha-core/src/Shesha.Application/SheshaApplicationModule.cs
🔇 Additional comments (3)
shesha-core/src/Shesha.Application/SheshaApplicationModule.cs (3)
138-138: Verify that mobile-only verification is the intended default.
SupportedVerificationMethodsdefaults toMobileNumberonly. This may exclude users who prefer or require email verification. Confirm this is the intended behavior, or consider supporting both methods by default.
161-161: LGTM!The service registration follows the existing pattern and uses the appropriate transient lifestyle.
130-157: Good consolidation of authentication settings.The unified
DefaultAuthenticationSettingsobject with logical groupings (Account Creation, Password Complexity, OTP, Password Resets) improves maintainability. The use ofOtpDefaultsconstants for OTP settings ensures consistency with existing defaults.
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs(1 hunks)shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T10:09:19.065Z
Learnt from: Tshepo1103-lab
Repo: shesha-io/shesha-framework PR: 4013
File: shesha-core/src/Shesha.NHibernate/Migrations/M20251106095300.cs:29-51
Timestamp: 2025-11-06T10:09:19.065Z
Learning: The legacy table Frwk_StoredFileVersionDownloads in the Shesha framework did not have a TenantId column. When migrating to the new frwk.stored_file_version_downloads table in migration M20251106095300, setting tenant_id to NULL for historical records is correct.
Applied to files:
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs
🔇 Additional comments (5)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs (2)
14-21: Good refactoring - previous issues resolved.The inline Person mapping successfully addresses the critical issues flagged in previous reviews:
- ✅ No database queries during mapping (eliminates N+1 problems)
- ✅ No service locator anti-pattern (removed
StaticContext.IocManager.Resolve)- ✅ Proper null-checking with defensive coding for
GetType().FullNameThis is a clean, maintainable approach to mapping the Person reference.
15-19: No action needed. TheEntityReferenceDto<Guid?>type in the mapping correctly matches thePersonproperty definition inShaRoleAppointedPersonDto, which is also declared asEntityReferenceDto<Guid?>. The mapping compiles and functions correctly.shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (3)
14-240: Consider transaction safety for partial failure scenarios.The SQL Server migration performs multiple sequential SELECT/UPDATE operations on different setting records. If a failure occurs mid-execution (e.g., during the DefaultAuthentication update), earlier modifications to UserManagement and Security settings will already be committed, leaving the database in a partially migrated state.
FluentMigrator typically wraps migrations in transactions, but verify that
Execute.Sql()participates in the migration transaction. If not, consider wrapping the SQL in an explicit transaction or structuring the migration to be idempotent so re-running is safe.
452-454: Verify PostgreSQL version compatibility forgen_random_uuid().The function
gen_random_uuid()is only available natively in PostgreSQL 13+. If the application supports PostgreSQL 12 or earlier, this will fail unless thepgcryptoextension is enabled.
234-238: The INSERT statement appears correct based on the migration's own UPDATE statement pattern. Both INSERT and UPDATE in the same migration use the same minimal column set: onlyValueand related metadata (creation_time,last_modification_time). Ifis_deletedortenant_idwere required NOT NULL columns without defaults, the UPDATE statement would also fail. The consistency between INSERT and UPDATE patterns suggests these columns either have defaults, are nullable, or are not required—making the concern likely unfounded.
...-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (2)
270-288: Add JSON validation before casting to prevent migration abort.The PostgreSQL branch casts
sv."value"::JSONBwithout validation (line 272), unlike SQL Server which validates withISJSON(sv.Value) = 1(line 31). Malformed JSON will raise an exception and abort the migration.Apply this diff to add validation:
WHERE ci.item_type = 'setting-configuration' AND ( ci."name" LIKE '%UserManagement%' OR sv."value"::TEXT LIKE '%supportedRegistrationMethods%' OR sv."value"::TEXT LIKE '%additionalRegistrationInfo%' ) AND sv."value" IS NOT NULL + AND sv."value"::TEXT ~ '^\s*[\{\[]' -- Basic JSON format check LIMIT 1;Alternatively, wrap the entire DO block in an exception handler to catch and log parse errors without aborting the migration.
280-286: Replace broad LIKE patterns with exact matches.Similar to the SQL Server branch issues (lines 84-88, 136-141), the PostgreSQL queries use broad LIKE patterns that could match unintended configuration items combined with
LIMIT 1.Apply this diff:
WHERE ci.item_type = 'setting-configuration' - AND ( - ci."name" LIKE '%UserManagement%' - OR sv."value"::TEXT LIKE '%supportedRegistrationMethods%' - OR sv."value"::TEXT LIKE '%additionalRegistrationInfo%' - ) + AND ci."name" = 'Shesha.UserManagement' -- exact match AND sv."value" IS NOT NULL LIMIT 1;Apply similar fixes to the Security query (lines 334-338) and OTP query (lines 376-381).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs(1 hunks)shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T10:09:19.065Z
Learnt from: Tshepo1103-lab
Repo: shesha-io/shesha-framework PR: 4013
File: shesha-core/src/Shesha.NHibernate/Migrations/M20251106095300.cs:29-51
Timestamp: 2025-11-06T10:09:19.065Z
Learning: The legacy table Frwk_StoredFileVersionDownloads in the Shesha framework did not have a TenantId column. When migrating to the new frwk.stored_file_version_downloads table in migration M20251106095300, setting tenant_id to NULL for historical records is correct.
Applied to files:
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs
🔇 Additional comments (4)
shesha-core/src/Shesha.Application/ShaRoleAppointedPersons/Dto/ShaRoleAppointedPersonMapProfile.cs (2)
14-21: VerifyPersonDTO type matchesEntityReferenceDto<Guid?>.The mapping now constructs
EntityReferenceDto<Guid?>with Id, FullName, and TypeName whenPersonis non‑null, which looks correct, but it assumesShaRoleAppointedPersonDto.Personis also declared asEntityReferenceDto<Guid?>. Please confirm the DTO’s generic type matches to avoid assignment/type mismatches.
22-23: RoleId mapping is now correctly populated from the Role navigation.Mapping
RoleIdfrome.Role != null ? e.Role.Id : (Guid?)nullensures the DTO gets a proper nullable Guid instead of the default, and keeps behavior explicit whenRoleis absent. No further changes needed here.shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (2)
16-66: UserManagement transformation looks correct.The exact match on
ci.Name = 'Shesha.UserManagement'at line 29 properly addresses the past review concern about broad LIKE patterns. The conditional block structure and JSON transformation logic are sound.
145-236: Control flow structure is now correct, but LIKE patterns need refinement.The
BEGIN...ENDblock structure properly addresses the past review comment about missing conditional scope. However, the query at lines 136-141 still uses broad LIKE patterns that could match unintended settings.Apply this diff to use exact matches:
WHERE ci.item_type = 'setting-configuration' AND ( - ci.Name LIKE '%Otp%' - OR ci.Name LIKE '%OneTimePin%' - OR sv.Value LIKE '%passwordLength%' - OR sv.Value LIKE '%alphabet%' + ci.Name IN ('Shesha.Otp', 'Shesha.OneTimePin') -- enumerate exact names ) AND sv.Value IS NOT NULL AND ISJSON(sv.Value) = 1;Likely an incorrect or invalid review comment.
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (3)
328-336: Inconsistent query pattern with SQL Server branch.Similar to the User Management section, this uses broad LIKE patterns while SQL Server uses an exact match. Align with SQL Server for consistent behavior.
Apply this diff:
WHERE ci.item_type = 'setting-configuration' - AND ( - ci.""name"" LIKE '%Security%' - OR sv.""value""::TEXT LIKE '%autoLogoffTimeout%' - OR sv.""value""::TEXT LIKE '%useResetPasswordViaEmailLink%' - ) + AND ci.""name"" = 'Shesha.Security' AND sv.""value"" IS NOT NULL LIMIT 1;
385-422: Logic inconsistency: PostgreSQL unconditionally creates DefaultAuth, SQL Server requires OTP settings.The SQL Server branch only creates DefaultAuthenticationSettings when
@OldOtpValue IS NOT NULL(line 141), but the PostgreSQL branch unconditionally buildsv_new_default_auth_valuehere regardless of whether any legacy settings were found. This causes different migration behavior across database platforms.If the intent is to require at least one legacy setting before creating DefaultAuth, wrap this block conditionally:
+IF v_old_otp_value IS NOT NULL OR v_old_user_mgmt_value IS NOT NULL OR v_old_security_value IS NOT NULL THEN + v_new_default_auth_value := jsonb_build_object( -- From old user management 'requireOtpVerification', COALESCE((v_old_user_mgmt_value->>'requireEmailVerification')::BOOLEAN, FALSE), ... ); -- Find or create DefaultAuthenticationSettings ... + +END IF;Alternatively, if DefaultAuth should always be created, remove the
IF @OldOtpValue IS NOT NULLcheck from the SQL Server branch (line 141).
266-284: Add JSON validation before JSONB cast to prevent migration failure.The SQL Server branch validates with
ISJSON(sv.Value) = 1, but PostgreSQL directly castssv."value"::JSONB. If malformed JSON exists, this will throw an exception and abort the migration.Add validation in the WHERE clause or use a safe cast approach:
AND sv.""value"" IS NOT NULL + AND sv.""value""::TEXT ~ '^\s*\{' -- Basic JSON object check LIMIT 1;Or use exception handling in the PL/pgSQL block to gracefully handle invalid JSON.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T10:09:19.065Z
Learnt from: Tshepo1103-lab
Repo: shesha-io/shesha-framework PR: 4013
File: shesha-core/src/Shesha.NHibernate/Migrations/M20251106095300.cs:29-51
Timestamp: 2025-11-06T10:09:19.065Z
Learning: The legacy table Frwk_StoredFileVersionDownloads in the Shesha framework did not have a TenantId column. When migrating to the new frwk.stored_file_version_downloads table in migration M20251106095300, setting tenant_id to NULL for historical records is correct.
Applied to files:
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs
🔇 Additional comments (2)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (2)
1-10: LGTM!The migration class structure is correct. Using
OneWayMigrationis appropriate for data transformation migrations where rollback logic would be complex and potentially lossy.
27-31: LGTM!The exact name match
ci.Name = 'Shesha.UserManagement'correctly addresses the previous concern about broad LIKE patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs (7)
82-86: Critical: MissingANDkeyword causes SQL syntax error.Line 84 is missing the
ANDkeyword beforeci.Name = 'Shesha.Security', which will cause the migration to fail with a syntax error when executed on SQL Server.Apply this diff to fix the syntax error:
WHERE ci.item_type = 'setting-configuration' + AND ci.Name = 'Shesha.Security' - ci.Name = 'Shesha.Security' AND sv.Value IS NOT NULL AND ISJSON(sv.Value) = 1;Based on past review comments, this issue was previously flagged but not yet resolved.
141-232: Critical: MissingBEGIN...ENDblock afterIFstatement causes incorrect conditional logic.The
IFstatement on line 141 lacks an explicitBEGIN...ENDblock. In T-SQL, without a block, only the immediately following statement is conditional. Since line 143 is a comment and line 144 is a DECLARE statement, the conditional scope is ambiguous and the intended logic (lines 143-232) will not execute as a single conditional block.Apply this diff to wrap the conditional logic in a proper block:
IF @OldOtpValue IS NOT NULL AND ISJSON(@OldOtpValue) = 1 +BEGIN -- Now combine OTP and any other auth settings into DefaultAuthenticationSettings DECLARE @NewDefaultAuthValue NVARCHAR(MAX);Then add the closing
ENDbefore line 233:END END +END ");Based on past review comments, this critical issue was previously flagged but not yet resolved.
276-284: Inconsistent query pattern: PostgreSQL uses broad LIKE patterns while SQL Server uses exact match.The SQL Server branch (line 29) uses an exact match
ci.Name = 'Shesha.UserManagement', but the PostgreSQL branch uses broad LIKE patterns (ci."name" LIKE '%UserManagement%'). This inconsistency could cause different records to be selected and migrated on each platform.Apply this diff to align with SQL Server behavior:
WHERE ci.item_type = 'setting-configuration' - AND ( - ci."name" LIKE '%UserManagement%' - OR sv."value"::TEXT LIKE '%supportedRegistrationMethods%' - OR sv."value"::TEXT LIKE '%additionalRegistrationInfo%' - ) + AND ci."name" = 'Shesha.UserManagement' AND sv."value" IS NOT NULL LIMIT 1;Based on past review comments, this issue was previously flagged but not yet resolved.
384-451: Logic inconsistency: PostgreSQL creates DefaultAuthentication if ANY legacy setting exists, SQL Server only if OTP exists.The PostgreSQL branch on line 384 checks
IF v_old_otp_value IS NOT NULL OR v_old_user_mgmt_value IS NOT NULL OR v_old_security_value IS NOT NULL, creating DefaultAuthenticationSettings when ANY legacy setting exists. However, the SQL Server branch (line 141) only checksIF @OldOtpValue IS NOT NULL, creating it only when OTP settings exist. This causes inconsistent migration behavior between database platforms.To ensure consistent behavior across both platforms, either:
Option 1: Make SQL Server match PostgreSQL logic (recommended if intent is to always create DefaultAuth when any legacy settings exist):
-IF @OldOtpValue IS NOT NULL AND ISJSON(@OldOtpValue) = 1 +IF (@OldOtpValue IS NOT NULL AND ISJSON(@OldOtpValue) = 1) + OR (@OldUserManagementValue IS NOT NULL AND ISJSON(@OldUserManagementValue) = 1) + OR (@OldSecurityValue IS NOT NULL AND ISJSON(@OldSecurityValue) = 1) BEGINOption 2: Make PostgreSQL match SQL Server logic (if DefaultAuth should only be created when OTP exists):
-IF v_old_otp_value IS NOT NULL OR v_old_user_mgmt_value IS NOT NULL OR v_old_security_value IS NOT NULL THEN +IF v_old_otp_value IS NOT NULL THENBased on past review comments, this issue was previously flagged but not yet resolved.
130-139: Consider using exact name matches for OTP settings if legacy names are known.The broad LIKE patterns (
'%Otp%','%OneTimePin%') and value content patterns may match unintended settings. If the exact legacy OTP setting names are known, enumerate them explicitly for more deterministic behavior.If exact names are known, consider:
WHERE ci.item_type = 'setting-configuration' AND ( - ci.Name LIKE '%Otp%' - OR ci.Name LIKE '%OneTimePin%' - OR sv.Value LIKE '%passwordLength%' - OR sv.Value LIKE '%alphabet%' + ci.Name IN ('Shesha.Otp', 'Shesha.OneTimePin') -- use actual known names ) AND sv.Value IS NOT NULL AND ISJSON(sv.Value) = 1;Apply the same pattern to PostgreSQL (lines 370-379).
Based on past review comments, this suggestion was previously made.
147-154: Redundant null checks within guarded IF block.These
CASE WHEN @OldOtpValue IS NOT NULLchecks are redundant since they're already inside theIF @OldOtpValue IS NOT NULLblock (line 141, once the missing BEGIN is added). The condition is already guaranteed true.Simplify by removing redundant checks:
-DECLARE @PasswordLength INT = CASE WHEN @OldOtpValue IS NOT NULL THEN CAST(JSON_VALUE(@OldOtpValue, '$.passwordLength') AS INT) ELSE NULL END; -DECLARE @Alphabet NVARCHAR(100) = CASE WHEN @OldOtpValue IS NOT NULL THEN JSON_VALUE(@OldOtpValue, '$.alphabet') ELSE NULL END; -DECLARE @DefaultLifetime INT = CASE WHEN @OldOtpValue IS NOT NULL THEN CAST(JSON_VALUE(@OldOtpValue, '$.defaultLifetime') AS INT) ELSE NULL END; +DECLARE @PasswordLength INT = CAST(JSON_VALUE(@OldOtpValue, '$.passwordLength') AS INT); +DECLARE @Alphabet NVARCHAR(100) = JSON_VALUE(@OldOtpValue, '$.alphabet'); +DECLARE @DefaultLifetime INT = CAST(JSON_VALUE(@OldOtpValue, '$.defaultLifetime') AS INT);Continue the pattern for the remaining declarations (lines 150-154).
Based on past review comments, this issue was previously flagged.
268-268: Add JSON validation to prevent migration failure on malformed JSON.The PostgreSQL branch directly casts
sv."value"::JSONBwithout validation (lines 268, 320, 362), while SQL Server validates withISJSON(sv.Value) = 1before processing. If any stored value contains malformed JSON, the PostgreSQL cast will raise an exception and abort the entire migration.Add a basic JSON format validation to the WHERE clauses. For the User Management query:
WHERE ci.item_type = 'setting-configuration' AND ci."name" = 'Shesha.UserManagement' AND sv."value" IS NOT NULL + AND sv."value"::TEXT ~ '^\s*[\{\[]' -- Basic JSON format check LIMIT 1;Apply similar validation to the Security (line 335) and OTP (line 378) queries. Alternatively, wrap the DO block's main logic in a PL/pgSQL
BEGIN...EXCEPTIONhandler to catch and log JSON parse errors without aborting the migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T10:09:19.065Z
Learnt from: Tshepo1103-lab
Repo: shesha-io/shesha-framework PR: 4013
File: shesha-core/src/Shesha.NHibernate/Migrations/M20251106095300.cs:29-51
Timestamp: 2025-11-06T10:09:19.065Z
Learning: The legacy table Frwk_StoredFileVersionDownloads in the Shesha framework did not have a TenantId column. When migrating to the new frwk.stored_file_version_downloads table in migration M20251106095300, setting tenant_id to NULL for historical records is correct.
Applied to files:
shesha-core/src/Shesha.Framework/Migrations/M20251215130900.cs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…//github.com/ntuthukomsane-boxfusion/shesha-framework into ntuthuko/consolidate-authentication-settings
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
- Migration
M20251215130900fails with exception Shesha/loginform fails to fetchuserManagementSettingsShesha/loginform contains unused background imageShesha/registrationform contains commented out code
Please test it thoroughly and make sure it works as expected and clean-up all forms
3 participants
This pr addresses the following:
[#4366]
Consolidation of authentication settings by taking all the authentication settings which were previously used as individual settings into a newly created "DefaultAuthenticationSettings" object
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.