# Admin Module — Deep Entity Analysis

**Date:** 2026-03-02
**Path:** `GB5Solution/Admin/` + `GB5Framework/FrameworkBLL/User|Role|Auth|Menu|SecurityGroup/`
**Entities:** User, Role, SecurityGroup, Menu, Permission/Authorization, Module, Operation, Page, UserAccessRights
**Risk Level:** 🔴 VERY HIGH — Password hashes returned in API, no row-level auth, privilege escalation possible

---

## CRITICAL SECURITY ISSUES

### ADM-01: Password Hashes Returned in API Responses — CRITICAL

**File:** `GB5Framework/FrameworkDAL/Query/User/UserQB.cs:17`

```sql
SELECT
    U.PASSWORD           AS UserPassword,          -- HASH EXPOSED
    U.PREVIOUSPASSWORD1  AS UserPreviousPassword1,  -- EXPOSED
    U.PREVIOUSPASSWORD2  AS UserPreviousPassword2,  -- EXPOSED
    U.PREVIOUSPASSWORD3  AS UserPreviousPassword3,  -- EXPOSED
    U.DIGITALFILEPASSWORD AS UserDigitalFilePassword -- EXPOSED
FROM MUSER U
WHERE U.USERID = @UserId
```

**Impact:** Any caller of `GET /User/GetUser` receives the user's password hash and 3 previous password hashes. If hashes are MD5/SHA1 (common in legacy ERPs) they can be cracked offline. If the response is cached, logged by API gateways (Dapr, nginx), or appears in OpenTelemetry traces, passwords propagate throughout the infrastructure.

**Fix:**
```sql
-- NEVER select password in general-purpose queries
SELECT
    U.USERID, U.USERCODE, U.USERNAME, U.USERPRIMARYMAIL,
    U.ROLEID, U.SECURITYGROUPID, U.USERVALIDFROM, U.USERVALIDTO
    -- PASSWORD fields completely omitted
FROM MUSER U
WHERE U.USERID = @UserId

-- Password only retrieved in dedicated auth-only method:
-- UserDAL.AuthenticateAsync(string userCode, string passwordHash, LoginDTO)
```

---

### ADM-02: All User/Role/Auth Endpoints — AllowAnonymous

**Severity:** 🔴 CRITICAL
**Files:** All endpoint files under `GB5Framework/FrameworkSL/Endpoints/User/`, `Role/`, `UserAccessRights/`

```csharp
// GetUser.cs:21, SaveUser.cs:22, DeleteUser.cs, SaveRole.cs:23, etc.
public override void Configure()
{
    Get("/User/GetUser");
    AllowAnonymous(); // No auth on user management!
}
```

**Exposed without authentication:**
- `GET /User/GetUser` — returns password hashes (ADM-01 + this = complete credential theft)
- `POST /User/SaveUser` — create/modify ANY user including admins
- `DELETE /User/DeleteUser` — delete any user
- `POST /Role/SaveRole` — create admin roles
- `POST /UserAccessRights/SaveUserAccessRights` — grant any permission to any user

**Fix:** Remove `AllowAnonymous()` from ALL these endpoints. Add role-based access:
```csharp
public override void Configure()
{
    Get("/User/GetUser");
    // No AllowAnonymous — JWT enforced by default
    Roles("SystemAdmin", "UserAdmin");
}
```

---

### ADM-03: No Row-Level Authorization — User A Can See User B's Data

**Severity:** 🔴 CRITICAL
**File:** `GB5Framework/FrameworkBLL/User/UserBLL.cs:32–42`

```csharp
public async Task<string> GetUser(int UserId, LoginDTO LoginDTO)
{
    try
    {
        return await _UserDAL.GetUser(UserId, LoginDTO);
        // MISSING: Is LoginDTO.UserId == UserId? Or is caller an Admin?
    }
    catch (Exception) { throw; }
}
```

**Impact:** Any authenticated user (once authentication is added) can retrieve any other user's profile including salary information, email, phone, access rights, by simply varying the `UserId` parameter.

**Fix:**
```csharp
public async Task<string> GetUser(int userId, LoginDTO login)
{
    // Allow: viewing own profile, or admin viewing any profile
    bool isSelf  = login.UserId == userId;
    bool isAdmin = login.Roles.Intersect(new[] { "SystemAdmin", "UserAdmin" }).Any();

    if (!isSelf && !isAdmin)
        throw new UnauthorizedAccessException("Access denied: cannot view other user profiles");

    return await _UserDAL.GetUser(userId, login);
}
```

---

### ADM-04: Role Deletion Without Checking Active Assignments — CRITICAL DATA INTEGRITY

**Severity:** 🟠 HIGH
**File:** `GB5Framework/FrameworkDAL/Query/Role/RoleQB.cs:80–81`

```sql
DELETE FROM MROLE WHERE ROLEID = @RoleId
-- Missing: Check MUSER.ROLEID references
-- Missing: Check MUSERACCESSRIGHTS
-- Missing: Check RoleVsMenu assignments
```

**Impact:** Deleting a role that has 500 users assigned breaks their authentication. All those users will have `RoleId` pointing to a deleted row — FK violations or NULL role resolution → access denied for all affected users.

**Fix (pre-delete validation):**
```csharp
public async Task DeleteRole(int roleId, LoginDTO login)
{
    var userCount = await _db.ExecuteScalarAsync<int>(
        "SELECT COUNT(1) FROM MUSER WHERE ROLEID = @RoleId AND USERSTATUS = 1",
        new { RoleId = roleId });

    if (userCount > 0)
        throw new BusinessException($"Cannot delete role: {userCount} active users are assigned to it");

    await _db.ExecuteAsync("DELETE FROM MUSERVSMENU WHERE ROLEID = @RoleId", new { roleId });
    await _db.ExecuteAsync("DELETE FROM MROLE WHERE ROLEID = @RoleId", new { roleId });
}
```

---

### ADM-05: User+AccessRights Not Created Atomically — Orphan Users Possible

**Severity:** 🟠 HIGH
**File:** `GB5Solution/Admin/AdminBLL/UserAccessRights/UserAccessRightsBLL.cs:56–90`

```csharp
// Step 1: API client calls SaveUser
await _userBLL.SaveUser(userDTO, login); // SUCCESS

// Step 2: API client must separately call SaveUserAccessRights
await _userAccessRightsBLL.SaveUserAccessRights(uar, login); // If this fails:
// → User exists but has NO access rights
// → User can log in but cannot access any screen
// → Admin must manually assign rights
```

**Fix:** Orchestrate in a single BLL method with transaction:
```csharp
public async Task CreateUserWithAccessRights(UserDTO user, UserAccessRightsDTO uar, LoginDTO login)
{
    await using var scope = await _queryExecutor.BeginTransactionAsync(login);
    try
    {
        await _userDAL.SaveUser(user, login, scope.Transaction);
        await _userAccessRightsDAL.SaveUserAccessRights(uar, login, scope.Transaction);
        await scope.Transaction.CommitAsync();
    }
    catch { await scope.Transaction.RollbackAsync(); throw; }
}
```

---

## HIGH SEVERITY ISSUES

### ADM-06: Duplicate UserCode/RoleCode Not Prevented

**File:** `GB5Framework/FrameworkBLL/User/UserBLL.cs:51–52`

```csharp
await _Validation.NotEmpty(UserDTO.UserCode, nameof(UserDTO.UserCode));
await _Validation.NotEmpty(UserDTO.UserName, nameof(UserDTO.UserName));
// MISSING uniqueness check
```

Two users with same `UserCode` = authentication ambiguity. Which user logs in? Last-insert-wins or first-match-wins?

**Fix:**
```csharp
var existing = await _userDAL.FindByCode(userDTO.UserCode, login);
if (existing != null && existing.UserId != userDTO.UserId)
    throw new ValidationException($"User code '{userDTO.UserCode}' is already taken");
```

---

### ADM-07: ValidationException Wrapped as Generic Exception — Type Safety Lost

**File:** `GB5Framework/FrameworkBLL/User/UserBLL.cs:73–75`

```csharp
catch (ValidationException vex)
{
    throw new Exception(vex.Message); // Loses type info, stack trace, inner exception
}
```

Endpoint cannot catch `ValidationException` specifically — falls through to generic 500 handler.

**Fix:**
```csharp
catch (ValidationException) { throw; } // Re-throw preserving type
```

---

### ADM-08: Password Reuse History Tracked But Not Enforced

**File:** `GB5Framework/FrameworkDAL/DTO/User/UserDTO.cs:49–52`

```csharp
private string userpreviouspassword1; // Stored
private string userpreviouspassword2; // Stored
private string userpreviouspassword3; // Stored
// No code checks: newPassword != any of the 3 previous
```

**Fix:**
```csharp
// In ChangePassword BLL method:
var hashed = HashPassword(newPassword);
if (hashed == user.UserPreviousPassword1 ||
    hashed == user.UserPreviousPassword2 ||
    hashed == user.UserPreviousPassword3)
{
    throw new ValidationException("Password cannot be the same as any of the last 3 passwords");
}
// Rotate: PreviousPassword3 = PreviousPassword2 → ...
user.UserPreviousPassword3 = user.UserPreviousPassword2;
user.UserPreviousPassword2 = user.UserPreviousPassword1;
user.UserPreviousPassword1 = user.UserPassword;
user.UserPassword = hashed;
```

---

### ADM-09: Password Expiration Tracked But Not Validated at Login

**File:** `GB5Framework/FrameworkDAL/DTO/UserDTO.cs:52`

```csharp
private DateTime userpasswordchangedon = Convert.ToDateTime("01-Jan-1899"); // Sentinel
// No check during authentication: Has password expired?
```

**Fix:** In `AuthenticateUser` BLL:
```csharp
var policy = await _parameterBLL.GetPasswordPolicy(login);
if ((DateTime.UtcNow - user.UserPasswordChangedOn).TotalDays > policy.MaxPasswordAgeDays)
    throw new PasswordExpiredException("Password has expired. Please change your password.");
```

---

### ADM-10: No Audit Trail on Period-Sensitive Operations

**File:** `GB5Framework/FrameworkDAL/CustomCode/Role/RoleDAL.cs:46–49`

```csharp
RoleDTO.RoleCreatedById = LoginDTO.UserId;
RoleDTO.RoleCreatedOn = DateTime.UtcNow;
// SET only — no log entry written to MAUDITLOG or equivalent
```

Role creation/deletion/modification doesn't create an audit log record. For SOX/ISO 27001 compliance, who created a role and when must be trackable in an audit table, not just in the entity row itself (which can be overwritten).

---

### ADM-11: N+1 Query in MenuDAL Report Criteria Loading

**File:** `GB5Framework/FrameworkDAL/CustomCode/Menu/MenuDAL.cs:146–200`

```csharp
// Stream all results into list
await foreach (var item in _queryExecutor.StreamAsync<ReportMenuLoadingFlatQueryNewDTO>(...))
    ReportMenuLoadingFlatQueryNewDTOs.Add(item);

// Then loop and do string operations per row
for (int i = 0; i < ReportMenuLoadingFlatQueryNewDTOs.Count; i++)
{
    string[] TempSplit = inputvalue.Split(','); // String ops in C# instead of SQL
}
```

---

## MEDIUM SEVERITY ISSUES

### ADM-12: Magic Sentinel Date "01-Jan-1899" in User Entity

**File:** `GB5Framework/FrameworkDAL/CustomCode/User/UserDAL.cs:268–283`

```csharp
if (noOfReadPeriod == 0)
{
    user.ApplicablePeriodFrom = "01-Jan-1899"; // Magic date — no documentation
}
```

Same pattern as BIZTransactionType (see `03_Solution_Modules_Analysis.md`). Extract as named constant with XML doc comment explaining the business meaning.

---

### ADM-13: SecurityQuestion + SecurityAnswer Not Paired Consistently

**File:** `GB5Framework/FrameworkDAL/DTO/User/UserDTO.cs:60–66`

```csharp
private int securityquestionid = -1;    // -1 = not set
private string usersecurityanswer = "NONE"; // Default "NONE"
// No validation: if question set → answer must not be "NONE"
// No validation: if question -1 → answer must be "NONE"
```

---

### ADM-14: Email/Phone Format Not Validated

**File:** `GB5Framework/FrameworkDAL/DTO/User/UserDTO.cs:32–34`

```csharp
private string userprimarymail;    // No email format validation
private string userprimarymobile;  // No phone format validation
```

Invalid email → MFA OTP never delivered → user locked out. No regex check in BLL.

---

### ADM-15: User ValidFrom/ValidTo Date Range Not Validated

**File:** `GB5Framework/FrameworkBLL/User/UserBLL.cs`

```csharp
private DateTime uservalidfrom = DateTime.UtcNow;
private DateTime uservalidto = DateTime.UtcNow;
// No: if (ValidTo <= ValidFrom) throw
```

---

### ADM-16: Audit Fields Set Without UserId Validation

**File:** `GB5Framework/FrameworkDAL/CustomCode/Role/RoleDAL.cs:46–49`

```csharp
RoleDTO.RoleCreatedById = LoginDTO.UserId; // Could be 0 or -1 if JWT parsing fails
```

If `LoginDTO.UserId` is invalid (JWT decode failure returns default 0), audit trail shows "User 0" modified the role — meaningless.

**Fix:**
```csharp
if (LoginDTO.UserId <= 0)
    throw new InvalidOperationException("Invalid session: UserId not resolved");
```

---

## Summary Table

| # | Issue | Severity | Category | File | Priority |
|---|-------|----------|----------|------|----------|
| ADM-01 | Password hashes returned in API | 🔴 CRITICAL | Security | UserQB.cs | P0 |
| ADM-02 | AllowAnonymous on user/role mgmt | 🔴 CRITICAL | Security | All endpoints | P0 |
| ADM-03 | No row-level authorization | 🔴 CRITICAL | Security | UserBLL.cs | P0 |
| ADM-04 | Role delete without checking users | 🟠 HIGH | Data Integrity | RoleQB.cs | P1 |
| ADM-05 | User+Rights not created atomically | 🟠 HIGH | Data Integrity | UserAccessRightsBLL.cs | P1 |
| ADM-06 | Duplicate UserCode allowed | 🟠 HIGH | Business Logic | UserBLL.cs | P1 |
| ADM-07 | ValidationException swallowed | 🟠 HIGH | Error Handling | UserBLL.cs:73–75 | P1 |
| ADM-08 | Password reuse not enforced | 🟠 HIGH | Compliance | UserDTO.cs | P1 |
| ADM-09 | Password expiry not checked at login | 🟠 HIGH | Compliance | UserDTO.cs | P1 |
| ADM-10 | No audit log on role operations | 🟠 HIGH | Compliance | RoleDAL.cs | P1 |
| ADM-11 | N+1 in menu report criteria | 🟡 MEDIUM | Performance | MenuDAL.cs | P2 |
| ADM-12 | Magic sentinel date "01-Jan-1899" | 🟡 MEDIUM | Code Quality | UserDAL.cs | P2 |
| ADM-13 | Security question not paired | 🟡 MEDIUM | Validation | UserDTO.cs | P2 |
| ADM-14 | No email/phone format validation | 🟡 MEDIUM | Validation | UserDTO.cs | P2 |
| ADM-15 | Date range not validated | 🟡 MEDIUM | Validation | UserBLL.cs | P2 |
| ADM-16 | Audit fields with unvalidated UserId | 🟡 MEDIUM | Audit | RoleDAL.cs | P2 |

---

## Remediation Priority

### P0 — Before Any External Access
1. **ADM-01:** Remove password fields from all SELECT queries (user-facing)
2. **ADM-02:** Remove `AllowAnonymous()` from all user/role/auth endpoints
3. **ADM-03:** Add ownership + admin check before returning any user data

### P1 — Sprint 1
4. **ADM-04:** Add pre-delete user-count check for role deletion
5. **ADM-05:** Atomic user creation with access rights in one transaction
6. **ADM-06:** Unique code validation in SaveUser/SaveRole
7. **ADM-08/ADM-09:** Enforce password reuse and expiry policies
8. **ADM-10:** Write to audit log on all role/permission changes

### P2 — Sprint 2
9. **ADM-13/ADM-14/ADM-15:** Add missing field validations
10. **ADM-16:** Validate UserId in LoginDTO before writing audit fields