Skip to content

fix: require 802.1x username for PEAP-MSCHAPv2 in wireless profile API - #1319

Merged
choonkeat1986 merged 2 commits into
mainfrom
jira1099
Oct 26, 2026
Merged

choonkeat1986 merged 2 commits into
mainfrom
jira1099

Conversation

@choonkeat1986

@choonkeat1986 choonkeat1986 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Copilot AI left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The tightened API contract is not reflected in OpenAPI or Postman artifacts.

1 open finding
What changed in this PR

Adds PEAP-MSCHAPv2 username validation to the wireless profile API.

Changes:

  • Rejects PEAP profiles without usernames.
  • Adds DTO and HTTP-route validation tests.
File Description
internal/​entity/​dto/​v1/​wifiprofile.go Enforces PEAP usernames.
internal/​entity/​dto/​v1/​wifiprofile_test.go Tests validation behavior.
internal/​controller/​httpapi/​v1/​wifiprofile_test.go Tests API rejection and acceptance.

🧠 Review effort: Balanced

JIRA:
#1099

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/entity/dto/v1/wifiprofile.go Outdated
@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.28%. Comparing base (ff1fb60) to head (de2c197).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
+ Coverage   62.24%   62.28%   +0.03%     
==========================================
  Files         159      159              
  Lines       13417    13420       +3     
==========================================
+ Hits         8351     8358       +7     
+ Misses       5066     5062       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The validation change is consistent across implementation, documentation, and comprehensive tests.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@rsdmike rsdmike left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @choonkeat1986! Thanks for adding the username check. does PEAP also need a password?, if so please require that too, and confirm that PEAP requests without a username never worked anyway, since those requests now get a 400 error.

@choonkeat1986

Copy link
Copy Markdown
Contributor Author

Thanks @choonkeat1986! Thanks for adding the username check. does PEAP also need a password?, if so please require that too, and confirm that PEAP requests without a username never worked anyway, since those requests now get a 400 error.

Yes. PEAPv0/EAP-MSCHAPv2 logs in with a username and a password, so the validator now requires both, and a request that is missing either gets 400.

Yes, requests without a username: console sent them to AMT with no Username element (go-wsman-messages uses omitempty), so the profile had no identity and MSCHAPv2 could not authenticate.

@choonkeat1986
choonkeat1986 merged commit 6dfb9aa into main Oct 26, 2026
22 checks passed
@choonkeat1986
choonkeat1986 deleted the jira1099 branch October 26, 2026 02:18
@RosieAMT

Copy link
Copy Markdown

🎉 This PR is included in version 1.46.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants