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

WIP: Upgrade deps and fix tests #16

Draft
wants to merge 114 commits into
base: master
Choose a base branch
from

Conversation

don-de-marco
Copy link

@don-de-marco don-de-marco commented Jun 3, 2023

I took the liberty and upgraded all dependencies to the latest version available (except the MacOS runtime stuff because I don't have a Mac and can't test it). Mainly because I got annoyed by the pinned pip version. tl;dr: fixed since ages, we just needed to upgrade pip-tools.

Also I made it so that the tests can be executed again. Not that they all run smoothly or return green but at least we can now see to that and fix everything that is broken there. I already removed a lot of tests for which there is no code anymore (mostly only the public profile related tests).

My observations so far: about half of the tests are green, a quarter is red, and the rest just deadlocks and never finishes (imo that's because of websocket stuff where some coroutines just end up waiting endlessly for results).

LauraRozier and others added 30 commits March 16, 2023 03:27
…es i had to a minimal version of this to get the integration working in GOG Galaxy. This version is newer than what i am testing, so it may have errors
… this patch for the current version i have working in the plugin. Still need to add hello message
… webpage to go to step 2. various little bugfixes, including no more infinite loops on getters
…sic code to display all the other pages. This is largely unchanged from the original, but the login page has evidently changed. the code-behind isn't implemented.

Various bugfixes as well
…Windows 11, macOS12,13). Other tweaks, idk anymore
… 2FA, but hopefully i haven't broken anything else
…ning about types overshadowing the standard types. Removed authentication.py. Moved all enums i could find in Steam_network to a dedicated enums.py. Moved All top level functions that may have been called in multiple spots to a utils.py Internally, downgraded my environment to 3.7 as per GOG Galaxy instructions. The resulting file was added to .gitignore
…perly. Removed an exceptions import that did nothing and randomly caused an error. Apparently i don't know how to stash things.
… of that is implemented. oops. layed out the empty methods for it.
…e, the py versions of proto files were not updated
… plugin works from task.py. Not finished; switching gears to localization so i don't go insane.
…n't print like i want them to, which annoys me immensely. Need to retrieve old py files in the repo before calling build install, which broke them
…few conflicts, i'll have to figure out the best way to handle them
…uld now be able to get the latest proto and py files from them
… finally build this thing and it'll run properly.
…d remaining logged in. will likely need to revert some changes in this commit, and possibly others back to the original fork.
@don-de-marco don-de-marco marked this pull request as draft June 3, 2023 15:18
@don-de-marco don-de-marco changed the title WIP: Upgrade deps fix tests WIP: Upgrade deps and fix tests Jun 3, 2023
@@ -2,8 +2,8 @@ galaxy.plugin.api==0.69

# steam_network backend
protobuf==3.19.4
Copy link
Owner

Choose a reason for hiding this comment

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

With better proto, we can update this to a much newer version as well, though i'm still working out the kinks there.

Copy link
Author

@don-de-marco don-de-marco Jun 3, 2023

Choose a reason for hiding this comment

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

We could use a newer protobuf, too, if Gog wouldn't still use python 3.7. Protobuf 3.19 is the latest compatible version for a python this old

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it's not ideal. As i mentioned before I'd prefer python 3.10+ because i want match aka a switch statement (but python is too much of a hipster to call it that).

@ABaumher
Copy link
Owner

ABaumher commented Jun 3, 2023

@TheSentry - Tagging you in so you see this.

I know @TheSentry was trying to find time to get to fixing some of the test cases but other things (i.e. work and personal life) have taken priority, which is completely understandable. I personally don't have nearly the time i used to, most of my work here lately has just been reading the various comments, commits, etc.

I'm assuming everything still builds and works properly with the newer versions? For the most part i inherited the pinned versions and ran into breaking changes whenever i tried to update them, so i just left them alone. One thing i did find though was that pip-tools needs to be at least 5.4.0 or it does not work on MacOS (credit @bgebhardt for that). One issue i was constantly getting was a get_argspec call which is deprecated to hell but some older modules used it. using the newest pip would break that, but maybe that's taken care of by updating a bunch of other pinned versions as well. I'll check their changelogs when i get a moment.

@don-de-marco
Copy link
Author

I'm assuming everything still builds and works properly with the newer versions? For the most part i inherited the pinned versions and ran into breaking changes whenever i tried to update them, so i just left them alone. One thing i did find though was that pip-tools needs to be at least 5.4.0 or it does not work on MacOS (credit @bgebhardt for that). One issue i was constantly getting was a get_argspec call which is deprecated to hell but some older modules used it. using the newest pip would break that, but maybe that's taken care of by updating a bunch of other pinned versions as well. I'll check their changelogs when i get a moment.

I've been using the updated dependencies locally for a few days now and there seem to be no errors so far; so I assume that the most important components still work as expected. However, I didn't do much manual testing besides that so I'd appreciate if someone else could try it out and report back how it went.

websockets==8.1
dataclasses-json==0.4.2
vdf==3.4
pyobjc-framework-CoreServices==5.1.2; sys_platform == 'darwin' # this one could use an update to 9.1.1 I guess; according to docu it's tested with 3.7 regularly
Copy link
Owner

Choose a reason for hiding this comment

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

I can try to get our Mac dev to try that latest version. The other potential issue is the newer versions might have some Mac issues we don't know about. I doubt it but i know from my own testing that pip tools 5.1.2 was broken on Mac (which we inherited from the old code). It required 5.4.0. A newer version should still work there but it's something we'll need to confirm with all of these updated versions.

Copy link
Owner

Choose a reason for hiding this comment

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

https://pyobjc.readthedocs.io/en/latest/changelog.html#backward-incompatible-changes
I have no idea where we use this but this might be an issue. Idk if we abused the optional argument rule before they nixed it in 8.0

Copy link
Author

@don-de-marco don-de-marco Jun 8, 2023

Choose a reason for hiding this comment

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

I'm wondering why we even need to include this specific dependency. We don't use any Objc bindings anywhere so this appears to me to be a band aid to fix another dependency on MacOS and since I don't know which dependency that might be, I cannot tell whether or not that change breaks our neck.

Copy link
Owner

Choose a reason for hiding this comment

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

Uri Scheme Handler uses it. i have no idea why/what it does though. See

from CoreServices.LaunchServices import LSCopyDefaultHandlerForURLScheme
from AppKit import NSWorkspace
def is_uri_handler_installed(protocol):
bundle_id = LSCopyDefaultHandlerForURLScheme(protocol)
if not bundle_id:
return False
return NSWorkspace.sharedWorkspace().absolutePathForAppBundleWithIdentifier_(bundle_id) is not None

@ABaumher
Copy link
Owner

ABaumher commented Jun 7, 2023

I'm working on a new version that uses betterproto. I'm going to copy in the pinned version changes here. I'm going to release this as alpha 1.0.6 so hopefully we can get a few test dummies to see if both work properly. If this all still works, i'm going to move forward cleaning up the entire code base, except the caches, which i barely understand and likely need a clean-up as well. As far as test cases are concerned, the outside calls should not change so that should not be affected.

Unfortunately, there are some people out there that do not trust GOG's browser for authentication. I do not understand how they expect it to work, but in this case it's actually (unfortunately) something we can handle. We literally are acting like a full steam client, just like steam.exe uses. Our communications are over TLS just like the OpenID web version they'd prefer. 🤨
Anyway, it involves a new html page and requires they manually copy/paste RSA info - we provide them the pulic key steam gives us, they take that key and generate their enciphered password and paste it back in. Our code (and also, critically, GOG) can't decipher it even if it wanted to. Normally, we get the user's password and do this ourselves, but w/e.

this is so dumb.png
Then again, how can they trust that whatever rsa tool they decide to use won't steal their password?

@engiefox
Copy link

engiefox commented Jun 7, 2023

That...seems excessive. I understand not trusting "foreign" programs, but adding more hoops doesn't seem like the solution after a certain point. That whole issue thread is a mess of opinions lol.

@engiefox
Copy link

engiefox commented Jun 7, 2023

I especially love the guy running sandboxes for every program he uses, just....wow. I cannot imagine the overhead they expend for that.

@engiefox
Copy link

engiefox commented Jun 7, 2023

I think the analogy of a lock is apt here. Locks aren't meant to be impenetrable, they just add enough difficulty to make what's inside not worth the effort of breaking them. I honestly doubt a bad actor is going to waste all the effort to exploit Steam's login process for a few video games and in my experience things like phishing are much more of a concern to the regular user than any questionable security shortfall from Steam itself. apologies for sharing opinions in a technical thread, but you're already dealing with a lot and adding other people's hangups to the pile isn't going to help.

@don-de-marco
Copy link
Author

I'm working on a new version that uses betterproto. I'm going to copy in the pinned version changes here. I'm going to release this as alpha 1.0.6 so hopefully we can get a few test dummies to see if both work properly. If this all still works, i'm going to move forward cleaning up the entire code base, except the caches, which i barely understand and likely need a clean-up as well. As far as test cases are concerned, the outside calls should not change so that should not be affected.

From what I saw, these caches are more or less just data classes which Galaxy only serializes to JSON and then saves the result into the SQLite database somewhere in %PROGRAMDATA%. There's probably not much we can clean up since this is just how it works.

Unfortunately, there are some people out there that do not trust GOG's browser for authentication. I do not understand how they expect it to work, but in this case it's actually (unfortunately) something we can handle. We literally are acting like a full steam client, just like steam.exe uses. Our communications are over TLS just like the OpenID web version they'd prefer. 🤨 Anyway, it involves a new html page and requires they manually copy/paste RSA info - we provide them the pulic key steam gives us, they take that key and generate their enciphered password and paste it back in. Our code (and also, critically, GOG) can't decipher it even if it wanted to. Normally, we get the user's password and do this ourselves, but w/e.

There's one crucial thing wrong with their arguments: we don't use the browser embedded into Galaxy for authentication. We only and solely use the browser to render our HTML to get the credentials from the user. All the authentication and authorization stuff is performed purely in the Python code. No browsers involved.

And, fwiw, implementing OpenID is not that easy on our end. Yes, we could ask the user to extract the token returned by Valve's authentication website (it's probably just a JWT) and provide it via a HTML form during authentication similar to how the MFA code thing is implemented. But that will be a nasty and really ugly hack since the OpenID authentication websites expect you to provide a return URL to which the token is sent (usually as a parameter in the URL). What website can we use? Our authentication attempt comes from within an application, there's no way we can let the OpenID website know that. If Galaxy supported data URIs we could maybe return the token via a URI like galaxy://openid-return/steam?token=<token> and have them inject it by a message via their plugin IPC comm channel. But that's nothing we use/do if it's not already implemented by Galaxy - and I do honestly suspect that they implemented that or would be willing to do so "just for us".

@ABaumher
Copy link
Owner

ABaumher commented Jun 8, 2023

From what I saw, these caches are more or less just data classes which Galaxy only serializes to JSON and then saves the result into the SQLite database somewhere in %PROGRAMDATA%. There's probably not much we can clean up since this is just how it works

I'm not sure but it may be possible to optimize those writes. If we write to the cache all the time it's incredibly inefficient, especially if the db file is closed and reopened. Most OS have an optimal block size for reading/writing, so for extremely large amounts of data that arrives at different times it may be useful to have a stream and write in chunks. It may be necessary when dealing with huge libraries to get all their data before that 10 minute timeout.

That said, this is definitely a very low priority thing. I guess a higher priority would be making it easier to understand if it's poorly written (which it probably is because everything here is)

@ABaumher
Copy link
Owner

ABaumher commented Jun 8, 2023

But that will be a nasty and really ugly hack since the OpenID authentication websites expect you to provide a return URL to which the token is sent (usually as a parameter in the URL). What website can we use?

It's technically possible to provide a callback url and use a locally-hosted site, but to do this we need to use an http server (the url would be roughly http://127.0.0.1/<port number> It'd also be http, afaik Mozilla is deprecating http so this approach would fail eventually), but this would massively increase the difficulty of code development/maintenance. Python has a module for that, but it'd further bloat our install size for minimal gain. It would allow us to make the UI a little cleaner - that site we display can send requests back to our python code so it wouldn't close and pop back up, it'd always be up. But the maintainability cost of this would be nuts. We'd be running full js instead of the bare minimum in our html currently, and pulling data from an HTTP server python instance, which would likely need to be in a separate process to not bottleneck our code, etc.

Ngl if someone raises an issue on this it's getting WontFixed

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.

8 participants