Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(SPV-1248) add domain check for paymail entry #828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cain4chain
Copy link

@cain4chain cain4chain commented Dec 20, 2024

  • feat(paymail) add checkPaymailDomain function for domain validation
  • feat(error) add error definition to engine code for invalid domain
  • chore(paymailCreateAddress) add logic for domain validation check

Pull Request Checklist

  • 📖 I created my PR using provided : CODE_STANDARDS
  • 📖 I have read the short Code of Conduct: CODE_OF_CONDUCT
  • 🏠 I tested my changes locally.
  • ✅ I have provided tests for my changes.
  • 📝 I have used conventional commits.
  • 📗 I have updated any related documentation.
  • 💾 PR was issued based on the Github or Jira issue.

feat(paymail) add checkPaymailDomain function for domain validation
feat(error) add error definition to engine code for invalid domain
chore(paymailCreateAddress) add logic for domain validation check
@cain4chain cain4chain added the chore Simple updates or version bumps label Dec 20, 2024
@cain4chain cain4chain self-assigned this Dec 20, 2024
@cain4chain cain4chain requested a review from a team as a code owner December 20, 2024 11:09
Copy link

Manual Tests

ℹ️ Remember to ask team members to perform manual tests and to assign tested label after testing.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 45.91%. Comparing base (de2a75c) to head (3302daa).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
actions/admin/paymail_addresses.go 0.00% 13 Missing ⚠️
actions/admin/paymail_addresses_old.go 0.00% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
- Coverage   45.97%   45.91%   -0.07%     
==========================================
  Files         369      369              
  Lines       17820    17840      +20     
==========================================
- Hits         8193     8191       -2     
- Misses       9020     9042      +22     
  Partials      607      607              
Flag Coverage Δ
unittests 45.91% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
actions/admin/paymail_addresses_old.go 0.00% <0.00%> (ø)
actions/admin/paymail_addresses.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de2a75c...3302daa. Read the comment docs.

Comment on lines +167 to +174
config := reqctx.AppConfig(c)
if config.Paymail.DomainValidationEnabled {
_, actualDomain, _ := paymail.SanitizePaymail(requestBody.Address)
if !checkPaymailDomain(actualDomain, config.Paymail.Domains) {
spverrors.ErrorResponse(c, spverrors.ErrInvalidDomain, logger)
return
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated code.
But since we plan to remove the "old" handlers soon (I believe 🙈), it is totally fine to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Simple updates or version bumps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants