fix(cli): check access process on windows #4293
Conversation
When running on windows, the process `username()` can contain the DOMAIN which makes the comparison fail.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a Windows-specific bug in the process access check where username comparison fails when the process username includes a domain prefix (e.g., "DOMAIN\username"). The fix extracts the username portion after the backslash on Windows systems.
Key changes:
- Added platform-specific handling for Windows usernames in
_can_access_process() - Extracts username from "DOMAIN\username" format by splitting on backslash
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ansys/mapdl/core/cli/stop.py
Outdated
| if os.name == 'nt' and '\\' in process_user: | ||
| return current_user == process_user.split('\\')[-1] |
There was a problem hiding this comment.
The current_user from getpass.getuser() on Windows may also include the domain prefix (e.g., 'DOMAIN\username'). Both usernames should be normalized to extract only the username portion for consistent comparison. Consider splitting current_user as well: current_user.split('\\\\')[-1] == process_user.split('\\\\')[-1]
| if os.name == 'nt' and '\\' in process_user: | |
| return current_user == process_user.split('\\')[-1] | |
| if os.name == 'nt': | |
| # Normalize both current_user and process_user to remove domain prefix if present | |
| return current_user.split('\\')[-1] == process_user.split('\\')[-1] |
There was a problem hiding this comment.
I wasn't able to produce a case where getpass.getuser() contains the DOMAIN so I'm not sure if it makes sense to use it. @germa89 Do you know if copilot can easily hallucinate / overthink when proposing changes ?
There was a problem hiding this comment.
mmhh it does sometimes. I wouldn't bother, if you think the suggestion is not relevant, just discard it.
src/ansys/mapdl/core/cli/stop.py
Outdated
| # Check if we can access basic process info and if it belongs to current user | ||
| current_user = getpass.getuser() | ||
| process_user = proc.username() | ||
| if os.name == 'nt' and '\\' in process_user: |
There was a problem hiding this comment.
[nitpick] Consider using platform.system() == 'Windows' instead of os.name == 'nt' for better readability and clarity, or check both usernames for domain prefixes regardless of platform to make the logic more robust.
| if os.name == 'nt' and '\\' in process_user: | |
| if '\\' in process_user: |
There was a problem hiding this comment.
I'm not sure if that behavior can happen on Linux, at least I wasn't able to reproduce it
There was a problem hiding this comment.
I don't know either, so let's keep it simple :)
for more information, see https://pre-commit.ci
| current_user = getpass.getuser() | ||
| process_user = proc.username() | ||
| if platform.system() == "Windows" and "\\" in process_user: | ||
| return current_user == process_user.split("\\")[-1] |
There was a problem hiding this comment.
I wonder if we should check the domain then too... but I am not sure if that is even the logical thing to do.
germa89
left a comment
germa89
left a comment
There was a problem hiding this comment.
I forgot to mention, you should add a test for this please :)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4293 +/- ##
==========================================
- Coverage 91.29% 91.26% -0.04%
==========================================
Files 193 193
Lines 15742 15745 +3
==========================================
- Hits 14372 14370 -2
- Misses 1370 1375 +5 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
|
@germa89 The current implementation of the tests do not allow me to test without having mapdl installed (or that's my understand from the need to provide a path for |
Description
When running on windows, the process
username()can contain the DOMAIN which makes the comparison fail.Issue linked
None
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)