[bug] fixed #14, UnboundLocalError: cannot access local variable 'ldap_session' where it is not associated with a value#15
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sectools library by reorganizing the LDAP functionality into a dedicated module structure and applying code formatting improvements. The changes restructure the existing LDAP code to improve maintainability and follows Python coding best practices.
- Reorganizes LDAP functionality into a dedicated
sectools/windows/ldap/module with separate files for core functionality and wrapper functions - Applies consistent code formatting using black formatter across multiple files including quote normalization and import organization
- Adds comprehensive documentation strings to functions in network utilities and crypto modules
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sectools/windows/ldap/wrappers.py | New file containing LDAP wrapper functions extracted from the original ldap.py |
| sectools/windows/ldap/ldap.py | New file with core LDAP functionality, reformatted and reorganized from original |
| sectools/windows/ldap.py | Original LDAP file removed as part of reorganization |
| sectools/windows/crypto.py | Code formatting improvements and added documentation |
| sectools/network/ip.py | Added comprehensive docstrings and formatting improvements |
| sectools/network/domains.py | Added docstring and formatting improvements |
| sectools/fileformats/init.py | Added all export list |
| sectools/fileformats/Markdown.py | Quote normalization and formatting improvements |
| sectools/data/regex.py | Consistent quote usage across regex definitions |
| sectools/init.py | Removed trailing newline |
| Makefile | Enhanced with additional targets for testing, linting, and documentation |
| .github/workflows/release-tests.yml | New GitHub Actions workflow for release testing |
| .github/workflows/python-pip-build.yml | Updated GitHub Actions to use newer versions |
| .github/workflows/auto_prefix_issues.yml | New GitHub Actions workflow for issue management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.github/workflows/release-tests.yml
Outdated
| cd bhopengraph/tests | ||
| python run_tests.py | ||
|
|
||
| - name: Run tests with verbose output | ||
| run: | | ||
| cd bhopengraph/tests | ||
| python -m unittest discover -v | ||
|
|
||
| - name: Run tests with coverage | ||
| run: | | ||
| pip install coverage | ||
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py |
There was a problem hiding this comment.
The workflow references 'bhopengraph' but this appears to be a 'sectools' project. The path should be 'sectools/tests' instead of 'bhopengraph/tests'.
| cd bhopengraph/tests | |
| python run_tests.py | |
| - name: Run tests with verbose output | |
| run: | | |
| cd bhopengraph/tests | |
| python -m unittest discover -v | |
| - name: Run tests with coverage | |
| run: | | |
| pip install coverage | |
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py | |
| cd sectools/tests | |
| python run_tests.py | |
| - name: Run tests with verbose output | |
| run: | | |
| cd sectools/tests | |
| python -m unittest discover -v | |
| - name: Run tests with coverage | |
| run: | | |
| pip install coverage | |
| coverage run --source=sectools sectools/tests/run_tests.py |
.github/workflows/release-tests.yml
Outdated
| cd bhopengraph/tests | ||
| python run_tests.py | ||
|
|
||
| - name: Run tests with verbose output | ||
| run: | | ||
| cd bhopengraph/tests | ||
| python -m unittest discover -v | ||
|
|
||
| - name: Run tests with coverage | ||
| run: | | ||
| pip install coverage | ||
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py |
There was a problem hiding this comment.
Same issue as above - the path should be 'sectools/tests' instead of 'bhopengraph/tests'.
| cd bhopengraph/tests | |
| python run_tests.py | |
| - name: Run tests with verbose output | |
| run: | | |
| cd bhopengraph/tests | |
| python -m unittest discover -v | |
| - name: Run tests with coverage | |
| run: | | |
| pip install coverage | |
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py | |
| cd sectools/tests | |
| python run_tests.py | |
| - name: Run tests with verbose output | |
| run: | | |
| cd sectools/tests | |
| python -m unittest discover -v | |
| - name: Run tests with coverage | |
| run: | | |
| pip install coverage | |
| coverage run --source=bhopengraph sectools/tests/run_tests.py |
.github/workflows/release-tests.yml
Outdated
| - name: Run tests with coverage | ||
| run: | | ||
| pip install coverage | ||
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py |
There was a problem hiding this comment.
The coverage command references 'bhopengraph' but should reference 'sectools' for both the source and test path.
| coverage run --source=bhopengraph bhopengraph/tests/run_tests.py | |
| coverage run --source=sectools sectools/tests/run_tests.py |
.github/workflows/release-tests.yml
Outdated
| - name: Upload build artifacts | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: bhopengraph-package |
There was a problem hiding this comment.
The artifact name should be 'sectools-package' to match the actual project name.
| name: bhopengraph-package | |
| name: sectools-package |
| if ldap_session is None: | ||
| raise ldap3.core.exceptions.LDAPBindError("Failed to establish LDAP session") |
There was a problem hiding this comment.
The variable 'ldap_session' is not initialized when kerberos authentication is used. Line 188 shows 'ldap_session = None' but the kerberos branch (lines 190-215) creates a connection without assigning it to 'ldap_session'.
No description provided.