Skip to content
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

Modified Secrets Handling to have Alternative Windows Codepath #697

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gosselind1
Copy link

Based on discussion in #695 I have gone ahead and implemented an alternative secrets object for windows, using pywin32.

To summarize the changes:

  1. Secrets providers have been removed from util.py, and move to their own Secrets.py module.
  2. Secret service object is now provided by the module rather than the util module, and imports have been updated accordingly.
  3. Added a function to determine if the application is operating under a msys2 environment.

I've done some testing on windows, but I haven't gotten around to testing this on linux. These changes shouldn't have any impact though.

There also might be some oddities if the system's group policy has been modified to a non-standard configuration, though I've attempted to handle those edge cases.

Additional minor tweaks to class to further prepare for implementation
and operation.
Fixed various bugs with implementation that prevented win32 api calls
from working correctly.
pithos/Secrets.py Fixed Show resolved Hide resolved
pithos/util.py Outdated
Comment on lines 80 to 87
_is_msys2 = None
def is_msys2() -> bool:
global _is_msys2

if _is_msys2 is None:
_is_msys2 = sysconfig.get_platform().startswith("mingw")

return _is_msys2
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this does any blocking IO or anything, you can just simplify it down to:

def is_msys2() -> bool:
    return sysconfig.get_platform().startswith("mingw")

The caching on Linux is because it access the filesystem (but honestly probably doesn't matter in Python much anyway).

@TingPing
Copy link
Member

Overall this looks OK. I need to find time to test it on Linux myself.

I do question why its limited to msys2 and not all of Windows?

@gosselind1
Copy link
Author

I'm using msys2 to run the program on windows, but could be possible without it. This would depend on dependencies and other things. Currently I'm using msys2 to resolve most of the library dependencies, and other software components though.

To check broader more native windows compatibility, I would need to manually compile and install a lot of different pieces of software, but it might be possible? I could change the current utility function to a more generic windows one for now whilst investigating and testing this.

My initial motivation with using msys2 was due to what appeared to be a medium percent of linux related software components that might not have pure windows implementations and or compatibility.

@TingPing
Copy link
Member

TingPing commented Jun 19, 2023

The whole stack supports being built with msvc. You don't really need to test it, I just mean change the check to is_windows() entirely.

@gosselind1
Copy link
Author

Ah. Let me clarify, I had wrote it to target msys2 because I was unsure if it had full native windows support, as I've been getting it to build via msys2. I'll change the function to generically target windows.

Clarified one conditional within secrets library
@gosselind1
Copy link
Author

I got a moment to poke through this again. What's the specific build process for flatpak? I wanted to replicate the failing build, but the flatpak configuration is crashing on code unrelated to the changes I made, and I do not think this is the issue the preference dialog is failing on either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants