Add RequireChangePassword property to user-related models and update … #4460
+284
−154
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…authentication logic
Contributor
WalkthroughAdds a RequireChangePassword flag across authentication, session and user-management DTOs/services and persistence; annotates TokenAuthController with API attributes; introduces a migration to mark admin accounts using the default password; updates TypeScript interfaces and embeds a .shaconfig resource. Changes
Sequence Diagram(s)(Skipped — changes are primarily DTO/property additions, controller annotation, and a migration; control flow remains largely unchanged.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@shesha-core/src/Shesha.Application/Authorization/TokenAuthController.cs`:
- Around line 71-99: The current AuthenticateAsync returns a redirect for
incomplete registrations before validating credentials, allowing account
enumeration and bypassing authentication; change the flow in AuthenticateAsync
to validate credentials first by calling GetLoginResultAsync (with
model.UserNameOrEmailAddress, model.Password, model.IMEI,
GetTenancyNameOrNull()) and ensure the login succeeded before returning a
RedirectNoAuth for incomplete registrations, or require/validate a one-time
token in the model if you must allow pre-auth redirects; also null-check
registration.AdditionalRegistrationInfoForm before reading Module/Name to avoid
a NullReferenceException and only populate RedirectModule/RedirectForm when that
object is present.
In
`@shesha-core/src/Shesha.Application/UserManagements/UserManagementAppService.cs`:
- Around line 128-143: The code creates a ShaUserRegistration with IsComplete =
registrationSettings.AdditionalRegistrationInfo ? false : true even when
AdditionalRegistrationInfoForm is null, causing null dereferences later; update
the creation logic for ShaUserRegistration so that if
registrationSettings.AdditionalRegistrationInfo is true but
registrationSettings.AdditionalRegistrationInfoFormModule or
.AdditionalRegistrationInfoFormName are missing, either validate and throw a
descriptive exception or set IsComplete = true (treat as no extra form), and
only assign AdditionalRegistrationInfoForm when both module and name are
non-empty; adjust the block that builds ShaUserRegistration (referencing
ShaUserRegistration, AdditionalRegistrationInfo, AdditionalRegistrationInfoForm,
registrationSettings, and IsComplete) accordingly so you never persist a record
with IsComplete=false while AdditionalRegistrationInfoForm==null.
In `@shesha-core/src/Shesha.Framework/Migrations/M20250127103000.cs`:
- Around line 67-85: The migration currently swallows exceptions from
passwordHasher.VerifyHashedPassword (called with dummyUser and hashedPassword)
which can leave shouldUpdate false unnoticed; update the catch to catch
Exception ex and log the failure (include ex.ToString() and a safe identifier
for the affected user/hash but do NOT log plaintext passwords) using the
migration's logger or Console.Error, and explicitly leave shouldUpdate false so
behavior is deterministic; locate the try/catch around
passwordHasher.VerifyHashedPassword and replace the empty catch with logging of
the exception and a brief message mentioning RequireChangePassword was not set
for that admin.
In
`@shesha-core/src/Shesha.Framework/Models/TokenAuth/AuthenticateResultModel.cs`:
- Around line 21-24: Rename the AuthenticateResultModel property
RequiredChangePassword to RequireChangePassword to match other DTOs and the user
model, update any usages/references accordingly, and adjust the mapping in
TokenAuthController where an AuthenticateResultModel is constructed (ensure the
controller assigns the user's RequireChangePassword value to the renamed
property); search for AuthenticateResultModel, RequiredChangePassword, and the
TokenAuthController mapping code to update all references to the new
RequireChangePassword identifier.
shesha-core/src/Shesha.Application/UserManagements/UserManagementAppService.cs
Show resolved
Hide resolved
shesha-core/src/Shesha.Framework/Models/TokenAuth/AuthenticateResultModel.cs
Outdated
Show resolved
Hide resolved
- Renamed `RequiredChangePassword` to `RequireChangePassword` in `AuthenticateResultModel` and `TokenAuthController` for consistency. - Enhanced error handling in migration `M20250127103000` by logging exceptions when password verification fails, providing clearer feedback for operators.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-core/src/Shesha.Application/Authorization/TokenAuthController.cs (1)
28-31: Security issue:SignOffAsyncrequires authentication but lacks[Authorize]with class-level[AllowAnonymous].The class has
[AllowAnonymous]at line 30, which makes all methods anonymous by default.SignOffAsync(lines 135-143) accessesUser.GetTokenId()andUser.GetTokenExpirationDate()without an explicit[Authorize]attribute, allowing unauthenticated requests to call this method. While the method may fail gracefully, it should explicitly require authentication.Add
[Authorize]toSignOffAsync:Proposed fix
[HttpPost] + [Authorize] public async Task<bool> SignOffAsync() {
🤖 Fix all issues with AI agents
In `@shesha-core/src/Shesha.Framework/Migrations/M20250127103000.cs`:
- Around line 57-61: Remove the unused local variable userId declared in the
reader.Read() block of the M20250127103000 migration (the line "var userId =
reader.GetInt64(0);"); delete that declaration so only the needed values
(userName from index 1 and hashedPassword from index 2) are read—if the ID must
be consumed for side-effects instead of used, replace with a discard assignment
like "_ = reader.GetInt64(0);" otherwise simply remove the userId line.
Contributor
Author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@shesha-core/src/Shesha.Framework/Migrations/M20250127103000.cs`:
- Around line 69-74: Replace the hardcoded literal "123qwe" used in
passwordHasher.VerifyHashedPassword(dummyUser, hashedPassword, "123qwe") with a
shared constant; add or reuse a public constant like DefaultPassword in the
existing HostRoleAndUserBuilder (or a small shared Defaults/Constants class) and
reference that constant here instead of the string literal to ensure a single
source of truth for the default password.
- Line 89: In the migration class M20250127103000 remove the redundant explicit
call to reader.Close(): the IDataReader (variable reader) is already wrapped in
a using statement, so delete the reader.Close() line to avoid
unnecessary/duplicate disposal and rely on the using block to dispose the
reader.
- Around line 100-107: The UPDATE statement in updateCommand.CommandText
currently only filters by {idColumn} = 1; change it to include the same
defensive conditions used in the earlier SELECT (IsDeleted = false AND Password
IS NOT NULL) so the update on {tableName} for {requireChangePasswordColumn} =
{boolValue} only applies when that row is not deleted and has a password; modify
the WHERE clause in the updateCommand.CommandText to include those conditions
alongside {idColumn} = 1.
- Around line 36-53: The migration currently queries the admin user by hardcoded
Id (idColumn = 1) which can target the wrong record; update the SELECT built in
selectCommand.CommandText to use the username lookup instead by replacing the
WHERE clause to use {userNameColumn} = 'admin' (keeping the existing
isDeletedCheck and {passwordColumn} IS NOT NULL checks and honoring SQL Server
vs PostgreSQL quoting via the userNameColumn variable); adjust no other
logic—only change the WHERE condition so selectCommand targets the 'admin'
username.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…authentication logic
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.