Skip to content

Conversation

@philippj
Copy link
Contributor

@philippj philippj commented Jun 4, 2016

For now it is used to store account credentials and creating login codes/confirmation keys.

The account credentials are json dumped and encrypted with the password, in byte format, as key.
Also i did add a newer version of setuptools to the requirements, because a fresh installed debian is failing to compile cryptography with an old version of it.

I tried my best at adapting your coding style, if it just isnt working out, please tell me.

@rossengeorgiev
Copy link
Member

The debian bit should really be a separate issue with more details and some testing before making changes.

I am assuming the rest of this PR is in relation to #32. From what I gather the whole purpose of this class is to create a encrypted file with all credentials.

If you think about a simple application, unless the credentials are provided at runtime by the user, they will be either in the source files or a config file. Encrypting them seem pointless as then the key will be in the source/config files.

So my question is, why does the steam package need this?

@philippj
Copy link
Contributor Author

philippj commented Jun 4, 2016

The point of this is to store the account related information and provide account related features.
Generating login codes/confirmation keys, retrieve confirmations, add phone numbers etc.

And it would be a nice way to store oauth information, so there is no need to login everytime when creating a new mobile session.

@rossengeorgiev
Copy link
Member

I understand what it does. To rephrase my question, is credential management in the project scope?

@philippj
Copy link
Contributor Author

philippj commented Jun 4, 2016

Depends on various factors. Eventually its your decision.
I created this because i thought it was a nice addition for #32 consider implementing a standard way to encrypt and store authenticator secrets and i think the class can be expanded further upon this.

@rossengeorgiev
Copy link
Member

The task in #32 is very specific and still remain an open question whether it should be done. The process of adding an authenticator produces a bunch of secrets. We end up with the a question. Should we just produce a dict of the secrets and draw the line there? Should there be a standard way of persisting those to disk? Technically that's separate from credentials and auth tokens.

You are proposing essentially credential and secrets management, right? Why is this needed? What a difficulties with credentials? How does this help? Are there any problems with this approach?

@philippj
Copy link
Contributor Author

philippj commented Jun 5, 2016

The main reason for implementing credential management is to keep everything organized and in one place, so the credentials are easily accessible.

An addition to this, would be to add some wrapping functions for other parts of the library, which would make them easier to access and improve the overall usability of the library.

The only downside i noticed so far, that it isn't 'secure', like you mentioned earlier, the key for accessing those credentials would be present in some parts of the code, but this would not change with different approaches and i cant think of a real problem with the class in general, except that there could be some naming inconsistencies.

Adding the mobile authenticator can not be fully automated, because of the SMS verification, but setting up an account with the SteamAccount class within a simple CLI application, that would add a whole new perspective to this topic.

@philippj
Copy link
Contributor Author

I have added some features to account.py and added webpresence.py

SteamAccount now provides functions for adding/removing the mobile authenticator and retrieve raw mobile confirmation data

Webpresence provides functions for the ISteamWebUserPresenceOAuth api endpoint and a class for threaded polling.

@rossengeorgiev
Copy link
Member

Hi, I've read your read your response and had a look at the code. This PR seems to be loaded with many things at this point making it hard to discuss. I'll try to break it down.

On the point of having a SteamAccount, I think it might be useful to have an abstraction that consolidates all account related credentials (e.g. user, pass, auth secrets, api key, etc). Also providing a way to initialize and use other parts of the module. However, the code from this PR is not that.

The setuptools change, as I mentioned before, is a separate issue and it should be dealt as such.

Not sure what the purpose of webpresense is, besides just chat, which can be done with the SteamClient. Seems an unnecessary addition. The implementation pattern (using threading) is also not compatible with how the module deals with concurrency.

I've also noticed that the proposed code doesn't use any of functions and features already available in the package, but rather reimplements them. This is undesirable and goes against the DRY principle.

All in all, I cannot merge this PR as is.

@philippj
Copy link
Contributor Author

Thank you for the feedback, I have made some changes.

webpresence removed, setuptools removed from the requirements and cleaned up SteamAccount.

I have added functions for retrieving the web api key and accessors for WebAPI, MobileWebAuth, WebAuth and SteamAuthenticator.

What account related features would be best to implement into SteamAccount, add_phone_numbermaybe?

steam/account.py Outdated

def _has_web_session(self):
if isinstance(self._web_auth, steam.webauth.WebAuth):
return True
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant. Just return isinstance(...)

@rossengeorgiev
Copy link
Member

rossengeorgiev commented Jun 18, 2016

I had a quick look over the code and there is a lot of cruft. Some quick points:

  • Instead of doing all that attribute shuffling put everything in a dict and expose it via __getattr__. See

    steam/steam/guard.py

    Lines 68 to 74 in f37ac61

    self.secrets = secrets or {}
    self.medium = medium
    def __getattr__(self, key):
    if key not in self.secrets:
    raise AttributeError("No %s attribute" % repr(key))
    return self.secrets[key]
  • I don't see what the purpose of encrypting stuff is. I think that is something the application using the package should deal with. Eventually if a common case emerges then consider providing a standard way of doing it. No need to try and solve problems that don't yet exist.
  • I like that that there is flag for whether to register for api key if there isn't one. That's great. However, the default value should be False. A parameters for specifying the hostname would be great as well, with say localhost as default.
  • I see WebAuth bits cause some issues. I don't think those whole process needs to be reimplemented. Just let WebAuth raise its' exceptions. If you have the twofactor secrets, you can simply send the twofactor code on the first request. Mail code and captcha will cause problems. I can't think off the top of my head how to do this so that the API end up nice and clean.

…ameter for get_api_key, changed attributes from instance attributes to variables accessed via __getattr__, proper atexit handling
@philippj
Copy link
Contributor Author

I have made some changes regarding your, suggestions and I will push some cleanup later this day. Also iam working on a separate branch for implementing Mobile Confirmations and Trade offers that will get some accessors in SteamAccount

@philippj
Copy link
Contributor Author

Any thoughts on this?

@philippj
Copy link
Contributor Author

push

@philippj
Copy link
Contributor Author

philippj commented Jun 7, 2017

class added to https://github.com/philippj/steam-totp, pr closed

@philippj philippj closed this Jun 7, 2017
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.

2 participants