-
Notifications
You must be signed in to change notification settings - Fork 4
RDKEMW-12168 Port the getAccountID Script functionality to RFC Module #148
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds account ID validation functionality to the RFC module by porting the getAccountID script logic. The implementation validates that account IDs are exactly 32 characters long and contain only alphanumeric characters, hyphens, and underscores before accepting them.
Changes:
- Added
validate_account_id()function to validate account ID format - Added validation check in
GetAccountID()to reject invalid account IDs - Defined
KEY_LENconstant (32) for account ID length validation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| rfcMgr/rfc_common.cpp | Implements the validate_account_id function with regex-based validation for account ID format |
| rfcMgr/rfc_common.h | Declares the validate_account_id function and defines KEY_LEN constant |
| rfcMgr/rfc_xconf_handler.cpp | Integrates validation into GetAccountID() to reject invalid account IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rfcMgr/rfc_xconf_handler.cpp
Outdated
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); | ||
| _accountId="Unknown"; |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation. The code inside this else-if block uses extra indentation (12 spaces or 3 tabs) compared to the surrounding if-else blocks which use standard indentation (8 spaces or 2 tabs). This should match the indentation of lines 821-822 and 831-833.
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); | |
| _accountId="Unknown"; | |
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); | |
| _accountId="Unknown"; |
rfcMgr/rfc_xconf_handler.cpp
Outdated
| else if(!validate_account_id(tempbuf)) | ||
| { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); | ||
| _accountId="Unknown"; |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after assignment operator. Should be "_accountId = "Unknown";" instead of "_accountId="Unknown";" for consistency with the coding style used elsewhere in the codebase (see line 833).
rfcMgr/rfc_xconf_handler.cpp
Outdated
| } | ||
| else if(!validate_account_id(tempbuf)) | ||
| { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message states "Invalid characters" but this validation also fails if the length is incorrect (not 32 characters). The error message should be more accurate, such as "Invalid accountId format (expected 32 alphanumeric characters, hyphens, or underscores)" to reflect both validation criteria.
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid characters in newly received accountId: %s\n", __FUNCTION__, __LINE__,tempbuf); | |
| RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "[%s][%d] Invalid accountId format (expected 32 alphanumeric characters, hyphens, or underscores): %s\n", __FUNCTION__, __LINE__, tempbuf); |
rfcMgr/rfc_common.cpp
Outdated
| bool validate_account_id(const std::string& accountId) | ||
| { | ||
| static const std::regex has_valid("[a-zA-Z0-9_-]"); | ||
| static const std::regex has_invalid("[!@#$%^&*()_+]"); | ||
|
|
||
| // Must be exactly KEY_LEN | ||
| if (accountId.length() != KEY_LEN) | ||
| return false; | ||
|
|
||
| return std::regex_search(accountId, has_valid) && | ||
| !std::regex_search(accountId, has_invalid); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new validation function validate_account_id lacks test coverage. Given that this function performs security-critical validation of account IDs and the repository has a gtest infrastructure in place (test/rfcTest.cpp), tests should be added to verify the function correctly validates valid account IDs and rejects invalid ones (wrong length, special characters, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add L1
rfcMgr/rfc_common.cpp
Outdated
| bool validate_account_id(const std::string& accountId) | ||
| { | ||
| static const std::regex has_valid("[a-zA-Z0-9_-]"); | ||
| static const std::regex has_invalid("[!@#$%^&*()_+]"); | ||
|
|
||
| // Must be exactly KEY_LEN | ||
| if (accountId.length() != KEY_LEN) | ||
| return false; | ||
|
|
||
| return std::regex_search(accountId, has_valid) && | ||
| !std::regex_search(accountId, has_invalid); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing include directive for regex. The validate_account_id function uses std::regex and std::regex_search but the required header is not included in this file. This will cause compilation errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed to ensure build failure
f9c53e8 to
55d2635
Compare
|
@madhubabutt - please address commentds |
d2c3116 to
8d16195
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
058913a to
dd9417c
Compare
d66c9c7 to
dad264c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (char c : str) | ||
| { | ||
| if(!std::isalnum(c)) | ||
| if(!std::isalnum(c) && c != '_' && c != '-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need length check and special char check also
similar to https://github.com/rdkcentral/sysint/blob/develop/lib/rdk/getAccountId.sh
d69359c to
95380b8
Compare
95380b8 to
94641a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
94641a3 to
6bd602a
Compare
6bd602a to
f44b897
Compare
f44b897 to
6aef810
Compare
6aef810 to
10ff6f3
Compare
10ff6f3 to
fe6e424
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe6e424 to
3988e3b
Compare
3988e3b to
a6cdf31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a6cdf31 to
1a39087
Compare
1a39087 to
0cc8b3a
Compare
0cc8b3a to
df1b46e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
df1b46e to
6f54b92
Compare
6f54b92 to
3b4c1d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
run_l2.sh:57
- The test_rfc_factory_reset.py test has been moved to run before test_rfc_trigger_reboot.py. If test execution order is significant (e.g., if tests share state or depend on specific sequences), ensure this reordering doesn't break the test suite.
pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_factory_reset.json test/functional-tests/tests/test_rfc_factory_reset.py
pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_trigger_reboot_unknown_accountid.json test/functional-tests/tests/test_rfc_trigger_reboot.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9d7264f to
65d72e1
Compare
fcd79eb to
a3d2502
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
run_l2.sh:55
- This line references test_rfc_factory_reset.py which doesn't exist in the repository. This will cause the test execution to fail. Either the test file needs to be added or this line should be removed.
pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_factory_reset.json test/functional-tests/tests/test_rfc_factory_reset.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5900536 to
fce09ac
Compare
No description provided.