tylerbrawl / Galaxy-Plugin-Rockstar

MIT License
71 stars 7 forks source link

Lost authorization due to many redirects on logon #45

Open FriendsOfGalaxy opened 4 years ago

FriendsOfGalaxy commented 4 years ago

Looks like I've lost authorization silently failed during plugin lifetime, but Galaxy wasn't notified about that. When Galaxy was restarted was trying to use stored credentials and then it failed.

You can use handle_exceptions from galaxy plugins http to handle exceptions that are unknown at hand. (https://galaxy-integrations-python-api.readthedocs.io/en/latest/galaxy.http.html#galaxy.http.handle_exception)

To handle your own excpetions first, you can do:

with handle_exception():
     try: 
         r = await request(*args, **kwars)
     except aiohttp.ClientResponseError as error:
         if error.status == 412:
             raise MyError
         raise error

rockstar.zip

tylerbrawl commented 4 years ago

I just pushed commit 645ee6f3a60ebd23dfcc02ab07901bbb1680f577 to the feature branch in order to attempt to fix this. Essentially, I made sure to raise AuthenticationRequired in the event that this occurs. Still, it would be better to understand why this happens, rather than to just raise an exception when it does. As such, I also added a check for an updated TS019978c2 cookie when making a GET request to https://www.rockstargames.com/auth/get-user.json. I recall this cookie being required when refreshing credentials (hence why it is explicitly added to the request headers), so perhaps the problem was a desynchronization between the session's cookie and what the endpoint expected it to be.

FriendsOfGalaxy commented 4 years ago

Still, it would be better to understand why this happens, rather than to just raise an exception when it does.

Of course. But raising proper exceptions is essential. Without it, we may never now that something goes wrong. In this case it took me a while to figure out where is the problem. Raising as fast as possible for crucial things (like authorization) will save time later.

I can test it again, can you attach build plugin please?

BTW: I'm not sure you are aware of this, but you're using feature branch as "develop" branch in the standard notation. In "gitflow" (and not only) "feature" is a metaphor for a branch created for one new feature. When a single purpose feature is done, it can be merged to master. This gives you flexibility to work on multiple small features simultaneously (but the best is to work on one thing at a time). You have to only remember to sync with master from time to time (I mean: after new changes in master, merge it to all feature and fix branches you work on and know that it may interfere with). And bugs introduced by one feature won't pollute another thing. The same for "hotfix" branches.

In git, creating a new branch is very cheap (just one history footprint, do not to copy files), so don't hesitate to create many to simplify your work, avoid complicated git history and problems with merge conflicts. In most cases you will create feature branches from latest master commit and delete it just after is done and merged to master again. Short living branches are easy to maintain, especially when many people merge their changes to main line. You don't have a big team, but it is still beneficial especially for testing :) In this case I could test your fix_auth_logging branch created from master being sure, that it won't explode because of all changes previously made in feature.

tylerbrawl commented 4 years ago

This implementation makes much more sense. I suppose I was taking what you said before regarding the feature branch too literally. Thanks for the clarification.

Here is the latest build of the feature branch. (I'll remove it and begin working with the separate branches system after I make the pull request to master.)

plugin-0.3.5.zip