Critical fixes modernization - #24
Open
MarijaGojkov wants to merge 12 commits into
Open
Conversation
Centralizes the connection string behind DatabaseAccess.ConnectionString, which reads the EBankingDb entry from App.config via ConfigurationManager. Replaces five hardcoded copies (four repositories + the unused, mismatched default in DatabaseAccess.cs that referenced a different database name). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Strips the test email/password that the login screen shipped with by default. Reviewers cloning the repo no longer see prefilled credentials. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds BCrypt.Net-Next to the data layer and a PasswordHasher helper (work-factor 11). UserRepository now hashes on AddUser and UpdateUserPassword, and IsValidUser pulls the stored hash and verifies against it instead of comparing plaintext. Also fixes a latent bug in UpdateUserPassword that called ExecuteScalar for a non-query statement. EBankingSystem_Seed.sql now ships pre-hashed passwords; the plaintext demo credentials are kept readable in a comment so a reviewer can still log in. EBanking.TestConsole — previously an empty project — is now a one-shot utility: 'hash <pwd>' prints a hash (used to prepare the seed), no-arg mode re-hashes any plaintext rows left over in an existing dev DB (detected via NOT LIKE '\$2%'). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Balance, Amount, BalanceAfterTransaction, and exchange-rate Value are now decimal end to end (DataAccess models, Services models, UI models, the IAccountRepository / IAccountService.UpdateBalance signatures, and the service implementation). Repository readers drop the decimal.ToDouble round-trip and read DECIMAL(18,2) directly. Writes for balance and transaction amounts now use explicit SqlParameter with Precision=18, Scale=2 instead of AddWithValue, which silently negotiates the wrong precision for decimal values. Side fix: two latent ExecuteScalar calls on UPDATE/INSERT statements (CreateAccount, UpdateBalance) are now ExecuteNonQuery. The DB schema is unchanged; this commit is purely about not laundering DECIMAL through binary floating-point in the application tier. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The old PaymentViewModel.Pay() inserted the payer's transaction row,
updated the payer's balance, checked the recipient, and then fired an
async-void recipient-credit before calling Close(). Any failure between
the four steps left money debited but uncredited, or an orphan
transaction row; in practice the window often closed before the
fire-and-forget credit completed.
Replaces the whole flow with ITransactionService.TransferFunds(request),
which:
- validates Amount > 0 and payer != recipient before touching SQL,
- opens one SqlConnection + SqlTransaction (ReadCommitted),
- reads the payer balance with (UPDLOCK, ROWLOCK) so a concurrent
transfer can't race the balance check,
- throws InvalidOperationException("Insufficient funds.") inside the
transaction — the real defense, since UI validation can be bypassed,
- debits the payer, and if the recipient is internal credits them and
writes their transaction row,
- writes the payer's transaction row,
- commits, or rolls back and rethrows on any failure.
PaymentViewModel is now a thin caller: validate, TransferFunds, Close —
or MessageBox the exception. The async-void AddTransactionToRecipient
helper is gone, and PaymentViewModel no longer needs IAccountService.
PaymentViewValidator now also blocks payer == recipient and Amount >
CurrentBalance, and drops two dead `== null` checks on non-nullable
fields.
Side fixes (same change set):
- AccountRepository.IsValidAccount switched from SELECT * + a fragile
`as string is not null` cast on the first column to SELECT 1 +
`ExecuteScalar() is not null`.
- GetAccountByAccountNumber loses its async marker; the method was
declared async Task<Account> but did no awaiting. Propagated the
sync signature through IAccountRepository, IAccountService, and
AccountService.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
AccountViewModel.Payment() was declared async void but did no awaiting (left over from when PaymentViewModel.AddTransactionToRecipient was called from here as an awaited helper). Drop the async marker; the method is plain void now. After this commit a solution-wide search for 'async void' in EBanking.UI/ returns no hits. The three view.Closing subscriptions — accountView.Closing in LoginViewModel.Login(), and view.Closing for both CurrencyExchangeView and PaymentView in AccountViewModel — were attached as anonymous lambdas that captured 'this' and were never removed. Each child-window open added a new subscription with no detach, pinning every prior ViewModel and its services for the lifetime of the app. Replaced with named local-function handlers that '-=' themselves on the first invocation, so the closure (and the captured ViewModel chain) is collectible after the child window closes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a README covering tech stack, the four projects, local setup, seed credentials (now BCrypt-hashed), and a per-bullet summary of the modernization pass that matches the six commits on this branch. Also documents the EBanking.TestConsole utility (hash generator + idempotent rehash for legacy dev DBs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ExecuteTransfer previously preserved the original PaymentViewModel.Pay
behavior of debiting the payer even when the recipient account number
didn't exist in [Account] (modeling a send to another bank). For a
closed-system demo this is confusing — the UI shows the payer
balance dropping with no indication that the money went anywhere.
Move the recipient lookup before the payer debit and throw
InvalidOperationException("Recipient account not found.") when it
returns null. The catch in ExecuteTransfer rolls back, so on this path
no rows are written at all. Payer-account-missing already had the
symmetric throw. PaymentViewModel.Pay catches and surfaces the message
via MessageBox, so the user gets clear feedback instead of a silent
debit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Amount and Converted-value fields gave no indication of which
currency they were in, so a user clicking exchange from an RSD account
to an EUR account would see '1000 -> 8.5' and assume the math was
broken (it wasn't: 1000 RSD * 0.0085 = 8.50 EUR).
Adds three small currency badges:
- next to the Amount textbox: source currency (Model.Account.Currency)
- next to the dropdown: target currency (was already there but had
no Foreground set, so it rendered invisible on the dark gradient)
- next to the Converted-value textbox: target currency
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The screen never created a user — it took a bank-issued email + PIN and
let the holder set their first password (UpdateUserPassword). The
wording all said 'Registration' / 'REGISTER', which led to thinking new
users were being created.
Renames the user-facing text (class names left alone to avoid churn in
the locator and navigation):
- Title bar 'REGISTRATION' -> 'ACCOUNT ACTIVATION'
- Model.Title 'Registration' -> 'Account Activation'
- Submit button 'REGISTER' -> 'ACTIVATE'
- Field labels: 'E-mail' -> 'Bank e-mail', 'User PIN' -> 'Activation
PIN', 'Password' -> 'New password', 'Confirm password' -> 'Confirm
new password' — the bank-issued vs user-chosen split is now obvious.
Drops the unused RegistrationModel.Label field. The 'Passwords do not
match' error was being assigned to Label, but no XAML binding existed,
so the message was silently swallowed. Surfaced inline on
ConfirmPasswordError (already shown in red below the field).
Replaces the wrong-PIN MessageBox with a form-level FormError field
bound to a new red label between the last field and the submit button.
Brings PIN-mismatch in line with the other inline errors.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The login screen prompted 'Don't have an account? Activate it!', which
promised self-service signup. The button actually opened a flow that
requires bank-issued credentials (email + PIN) and only sets the
online-banking password — there is no AddUser path in the UI today.
Rewords the entry point so the prompt matches what the flow actually
does. This is intentional: a real bank pre-creates users and customers
activate online access; self-service registration is out of scope for
this modernization pass.
LoginView prompt: 'Don't have an account?' ->
'First time logging in or forgot your password?'
LoginView button: 'Activate it!' -> 'Activate access'
Activation screen titlebar: 'ACCOUNT ACTIVATION' -> 'ACTIVATE ACCESS'
Activation screen subtitle: generic 'Welcome' blurb ->
'Use the e-mail and PIN your bank issued
to set or reset your online banking
password.'
Activation screen button: 'ACTIVATE' -> 'ACTIVATE ACCESS'
Activation window title: 'Account Activation' -> 'Activate access'
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds ITransactionRepository.ExecuteExchange (UPDLOCK both rows, re-check funds, debit + credit + two transaction rows, commit or rollback) and ICurrencyExchangeService.Exchange (validate, look up rate, round to 2dp, call ExecuteExchange). CurrencyExchangeViewModel no longer open-codes the two-leg flow; AccountViewModel updated to the new VM signature. README updated.
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
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.
Each change is its own commit so the PR can be read top-to-bottom.
Security
string-equality comparison are gone.
UserRepository.IsValidUserpulls the stored hash and calls
BCrypt.Verify. Seed data shipspre-hashed;
EBanking.TestConsoleis now a one-shot utility thatrehashes any plaintext rows in a stale dev DB.
App.config. Removed fivehardcoded copies, including a dead
DatabaseAccess.csconstant thatpointed at a different database name.
LoginModel.Correctness
SqlConnectionSqlTransaction. The payer row is locked withWITH (UPDLOCK, ROWLOCK); balance and insufficient-funds checks happen inside thetransaction so UI validation isn't load-bearing. Either both balance
changes and both transaction rows land, or the transaction rolls back
and nothing does. Previously the flow was four sequential calls with
an
async voidfire-and-forget for the recipient credit.rollback (was silently debiting the payer with no destination).
decimalfor all money values (Balance, Amount,BalanceAfterTransaction, exchange-rate Value), end-to-end across
DataAccess / Services / UI. Repository readers stopped round-tripping
through
decimal.ToDouble; decimal parameters are now bound viaexplicit
SqlParameterwithPrecision = 18, Scale = 2.WPF lifecycle
async voidhandlers in the UI project.view.Closinglambda subscriptions that pinned ViewModels acrosschild-window opens are now named handlers that detach themselves on
first invocation.
UX polish
it!" prompt promised self-service signup but the flow only set/reset
the online-banking password using bank-issued credentials. Reframed
end-to-end as "First time logging in or forgot your password? Activate
access."
to the Amount and Converted-value fields.
Test plan
ana.petrovic@email.com/password123transaction rows present in
[Transaction]labels and accurate decimal math
9012) → log in with the newpassword