# Authentication Module — Deep Code Analysis
**Scope:** AuthenticationBLL, AuthenticationDAL, LoginEncryption, PasswordEncryption, KeyCloakService, SSOService,
PasswordPolicyBLL/DAL/QB, UserLoginQB, GetUserLoginDetail endpoint, OTP stack, AuthenticationDTO, UserQB
**Date:** 2026-03-03

---

## Issue Index

| # | Severity | Category | Location | Summary |
|---|----------|----------|----------|---------|
| 1 | CRITICAL | Security/VAPT | `LoginEncryption.cs:12-13` | Hardcoded AES key + static IV — symmetrically breakable |
| 2 | CRITICAL | Security/VAPT | `KeyCloakService.cs:187` | Hardcoded AES encryption key in session token generation |
| 3 | CRITICAL | Security/VAPT | `KeyCloakService.cs:150` | TLS certificate validation disabled (`DangerousAcceptAnyServerCertificateValidator`) |
| 4 | CRITICAL | Security/VAPT | `KeyCloakService.cs:95-96` | OAuth `clientSecret` passed as query parameter — logged in plain text |
| 5 | CRITICAL | Security/VAPT | `AuthenticationBLL.cs:845-848` | Password verification commented-out in Gmail/OAuth login path |
| 6 | CRITICAL | Security/VAPT | `AuthenticationBLL.cs:1311-1314` | MD5 used for password digest encoding — cryptographically broken |
| 7 | CRITICAL | Security/VAPT | `AuthenticationBLL.cs:971,1213` | TOTP/MFA secret key returned to client in login response |
| 8 | CRITICAL | Security/VAPT | `UserQB.cs:131` | `GET_USER` (SQL Server) has no tenant filter — IDOR across tenants |
| 9 | CRITICAL | Security/VAPT | `UserLoginQB.cs:45,71` | `GET_USERLOGINID_SQL` and `GET_USERLOGINCODE_SQL` have no tenant filter |
| 10 | HIGH | Security | `AuthenticationBLL.cs:1285-1305` | Session validity token is plain Base64 — no HMAC, no signature |
| 11 | HIGH | Security | `AuthenticationBLL.cs:1293-1297` | Session token concatenation without delimiters — token confusion attack |
| 12 | HIGH | Security | `UserQB.cs:346-347` | Oracle and MySQL auth queries are empty semicolons — silent auth failure |
| 13 | HIGH | Security | `UserLoginQB.cs:177-184` | All Oracle/MySQL login detail queries are empty strings |
| 14 | HIGH | Security | `GetUserLoginDetail.cs:21` | `AllowAnonymous()` on user login detail endpoint |
| 15 | HIGH | Security | `GetUserLoginDetail.cs:38` | `ex.Message` leaked in error response |
| 16 | HIGH | Security/VAPT | `PasswordPolicyQB.cs:41` | Password policy query has no tenant filter |
| 17 | HIGH | Best Practice | `KeyCloakService.cs:27` | `KeyCloakService` is MVC `ControllerBase` — violates FastEndpoints rule |
| 18 | HIGH | Best Practice | `SSOService.cs:15` | `SSOService` is MVC `ControllerBase` — violates FastEndpoints rule |
| 19 | HIGH | Security | `SSOService.cs:21` | `IMemoryCache` for nonce storage — does not work in multi-instance deployments |
| 20 | HIGH | Security | `PasswordEncryption.cs` | `Encrypt()` is reversible AES-GCM — passwords must be one-way hashed |
| 21 | MEDIUM | Best Practice | `AuthenticationBLL.cs:139,411,478,810,933,1012,1044,1192,1235,1261` | `DateTime.Now` used throughout auth flow instead of `DateTime.UtcNow` |
| 22 | MEDIUM | Best Practice | `AuthenticationBLL.cs` | 90%+ code duplication across `GetLoginDetail`, `GetLoginDetailByEMail`, `GetLoginDetailViaKeycloak` |
| 23 | MEDIUM | Security | `AuthenticationBLL.cs` | 500+ lines of commented-out dead code — misleads security reviewers |
| 24 | MEDIUM | Security | `AuthenticationDTO.cs` | `UserPassword`, `PreviousPassword1/2/3`, `MFAUserSecretKey` all in the same DTO |
| 25 | MEDIUM | Best Practice | `AuthenticationBLL.cs:1347` | `throw error` re-throw loses stack trace in `EncryptPassword` |
| 26 | MEDIUM | Best Practice | `AuthenticationBLL.cs:133,406,472,732,880` | Dead null-check after `GetUser` which already throws on null |
| 27 | MEDIUM | Performance | `AuthenticationBLL.cs` | No `CancellationToken` in any BLL method |
| 28 | MEDIUM | Best Practice | `AuthenticationBLL.cs:1019-1026` | Dead if/else — identical branches both assign `AuthenticationDTO.UserCode = UserCode` |
| 29 | MEDIUM | Best Practice | `AuthenticationBLL.cs:90-103` | Repeated duplicate field assignments (`ServerId`, `ServerMachineName` etc. set twice) |
| 30 | MEDIUM | Security | `SSOService.cs:90` | Auth URL logged at Info level including nonce — nonce in server logs |
| 31 | LOW | Best Practice | `AuthenticationBLL.cs:52-54` | `LoggingDTO LoggingDTO = new LoggingDTO()` — instantiated but never used |
| 32 | LOW | Best Practice | `AuthenticationBLL.cs:52` | `UserDTO UserDTO = new()` — declared but never used (shadowed by `LoginUserDTO`) |
| 33 | LOW | Best Practice | `AuthenticationBLL.cs:138` | `new System.TimeSpan(0, 0, 0)` — should be `TimeSpan.Zero` |
| 34 | LOW | Best Practice | `UserLoginQB.cs:9` | `UserLoginQB` is a `class`, not a `static class` — inconsistent with all other QBs |
| 35 | LOW | Best Practice | `PasswordPolicyDAL.cs:35` | `finally { json = null; }` — no-op (systemic anti-pattern) |
| 36 | LOW | Security | `UserLoginQB.cs:82-83` | `GET_ALL_PERIOD_SQL` returns all periods with no tenant filter |

---

## Detailed Findings

---

### CRITICAL-1 — Hardcoded AES Key + Static IV in LoginEncryption
**File:** [GB5Shared/EncryptionHelper/LoginEncryption.cs:12-13](GB5Shared/EncryptionHelper/LoginEncryption.cs#L12-L13)

```csharp
private static readonly byte[] Key = Encoding.UTF8.GetBytes("0123456789ABCDEF0123456789ABCDEF"); // 32 bytes
private static readonly byte[] IV  = Encoding.UTF8.GetBytes("1234567890ABCDEF");                 // 16 bytes (STATIC)
```

**Issues:**
- Key is hardcoded in source code — anyone with repo access possesses the decryption key
- IV is **static** (never rotated per encryption call) — AES-CBC with a fixed IV means identical plaintexts always produce identical ciphertexts (allows frequency analysis and pattern attacks)
- Sequential ASCII key (`0123...F`) is trivially guessable

**Impact:** Every value encrypted by `LoginEncryption.Encrypt()` (used for credentials in transit per CLAUDE.md) is permanently compromised. An attacker with the source can decrypt any captured `LoginDTO` credential blob.

**Fix:** Remove all hardcoded key material. Load key from HashiCorp Vault. Generate a cryptographically random IV **per encryption call** using `RandomNumberGenerator.GetBytes(16)`.

---

### CRITICAL-2 — Hardcoded AES Key for SSO Session Tokens
**File:** [GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs:187](GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs#L187)

```csharp
string encrypted = EncryptString(combined, "12345678901234567890123456789012");
```

**Issues:**
- 32-character literal AES key hardcoded at the call site — identical severity to CRITICAL-1
- Encrypts SSO session data (`realm|clientId|keycloakHost|username`) that is then sent to the browser as a `Token` query parameter
- Same key used for every tenant in every deployment

**Impact:** Any holder of the source code can forge or decrypt all SSO session tokens, enabling full session impersonation.

---

### CRITICAL-3 — TLS Certificate Validation Disabled for Keycloak Communication
**File:** [GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs:148-151](GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs#L148-L151)

```csharp
using (var client = new HttpClient(new HttpClientHandler
{
    ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
}))
```

**Issues:**
- `DangerousAcceptAnyServerCertificateValidator` accepts any certificate, including self-signed, expired, or adversarially controlled ones
- Entire OAuth 2.0 token exchange (authorization code → access token → id_token → userinfo) takes place over this unvalidated channel
- The `DangerousAccept...` prefix in .NET is an explicit red flag documented by Microsoft as production-unsafe

**Impact:** MitM attacker between the application server and Keycloak server can intercept authorization codes, access tokens, and user information.

**Fix:** Remove this handler entirely and use the standard `IHttpClientFactory`-managed `HttpClient` which validates certificates by default. If Keycloak uses an internal CA, configure the CA certificate properly in the trust store.

---

### CRITICAL-4 — OAuth Client Secret Passed as Query Parameter
**File:** [GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs:91-127](GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs#L91-L127)

```csharp
[HttpGet("UserLoginvalidation")]
public IActionResult RedirectToKeycloakLogin(
    [FromQuery] string clientSecret,   // ← secret in GET parameter
    ...)
{
    string state = Uri.EscapeDataString(
        $"{realm}|{clientId}|{clientSecret}|{BackendCallback}|{FECallBackUrl}"
    );  // ← secret embedded in OAuth state parameter
```

**Issues:**
- `clientSecret` arrives as a GET query parameter — appears verbatim in:
  - Web server access logs (IIS, nginx, nginx-ingress, load balancer)
  - Keycloak authorization server logs (in the `state` parameter)
  - Browser URL history and DevTools Network tab
  - HTTP Referer header on subsequent navigations
  - APM/observability traces
- The secret is then embedded into the `state` parameter sent to Keycloak and returned in the callback URL
- At `UserLoginRedirectCallback`, `clientSecret = parts[2]` is extracted from the `state` and used to acquire access tokens

**Impact:** Keycloak `clientSecret` is permanently logged across multiple systems. Any operator, log aggregator, or proxy operator has persistent access to the credential.

**Fix:** Store `clientSecret` in server-side session or signed cookies at the start of the flow. Never include credentials in URLs or `state` parameters.

---

### CRITICAL-5 — Password Verification Commented Out for Gmail / OAuth Login
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:845-848](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L845-L848)

```csharp
string password = Encode(AuthenticationDTO, LoginUserDTO.UserPassword);
//if (AuthenticationDTO.Response != password)    //password check
//{
//    throw new MethodNotAllowedException("Incorrect password..");
//}
```

Compare with the **standard** login path in `GetLoginDetail` (lines 1069-1072):
```csharp
string password = Encode(AuthenticationDTO, LoginUserDTO.UserPassword);
if (AuthenticationDTO.Response != password)    //password check
{
    throw new MethodNotAllowedException("Incorrect password..");
}
```

**Impact:** The Gmail login path (`AuthenticateUserByGmail`) and Keycloak path (`GetLoginDetailViaKeycloak`) skip password verification entirely. Any caller who supplies a valid email address or username for an existing user can authenticate without any credential, bypassing all password checks for those login flows.

---

### CRITICAL-6 — MD5 Used for Password Digest Encoding
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:1307-1321](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L1307-L1321)

```csharp
public string Encode(AuthenticationDTO DigestHeader, string UserCode, string Password)
{
    MD5Encoder md5Encoder = new MD5Encoder();
    string ha1 = md5Encoder.Encode(UserCode + DigestHeader.Realm + Password);  // MD5
    string ha2 = md5Encoder.Encode(DigestHeader.Method + DigestHeader.Uri);    // MD5
    string ha3 = md5Encoder.Encode(ha1 + DigestHeader.Nonce + ... + ha2);      // MD5
    return ha3;
}
```

**Issues:**
- MD5 has been cryptographically broken since 1996 (collision attacks) and since 2004 (practical preimage)
- This is HTTP Digest Auth (RFC 2617) using MD5 — superseded by SHA-256 variants in RFC 7616
- MD5 digests for common username:realm:password combinations are publicly available in rainbow tables
- The `Realm` and `Nonce` values visible in `AuthenticationDTO` reduce brute-force resistance

**Impact:** Credential hashes sent over the network can be cracked offline even without database access.

---

### CRITICAL-7 — TOTP / MFA Secret Key Returned to Client in Login Response
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:967-972](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L967-L972)

```csharp
AuthenticationDTO.MFAUserSetting    = LoginUserDTO.CheckUserMFAUserSetting;
AuthenticationDTO.MFAUserAccountId  = LoginUserDTO.UserMFAUserAccountId;
AuthenticationDTO.MFAUserSecretKey  = LoginUserDTO.UserMFAUserSecretKey;   // ← TOTP seed
AuthenticationDTO.MFAQRCode         = LoginUserDTO.UserMFAQRCode;
AuthenticationDTO.ShowQRCode        = LoginUserDTO.CheckUserShowQRCode;
```

**Issues:**
- `MFAUserSecretKey` is the TOTP seed (RFC 6238 shared secret) used by Google Authenticator / TOTP apps
- It is placed in `AuthenticationDTO` which is serialized and returned in the HTTP login response
- Any attacker who captures the login response (or the token it produces) has the TOTP seed and can generate valid TOTP codes indefinitely, completely defeating MFA

**Fix:** The TOTP secret must never leave the server. The server should validate the TOTP code server-side before granting the session, then discard the secret from the response DTO.

---

### CRITICAL-8 — `GET_USER` (SQL Server) Has No Tenant Filter — Cross-Tenant IDOR
**File:** [GB5Framework/FrameworkDAL/Query/User/UserQB.cs:131](GB5Framework/FrameworkDAL/Query/User/UserQB.cs#L131)

```sql
-- SQL Server variant
WHERE U.USERID = @userid;
-- No CLIENTID / DATABASENAME filter!
```

Note: The **PostgreSQL** variant (`GET_USER_PG`, line 342-344) correctly includes `AND a.tenantid = @clientid`, showing the fix is known but not applied to SQL Server.

**Impact:** By guessing a user ID from another tenant, an authenticated SQL Server tenant user can retrieve full user records — including `UserPassword`, `UserPreviousPassword1/2/3`, `SecurityAnswer`, and `MFAUserSecretKey` — from any other tenant.

---

### CRITICAL-9 — `UserLoginQB` SQL Server Queries Have No Tenant Filter
**File:** [GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs:45,71](GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs#L45)

```sql
-- GET_USERLOGINID_SQL
WHERE u.USERID = @userid;             -- no CLIENTID

-- GET_USERLOGINCODE_SQL
WHERE u.USERCODE = @UserCode;         -- no CLIENTID
```

**Impact:** Exposes org unit logos, party branch details, period dates, and user thumbnails for users from any tenant by providing their ID or code. The `UserCode` variant is particularly dangerous — user codes across tenants may overlap (e.g., "ADMIN", "SYSTEM").

---

### HIGH-10 — Session Validity Token Is Plain Base64 — No Cryptographic Protection
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:1285-1305](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L1285-L1305)

```csharp
public string CreateValidityOfSession(AuthenticationDTO dto, string BaseUri, string MachineIp)
{
    ValidationSessionData = dto.UserId + "" + dto.ServerId + "" + dto.RoleId + "" + dto.AppId
                          + "" + dto.DeviceId + "" + dto.ValidToTime + "" + BaseUri + "" + MachineIp
                          + dto.DeveloperId + "" + dto.DeveloperAccessKeyId;
    OutPut = Base64Encode(ValidationSessionData);  // ← NOT a signature, NOT encrypted
    return OutPut;
}
```

**Issues:**
- Base64 is encoding, not encryption — entirely reversible without any secret
- No HMAC or signature means the token can be altered and re-encoded by a client
- If client can modify `UserId`, `RoleId`, `ServerId` in the token and the server trusts it, privilege escalation is possible
- `ValidToTime` is also Base64-encoded `DateTime.Now.ToString()` — trivially decodable

**Fix:** Session validity must be protected with `HMAC-SHA256` using a server-side secret loaded from Vault, or replaced with ASP.NET Core's built-in `DataProtectionProvider`.

---

### HIGH-11 — Token Confusion Attack: String Concatenation Without Delimiters
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:1293](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L1293)

```csharp
// UserId=1, ServerId=23 → "123..."
// UserId=12, ServerId=3  → "123..."  (same token!)
ValidationSessionData = dto.UserId + "" + dto.ServerId + "" + dto.RoleId + ...
```

Because integer fields are concatenated with empty strings (`+ ""`) and no separators, different combinations of (UserId, ServerId, RoleId) values can produce the same token string.

---

### HIGH-12 — Oracle and MySQL Authentication Queries Are Empty
**File:** [GB5Framework/FrameworkDAL/Query/User/UserQB.cs:346-347](GB5Framework/FrameworkDAL/Query/User/UserQB.cs#L346-L347)

```csharp
public const string GET_USER_ORACLE = @";";
public const string GET_USER_MYSQL  = @";";
```

All Oracle and MySQL authentication query constants are empty semicolons or empty strings. Any organization running Oracle or MySQL databases would have completely broken authentication — every login attempt fails silently or throws a query execution error.

---

### HIGH-13 — All Oracle/MySQL Login Detail Queries Are Empty Strings
**File:** [GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs:177-184](GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs#L177-L184)

```csharp
public const string GET_USERLOGINID_ORACLE    = @"";
public const string GET_USERLOGINID_MYSQL     = @"";
public const string GET_USERLOGINCODE_ORACLE  = @"";
public const string GET_USERLOGINCODE_MYSQL   = @"";
public const string GET_ALL_PERIOD_ORACLE     = @"";
public const string GET_ALL_PERIOD_MYSQL      = @"";
```

Same issue as HIGH-12 — post-authentication detail retrieval is completely broken for Oracle and MySQL.

---

### HIGH-14 — `AllowAnonymous()` on User Login Detail Endpoint
**File:** [GB5Framework/FrameworkSL/Endpoints/UserLogin/GetUserLoginDetail.cs:21](GB5Framework/FrameworkSL/Endpoints/UserLogin/GetUserLoginDetail.cs#L21)

```csharp
public override void Configure()
{
    Get("/UserLoginDetail/GetUserLoginDetail");
    AllowAnonymous();   // ← no authentication required
}
```

This endpoint returns user profile information (OU name, logo, party branch, work period dates, store details) for any `UserId` passed in the `Login` header. Unauthenticated callers can enumerate user and organizational data.

---

### HIGH-15 — `ex.Message` Leaked in Error Response
**File:** [GB5Framework/FrameworkSL/Endpoints/UserLogin/GetUserLoginDetail.cs:38](GB5Framework/FrameworkSL/Endpoints/UserLogin/GetUserLoginDetail.cs#L38)

```csharp
return await GB5Shared.ResponseStandard.Response.CreateExceptionError<string>(
    ex, CacheKeyLevel.USER_LEVEL, LoginDTO, ex.Message, 500);
```

Exception messages leak internal implementation details (SQL error text, null reference paths, table names) to unauthenticated callers.

---

### HIGH-16 — PasswordPolicyQB Has No Tenant Filter
**File:** [GB5Framework/FrameworkDAL/Query/PasswordPolicy/PasswordPolicyQB.cs:41](GB5Framework/FrameworkDAL/Query/PasswordPolicy/PasswordPolicyQB.cs#L41)

```sql
WHERE PP.PASSWORDPOLICYID = @passwordpolicyid;
-- No CLIENTID / DATABASENAME filter
```

Any authenticated user can enumerate password policy settings (minimum length, complexity requirements, allowed wrong attempts) for any tenant by guessing policy IDs.

---

### HIGH-17 & HIGH-18 — `KeyCloakService` and `SSOService` Are MVC ControllerBase
**Files:**
- [GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs:27](GB5Framework/FrameworkSL/Controllers/KeyCloak/KeyCloakService.cs#L27)
- [GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs:15](GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs#L15)

```csharp
[ApiController]
[Route("Authorize")]
public class KeyCloakService : ControllerBase { }   // MVC — violates CLAUDE.md

[ApiController]
[Route("SSO/Authorize")]
public class SSOService : ControllerBase { }         // MVC — violates CLAUDE.md
```

CLAUDE.md explicitly prohibits MVC controllers — all HTTP endpoints must use FastEndpoints.

---

### HIGH-19 — In-Process `IMemoryCache` for OIDC Nonce — Breaks Multi-Instance
**File:** [GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs:21](GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs#L21)

```csharp
private readonly IMemoryCache _cache;
// ...
_cache.Set(NONCE_PREFIX + nonce, true, TimeSpan.FromMinutes(5));
```

`IMemoryCache` is in-process, per-instance. In a multi-replica Kubernetes deployment:
- The login request hits Instance A → nonce stored in Instance A's memory
- The Keycloak callback hits Instance B → nonce not found → login fails or nonce replay goes undetected

**Fix:** Use `IDistributedCache` (Redis-backed) for nonce storage, consistent with the existing Redis infrastructure.

---

### HIGH-20 — `PasswordEncryption.Encrypt()` Is Reversible — Passwords Must Be Hashed
**File:** [GB5Shared/EncryptionHelper/PasswordEncryption.cs](GB5Shared/EncryptionHelper/PasswordEncryption.cs)

```csharp
public static string Encrypt(string plaintext, string key)
{
    // AES-GCM encrypt — reversible with the key
    ...
    return Convert.ToBase64String(result);
}
```

Passwords must be stored as one-way hashes using a work-factor algorithm (`bcrypt`, `Argon2id`, or `PBKDF2-SHA256` with ≥100k iterations). Reversible encryption means anyone who obtains the encryption key (or the database backup) can recover every user's plaintext password.

---

### MEDIUM-21 — `DateTime.Now` Used Throughout Auth Flow
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs)

Occurrences at lines 139, 411, 478, 761, 810, 933, 1012, 1044, 1192, 1235, 1261.

```csharp
DateTime Validtodate = DateTime.Now.Add(Duration);
AuthenticationDTO.ServerDate = DateTime.Now;
AuthenticationDTO.LastLoginUsedTime = Base64Encode(DateTime.Now.ToString());
double OffSet = (DateTime.Now - DateTime.UtcNow).TotalMinutes;
```

All timestamps in the auth response should use `DateTime.UtcNow`. The `(DateTime.Now - DateTime.UtcNow).TotalMinutes` pattern for computing the UTC offset is fragile — it is server-local and will produce incorrect results on servers configured for non-UTC timezones or during DST transitions. The canonical fix is `TimeZoneInfo.Local.GetUtcOffset(DateTime.UtcNow).TotalMinutes`.

---

### MEDIUM-22 — Massive Code Duplication Across Three Login Methods
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs)

`GetLoginDetail` (~lines 859-1146), `GetLoginDetailByEMail` (~lines 712-857), and `GetLoginDetailViaKeycloak` (~lines 1148-1270) are 90% identical. Each contains the same ~120 lines of `AuthenticationDTO` field assignment. All share the same bugs (DateTime.Now, dead null-checks, etc.) tripling the maintenance surface.

**Fix:** Extract common population logic into a private `PopulateSessionFromUserDTO(AuthenticationDTO dto, UserDTO user, LoginDTO login)` method.

---

### MEDIUM-23 — 500+ Lines of Commented-Out Legacy Code
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs)

Approximately 400–500 lines of commented-out NHibernate ORM code, WCF `WebOperationContext`, event log SQL, and session code from the 2012–2021 era remain in the file. This includes commented-out raw SQL INSERT statements concatenating user data (potential injection if ever uncommented) and misleads security reviewers by obscuring what code actually runs vs what was old behavior.

---

### MEDIUM-24 — `AuthenticationDTO` Contains All Credential Fields
**File:** [GB5Shared/DTO/Framework/Authentication/AuthenticationDTO.cs](GB5Shared/DTO/Framework/Authentication/AuthenticationDTO.cs)

`AuthenticationDTO` contains 700+ lines with fields for `UserPassword` (via `UserDTO`), `MFAUserSecretKey`, `DeveloperSecretkey`, `EncryptedUserAuthPass`, and `DeveloperAccessKeyId` alongside user-facing display data. These credential fields are on the same DTO serialized and returned to the browser. Even if the code is "fixed" to not populate some of them, future maintainers may re-introduce the exposure.

**Fix:** Create a separate `SessionResponseDTO` containing only non-sensitive fields (UserId, RoleId, WorkOUId, formats, etc.) for the HTTP response.

---

### MEDIUM-25 — `throw error` Re-Throw Loses Stack Trace
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:1345-1348](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L1345-L1348)

```csharp
catch (Exception error)
{
    throw error;   // WRONG — loses original stack trace
}
```

Must be `throw;` (bare rethrow). This is in the `EncryptPassword` method called during the password verification path.

---

### MEDIUM-26 — Dead Null-Check After `GetUser` Which Already Throws on Null
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:131-136](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L131-L136)

```csharp
UserDTO LoginUserDTO = await _AuthenticationDAL.GetUser(UserCode, LoginDTO);
DigestHeader.AttachmentOption = LoginUserDTO.TempAttachmentOption;  // NRE if null
if (LoginUserDTO != null)  // ← always true (GetUser throws if null)
{
    await GetLoginDetail(UserCode, ConnectionName, DigestHeader, ServerConfigDTO);
}
```

`GetUser` throws `Exception("No user found with code: ...")` when the user is not found (DAL line 53). So `LoginUserDTO` is never null after line 131. The null-check provides false confidence to readers, and line 132 (`LoginUserDTO.TempAttachmentOption`) would NRE first anyway.

---

### MEDIUM-27 — No `CancellationToken` in Any Authentication BLL Method
**File:** [GB5Framework/FrameworkBLL/Authentication/IAuthenticationBLL.cs](GB5Framework/FrameworkBLL/Authentication/IAuthenticationBLL.cs)

```csharp
Task<AuthenticationDTO> AuthenticateUser(string ConnectionName, string UserCode, AuthenticationDTO DigestHeader);
Task<AuthenticationDTO> GetAuthorizeUser(string UserCode, string ConnectionName);
// ... none accept CancellationToken
```

Authentication is the hottest path in the system. Long-running auth queries cannot be cancelled when the client disconnects, wasting DB connections and threadpool threads during login storms.

---

### MEDIUM-28 — Dead If/Else: Both Branches Assign the Same Value
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:1019-1026](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L1019-L1026)

```csharp
if (AuthenticationDTO.UserCode == null)
    AuthenticationDTO.UserCode = UserCode;   // assigns UserCode
else
    AuthenticationDTO.UserCode = UserCode;   // also assigns UserCode (identical)
```

This appears in all three login methods. Both branches are identical — a copy-paste artifact.

---

### MEDIUM-29 — Repeated Duplicate Field Assignments
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:90-103](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L90-L103)

```csharp
DigestHeader.ServerId          = ServerConfigDTO.ServerId;       // line 91
DigestHeader.ServerConfigId    = ServerConfigDTO.ServerConfigId;
DigestHeader.ConnectionDatabaseName = ServerConfigDTO.DatabaseName; // line 93
DigestHeader.ServerId          = ServerConfigDTO.ServerId;       // line 94 — duplicate!
...
DigestHeader.ConnectionDatabaseName = ServerConfigDTO.DatabaseName; // line 98 — duplicate!
DigestHeader.ServerMachineName = ServerConfigDTO.ServerMachineName; // line 100
DigestHeader.ServerName        = ServerConfigDTO.ServerName;     // line 101
DigestHeader.ServerIP          = ServerConfigDTO.ServerIP;       // line 102
DigestHeader.ServerMachineName = ServerConfigDTO.ServerMachineName; // already set line 100!
```

Fields are assigned twice in sequence with no intervening mutation, adding noise and making code harder to diff.

---

### MEDIUM-30 — Auth URL with Nonce Logged at Info Level
**File:** [GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs:90](GB5Framework/FrameworkSL/Controllers/KeyCloak/SSOService.cs#L90)

```csharp
_logger.LogInformation("SSO Redirect → {url}", authUrl);
```

`authUrl` contains the `nonce` parameter. Nonces in OIDC are security assertions tied to the session. Logging them allows:
- Log-based nonce replay if the log sink is compromised
- Correlation of nonce values across log events to map user sessions

Log the redirect without the `nonce` value, or use Debug level with masking.

---

### LOW-31 — Unused `LoggingDTO` Instances
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:54](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L54)

`LoggingDTO LoggingDTO = new LoggingDTO();` is instantiated at the top of every login method and never called. All actual logging references are commented out (MEDIUM-23). Remove the declaration.

---

### LOW-32 — Unused `UserDTO UserDTO = new()` Declaration
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:52](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L52)

`UserDTO UserDTO = new();` is declared but immediately shadowed by `UserDTO LoginUserDTO = await _AuthenticationDAL.GetUser(...)`. Dead variable.

---

### LOW-33 — `new System.TimeSpan(0, 0, 0)` Should Be `TimeSpan.Zero`
**File:** [GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs:138](GB5Framework/FrameworkBLL/Authentication/AuthenticationBLL.cs#L138)

```csharp
System.TimeSpan Duration = new System.TimeSpan(0, 0, 0);  // unnecessary
DateTime Validtodate = DateTime.Now.Add(Duration);         // equivalent to DateTime.Now
```

This creates a zero-duration TimeSpan and adds it to `DateTime.Now` — producing exactly `DateTime.Now`. The entire `ValidToTime` calculation is a no-op.

---

### LOW-34 — `UserLoginQB` Is a `class`, Not a `static class`
**File:** [GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs:9](GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs#L9)

```csharp
public class UserLoginQB { ... }   // wrong — should be static class
```

All other QB files are `public static class`. `UserLoginQB` can be instantiated unnecessarily (no instance state).

---

### LOW-35 — `finally { json = null; }` No-Op Pattern
**File:** [GB5Framework/FrameworkDAL/CustomCode/PasswordPolicy/PasswordPolicyDAL.cs:35](GB5Framework/FrameworkDAL/CustomCode/PasswordPolicy/PasswordPolicyDAL.cs#L35)

```csharp
finally { json = null; }
```

Setting a local variable to `null` in `finally` does nothing — the variable goes out of scope at the method end. Systemic anti-pattern found throughout the DAL layer.

---

### LOW-36 — `GET_ALL_PERIOD_SQL` Has No Tenant Filter
**File:** [GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs:80-83](GB5Framework/FrameworkDAL/Query/UserLogin/UserLoginQB.cs#L80-L83)

```sql
SELECT FROMDATE AS PeriodFromDate
FROM MPERIOD
WHERE PERIODID <> -1
ORDER BY FROMDATE DESC;
-- No CLIENTID / DATABASENAME filter
```

Returns all financial/accounting periods for all tenants on the database server.

---

## OTP Stack Assessment — Points of Note

**Good practices found:**
- `GeneratorOTP.Generate()` uses `RandomNumberGenerator.GetInt32()` (CSPRNG) — correct
- `EmailOtpRedisService` hashes OTP with SHA-256 before Redis storage — good
- 5-minute TTL and `MaxAttempts = 5` guard implemented
- Successful OTP deletes the Redis key immediately — correct

**Remaining concerns:**
- SHA-256 for OTP hashing: since OTPs are 6-digit (10^6 space), SHA-256 is fast enough to be brute-forced. HMAC-SHA256 with a server secret would add resistance.
- Redis key format: `OTP:EMAIL{ConnectionName}:{email}` (note the missing `:` separator between "EMAIL" and `ConnectionName`) — could cause key collisions if `ConnectionName` starts with a digit and email starts with a digit in an edge case. Use `OTP:EMAIL:{ConnectionName}:{email}`.

---

## Priority Remediation Plan

### Tier 1 — Immediate (blocks production deployment)

| # | Action |
|---|--------|
| C-1+C-2 | Remove ALL hardcoded AES keys; load from HashiCorp Vault |
| C-3 | Remove `DangerousAcceptAnyServerCertificateValidator`; configure proper CA trust |
| C-4 | Remove `clientSecret` from query params; store in signed session cookie |
| C-5 | Restore password verification in `GetLoginDetailByEMail` |
| C-7 | Remove `MFAUserSecretKey`, `MFAQRCode` from all login response DTOs |
| C-8+C-9 | Add `AND CLIENTID = @clientId` to all SQL Server tenant-scoped queries |
| H-10+H-11 | Replace Base64 session token with HMAC-SHA256 signed token |

### Tier 2 — High Priority (security hardening)

| # | Action |
|---|--------|
| C-6 | Migrate from MD5 Digest Auth to SHA-256 Digest (RFC 7616) or JWT/OAuth |
| H-12+H-13 | Implement Oracle and MySQL auth queries |
| H-14+H-15 | Remove `AllowAnonymous()` from `GetUserLoginDetail`; remove `ex.Message` from errors |
| H-19 | Replace `IMemoryCache` with `IDistributedCache` for SSO nonces |
| H-20 | Replace `PasswordEncryption.Encrypt()` with `bcrypt`/`Argon2id` hashing |
| H-17+H-18 | Migrate `KeyCloakService` and `SSOService` to FastEndpoints |

### Tier 3 — Refactor

| # | Action |
|---|--------|
| M-21 | Replace all `DateTime.Now` with `DateTime.UtcNow` in auth methods |
| M-22 | Extract common `PopulateSessionFromUserDTO()` method; eliminate ~300 duplicate lines |
| M-23 | Delete all commented-out NHibernate/WCF code |
| M-24 | Create `SessionResponseDTO` — separate from `AuthenticationDTO` for HTTP responses |
| M-25 | Fix `throw error` → `throw;` in `EncryptPassword` |
| M-27 | Add `CancellationToken` to all `IAuthenticationBLL` method signatures |
| L-31–L-35 | Remove dead variables, fix static class, remove no-op finally blocks |

---

## Issue Totals
| Severity | Count |
|----------|-------|
| CRITICAL | 9 |
| HIGH | 11 |
| MEDIUM | 10 |
| LOW | 6 |
| **Total** | **36** |