Fix/ldap levels feedback#539
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
============================================
+ Coverage 51.48% 51.52% +0.03%
Complexity 420 420
============================================
Files 65 65
Lines 2451 2467 +16
Branches 256 261 +5
============================================
+ Hits 1262 1271 +9
- Misses 1079 1082 +3
- Partials 110 114 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| </div> | ||
|
|
||
| <button id="searchBtn">Search User</button> | ||
| <button id="searchBtn">Login</button> |
There was a problem hiding this comment.
Ldap subtitle on line 4 above is still search user ?
There was a problem hiding this comment.
My bad - I've updated the subtitle for each level.
| element.classList.remove("hidden"); | ||
| } | ||
|
|
||
| function getSubtitleForLevel(url) { |
There was a problem hiding this comment.
Instead of relying on URL, use method called getCurrentVulnerabilityLevel. it will provide the level of vulnerability.
There was a problem hiding this comment.
I've used vulnerabilityLevelSelected instead of getCurrentVulnerabilityLevel as it was resulting in undefined.
| if (url.includes("LEVEL_4")) { | ||
| return "Search sanitized user input using LDAP filter."; | ||
| } | ||
| if (url.includes("LEVEL_5")) { |
There was a problem hiding this comment.
Better to create small json object config and use it instead of if/else.
level_config=
{
LEVEL_1 : "",
LEVEL_2: ""
}
| document.getElementById("ldapSubtitle").textContent = subtitle; | ||
|
|
||
| if ( | ||
| url.includes("LEVEL_1") || |
There was a problem hiding this comment.
This if can also move to config
|
Overall looks good. can you please change the if/else in js file to config in the same js file and derive logic as that is better and easier to maintain. |
|
Overall, PR looks good and is ready to merge once the config change is done. Thanks a lot for all the effort @antriksh-9 |
a1241ed to
4a55f4b
Compare
4a55f4b to
2e9eb14
Compare
preetkaran20
left a comment
There was a problem hiding this comment.
Amazing work @antriksh-9 ❤️ Merging it now.
completes: #526
fixes: #517