Add SMTP authentication support to Forward dialog and Name property to rules#364
Open
Add SMTP authentication support to Forward dialog and Name property to rules#364
Conversation
…o rules The Forward dialog now supports SMTP authentication (username/password), port configuration, and SSL toggle. Users can also select from pre-configured forwarding rules via a dropdown, which auto-populates all fields. Rules now have a Name property for easy identification in both the rules list and the forward dialog's rule picker. Closes #363 https://claude.ai/code/session_01K1oUZzF99AeJhxX3ScrBcS
📝 WalkthroughWalkthroughAdds a Name property to the rule interface and base classes; extends the Forward UI and ViewModel to support selecting relay rules and SMTP auth (username, password, port, SSL), persists selected SMTP settings (except password), updates MainViewModel mapping, and shows rule names in the rules grid. Changes
Sequence DiagramsequenceDiagram
actor User
participant ForwardView as ForwardView (UI)
participant FVM as ForwardViewModel (VM)
participant RuleService as RuleService (Service)
participant Settings as Settings (Config)
participant MVM as MainViewModel (VM)
participant ForwardRule as ForwardRule (Domain)
User->>ForwardView: Open Forward Dialog
ForwardView->>FVM: Initialize & Load
FVM->>RuleService: LoadAvailableRules()
RuleService-->>FVM: RelayRule list
FVM->>Settings: Load saved SMTP settings
FVM-->>ForwardView: Display rules & SMTP fields
User->>ForwardView: Select Rule or Edit Fields
ForwardView->>FVM: SelectedRule changed
FVM->>FVM: Populate SMTP fields from selected rule
User->>ForwardView: Click Send
ForwardView->>FVM: Execute Send
FVM->>Settings: Persist SMTP settings (except password)
FVM->>MVM: Request forward with collected SMTP data
rect rgba(100,150,200,0.5)
MVM->>ForwardRule: Create & set SmtpServer, SmtpPort, SmtpUseSSL, SmtpUsername, SmtpPassword
end
ForwardRule-->>MVM: ForwardRule ready
MVM->>User: Email forwarded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
src/Papercut.Rules/Domain/Rules/PeriodicBackgroundRuleBase.cs (1)
34-51: Consider extracting common implementation to a shared base class.
PeriodicBackgroundRuleBaseandNewMessageRuleBasenow have nearly identical implementations ofName,IsEnabled,Type,Description, andOnPropertyChanged. A shared abstractRuleBasecould reduce duplication.This is a larger refactor and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Papercut.Rules/Domain/Rules/PeriodicBackgroundRuleBase.cs` around lines 34 - 51, PeriodicBackgroundRuleBase and NewMessageRuleBase duplicate property implementations (Name, IsEnabled, Type, Description) and OnPropertyChanged; extract these into a new abstract base class (e.g., RuleBase) that declares the common properties and implements backing fields, getters/setters and OnPropertyChanged, then have PeriodicBackgroundRuleBase and NewMessageRuleBase inherit from RuleBase and remove their duplicated code; ensure RuleBase exposes protected members or virtuals if either derived class needs to customize behavior and update constructors/signatures to call base as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Papercut.UI/Properties/Settings.Designer.cs`:
- Around line 110-120: The ForwardSmtpPassword property currently persists
plaintext credentials via the Settings designer; update persistence to
encrypt/decrypt the value when saving/loading by switching storage to a secure
API (e.g., use ProtectedData.Protect/Unprotect or Windows Credential Manager)
instead of storing directly in this property. Modify the Settings accessor logic
for ForwardSmtpPassword (or the Settings class load/save hooks) so that getters
return the decrypted secret and setters encrypt before calling
this["ForwardSmtpPassword"], and ensure decryption is applied when reading the
underlying stored value; alternatively implement a separate secure store
(Credential Manager) and change ForwardSmtpPassword to retrieve/store from that
store. Ensure any migration handles existing plaintext values
(decrypt-pass-through or re-encrypt) and add a short code comment documenting
the chosen secure storage approach.
In `@src/Papercut.UI/ViewModels/ForwardViewModel.cs`:
- Around line 208-211: The code is persisting SMTP secrets via
Settings.Default.ForwardSmtpPassword; remove reads/writes of
Settings.Default.ForwardSmtpPassword in ForwardViewModel (the places assigning
this.Password from Settings.Default and any code writing it back around the same
symbol, also at the similar assignments at lines 253-256) and instead keep
Password as a session-only in-memory property or replace those accesses with
calls to your OS-protected secret store API (store only a reference/token in
Settings if needed). Update the constructor/property setters in ForwardViewModel
so Username/Port/UseSsl can still come from Settings.Default but Password is
never saved to Settings.Default; if a protected store isn’t wired yet, leave
Password transient and ensure any Save/Apply methods do not persist it to
Settings.Default.ForwardSmtpPassword.
- Around line 139-157: The Port property is user-editable but not validated, so
invalid values (0, negative, >65535) can be persisted; update
ForwardViewModel.Send() to validate Port is within 1..65535 before
saving/closing, and if invalid prevent the save/close (e.g., return early) and
surface a validation/error message to the user (use the existing
notification/validation mechanism in ForwardViewModel), referencing the Port
property and the Send() method so the invalid transport settings never get
persisted.
- Around line 176-199: LoadAvailableRules currently adds all RelayRule instances
to AvailableRules while PopulateFromRule only sets From/To for ForwardRule,
causing non-forward rules to leave stale addresses; update LoadAvailableRules to
only enumerate and add rules.OfType<ForwardRule>() (or otherwise filter to
ForwardRule) so the picker only shows rules that can populate the full forward
form, and ensure PopulateFromRule still handles non-ForwardRule cases (clearing
From/To) if you prefer the alternative approach.
In `@src/Papercut.UI/ViewModels/MainViewModel.cs`:
- Around line 699-705: The assignment to SmtpServer uses
forwardViewModel.Server.Trim() which can throw a NullReferenceException if
Server is null; change the assignment in the MainViewModel where the new SMTP
settings are created (the block that sets ToEmail, SmtpServer, SmtpPort,
SmtpUseSSL, SmtpUsername, SmtpPassword) to guard against null—e.g., use a
null-coalescing or null-conditional pattern so SmtpServer is set from
(forwardViewModel.Server?.Trim() ?? string.Empty) or similar to ensure a
non-null string is assigned.
---
Nitpick comments:
In `@src/Papercut.Rules/Domain/Rules/PeriodicBackgroundRuleBase.cs`:
- Around line 34-51: PeriodicBackgroundRuleBase and NewMessageRuleBase duplicate
property implementations (Name, IsEnabled, Type, Description) and
OnPropertyChanged; extract these into a new abstract base class (e.g., RuleBase)
that declares the common properties and implements backing fields,
getters/setters and OnPropertyChanged, then have PeriodicBackgroundRuleBase and
NewMessageRuleBase inherit from RuleBase and remove their duplicated code;
ensure RuleBase exposes protected members or virtuals if either derived class
needs to customize behavior and update constructors/signatures to call base as
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75b03ad4-6a02-40f8-95b5-57d4fef1ca96
📒 Files selected for processing (9)
src/Papercut.Core/Domain/Rules/IRule.cssrc/Papercut.Rules/Domain/Rules/NewMessageRuleBase.cssrc/Papercut.Rules/Domain/Rules/PeriodicBackgroundRuleBase.cssrc/Papercut.UI/Properties/Settings.Designer.cssrc/Papercut.UI/Properties/Settings.settingssrc/Papercut.UI/ViewModels/ForwardViewModel.cssrc/Papercut.UI/ViewModels/MainViewModel.cssrc/Papercut.UI/Views/ForwardView.xamlsrc/Papercut.UI/Views/RulesConfigurationView.xaml
Comment on lines
+110
to
+120
| [global::System.Configuration.UserScopedSettingAttribute()] | ||
| [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] | ||
| [global::System.Configuration.DefaultSettingValueAttribute("")] | ||
| public string ForwardSmtpPassword { | ||
| get { | ||
| return ((string)(this["ForwardSmtpPassword"])); | ||
| } | ||
| set { | ||
| this["ForwardSmtpPassword"] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any existing credential protection pattern in the codebase
rg -n "ProtectedData|Credential|Encrypt|Decrypt" --type=cs -C2Repository: ChangemakerStudios/Papercut-SMTP
Length of output: 58
Security concern: SMTP password stored in plaintext.
ForwardSmtpPassword is persisted as plaintext in the user's configuration file (user.config). While Papercut is a development/testing SMTP tool, storing credentials unencrypted poses a risk if the machine is compromised or shared with other users.
Consider using Windows DPAPI (ProtectedData.Protect/Unprotect) or the Windows Credential Manager to securely store the password. If plaintext storage is intentional for this tool's use case, document this security trade-off explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Papercut.UI/Properties/Settings.Designer.cs` around lines 110 - 120, The
ForwardSmtpPassword property currently persists plaintext credentials via the
Settings designer; update persistence to encrypt/decrypt the value when
saving/loading by switching storage to a secure API (e.g., use
ProtectedData.Protect/Unprotect or Windows Credential Manager) instead of
storing directly in this property. Modify the Settings accessor logic for
ForwardSmtpPassword (or the Settings class load/save hooks) so that getters
return the decrypted secret and setters encrypt before calling
this["ForwardSmtpPassword"], and ensure decryption is applied when reading the
underlying stored value; alternatively implement a separate secure store
(Credential Manager) and change ForwardSmtpPassword to retrieve/store from that
store. Ensure any migration handles existing plaintext values
(decrypt-pass-through or re-encrypt) and add a short code comment documenting
the chosen secure storage approach.
- Don't persist SMTP password to settings (session-only for security) - Add port range validation (1-65535) in Send() - Filter rule picker to ForwardRule only (not all RelayRules) - Add null-safe operators for Server/Username/Password in MainViewModel https://claude.ai/code/session_01K1oUZzF99AeJhxX3ScrBcS
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Papercut.Rules/App/RulesRunner.cs`:
- Around line 130-133: The current catch block in RulesRunner (in class
RulesRunner) unconditionally rethrows OperationCanceledException; change it to
catch (OperationCanceledException ex) and only rethrow when ex.CancellationToken
equals this runner's cancellation token (the field used by the runner, e.g.
_cancellationToken or runnerToken); if the exception was canceled by a different
token, treat it as handled (do not rethrow) so dispatcher/internal timeouts
don't abort this run. Ensure you reference the runner's cancellation token field
when comparing before rethrowing.
In `@src/Papercut.UI/Views/ForwardView.xaml`:
- Line 16: The window in ForwardView.xaml is hard-coded with
ResizeMode="NoResize" and Width="340" Height="360", which causes overflow when
auth rows conditionally appear; change the Window properties to allow vertical
growth (e.g., remove NoResize or set ResizeMode="CanResize" and set
SizeToContent="Height" or remove fixed Height) or wrap the form Grid that
contains the conditional auth rows/buttons in a ScrollViewer so added rows won’t
be clipped; look for the Window tag with ResizeMode/Width/Height and the main
form Grid around the conditional auth rows to apply one of these fixes.
- Around line 55-60: The form is split between bound controls (AvailableRules,
SelectedRule, HasAvailableRules) and unbound TextBoxes (Server, Username, From,
To), causing state divergence; fix by binding each TextBox (currently unbound
Server, Username, From, To) to the corresponding ViewModel properties (e.g.,
Server, Username, FromAddress, ToAddress or whatever properties exist) with
Mode=TwoWay and UpdateSourceTrigger=PropertyChanged so the rule picker can
populate and the submit logic reads the same source of truth, and ensure the
ViewModel updates those properties when SelectedRule changes (or expose a
method/property on the rule DTO to map into the ViewModel) so all fields share
one canonical state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db18b6a0-1698-4c87-aea3-01ccbecf728f
📒 Files selected for processing (4)
src/Papercut.Rules/App/RulesRunner.cssrc/Papercut.UI/ViewModels/ForwardViewModel.cssrc/Papercut.UI/Views/ForwardView.xamltest/Papercut.Rules.Tests/RulesRunnerTests.cs
✅ Files skipped from review due to trivial changes (1)
- test/Papercut.Rules.Tests/RulesRunnerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Papercut.UI/ViewModels/ForwardViewModel.cs
Comment on lines
+130
to
+133
| catch (OperationCanceledException) | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Rethrow cancellation only when this runner token is canceled.
At Lines 130-133, rethrowing every OperationCanceledException can propagate unrelated dispatcher-internal cancellations (e.g., timeout-driven TaskCanceledException) and abort the whole run unexpectedly.
Proposed fix
- catch (OperationCanceledException)
+ catch (OperationCanceledException) when (token.IsCancellationRequested)
{
throw;
}
+ catch (OperationCanceledException ex)
+ {
+ _logger.Warning(
+ ex,
+ "Rule dispatch canceled without runner cancellation for Rule {Rule} on Message {`@MessageEntry`}",
+ rule,
+ messageEntry);
+ }
catch (Exception ex)
{
_logger.Warning(
ex,
"Failure Dispatching Rule {Rule} on Message {`@MessageEntry`}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (OperationCanceledException) | |
| { | |
| throw; | |
| } | |
| catch (OperationCanceledException) when (token.IsCancellationRequested) | |
| { | |
| throw; | |
| } | |
| catch (OperationCanceledException ex) | |
| { | |
| _logger.Warning( | |
| ex, | |
| "Rule dispatch canceled without runner cancellation for Rule {Rule} on Message {`@MessageEntry`}", | |
| rule, | |
| messageEntry); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Papercut.Rules/App/RulesRunner.cs` around lines 130 - 133, The current
catch block in RulesRunner (in class RulesRunner) unconditionally rethrows
OperationCanceledException; change it to catch (OperationCanceledException ex)
and only rethrow when ex.CancellationToken equals this runner's cancellation
token (the field used by the runner, e.g. _cancellationToken or runnerToken); if
the exception was canceled by a different token, treat it as handled (do not
rethrow) so dispatcher/internal timeouts don't abort this run. Ensure you
reference the runner's cancellation token field when comparing before
rethrowing.
| d:DataContext="{x:Type ViewModels:ForwardViewModel}" | ||
| Title="{Binding WindowTitle}" | ||
| ResizeMode="NoResize" Width="243" Height="162.333" | ||
| ResizeMode="NoResize" Width="340" Height="360" |
There was a problem hiding this comment.
Allow the dialog to expand when the auth rows appear.
Line 16 still hard-codes a non-resizable 340x360 window, but Lines 44-50 and 86-96 add enough conditional content that the bottom fields/buttons can get clipped under DPI scaling or when authentication is enabled.
↕️ Suggested layout fix
- ResizeMode="NoResize" Width="340" Height="360"
+ ResizeMode="CanResize" Width="340" MinWidth="340" SizeToContent="Height"If you need to keep a fixed outer size, wrapping the form grid in a ScrollViewer would avoid the overflow too.
Also applies to: 44-50, 86-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Papercut.UI/Views/ForwardView.xaml` at line 16, The window in
ForwardView.xaml is hard-coded with ResizeMode="NoResize" and Width="340"
Height="360", which causes overflow when auth rows conditionally appear; change
the Window properties to allow vertical growth (e.g., remove NoResize or set
ResizeMode="CanResize" and set SizeToContent="Height" or remove fixed Height) or
wrap the form Grid that contains the conditional auth rows/buttons in a
ScrollViewer so added rows won’t be clipped; look for the Window tag with
ResizeMode/Width/Height and the main form Grid around the conditional auth rows
to apply one of these fixes.
Comment on lines
+55
to
+60
| <ComboBox Grid.Column="1" Grid.Row="0" Margin="2" Height="26" | ||
| ItemsSource="{Binding AvailableRules}" | ||
| SelectedItem="{Binding SelectedRule}" | ||
| DisplayMemberPath="DisplayName" | ||
| IsEnabled="{Binding HasAvailableRules}" | ||
| controls:TextBoxHelper.Watermark="(manual entry)" /> |
There was a problem hiding this comment.
Keep the whole form on the same source of truth.
Line 55 routes rule selection through SelectedRule, but Lines 67, 88, 100, and 104 leave Server, Username, From, and To as unbound TextBoxes. That split means the rule picker cannot fully populate the dialog, and submit logic can easily read stale values from the wrong side.
🔗 Suggested binding fix
<TextBox Grid.Column="1" Grid.Row="2" Margin="2" TabIndex="0"
- x:Name="Server" />
+ x:Name="Server"
+ Text="{Binding Server, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />
<TextBox Grid.Column="1" Grid.Row="6" Margin="2" TabIndex="4"
x:Name="Username"
+ Text="{Binding Username, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"
Visibility="{Binding UseAuthentication, Converter={StaticResource BooleanToVisibilityConverter}}" />
<TextBox Grid.Column="1" Grid.Row="8" Margin="2" TabIndex="6"
- x:Name="From" />
+ x:Name="From"
+ Text="{Binding From, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />
<TextBox Grid.Column="1" Grid.Row="9" Margin="2" TabIndex="7"
- x:Name="To" />
+ x:Name="To"
+ Text="{Binding To, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />Also applies to: 67-68, 88-90, 100-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Papercut.UI/Views/ForwardView.xaml` around lines 55 - 60, The form is
split between bound controls (AvailableRules, SelectedRule, HasAvailableRules)
and unbound TextBoxes (Server, Username, From, To), causing state divergence;
fix by binding each TextBox (currently unbound Server, Username, From, To) to
the corresponding ViewModel properties (e.g., Server, Username, FromAddress,
ToAddress or whatever properties exist) with Mode=TwoWay and
UpdateSourceTrigger=PropertyChanged so the rule picker can populate and the
submit logic reads the same source of truth, and ensure the ViewModel updates
those properties when SelectedRule changes (or expose a method/property on the
rule DTO to map into the ViewModel) so all fields share one canonical state.
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
2 participants
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.
The Forward dialog now supports SMTP authentication (username/password),
port configuration, and SSL toggle. Users can also select from pre-configured
forwarding rules via a dropdown, which auto-populates all fields. Rules now
have a Name property for easy identification in both the rules list and the
forward dialog's rule picker.
Closes #363
https://claude.ai/code/session_01K1oUZzF99AeJhxX3ScrBcS
Summary by CodeRabbit
New Features
UI Updates