Skip to content

Ability to instantiate the API via a session id#77

Closed
manneohrstrom wants to merge 14 commits intomasterfrom
28019_session_handling
Closed

Ability to instantiate the API via a session id#77
manneohrstrom wants to merge 14 commits intomasterfrom
28019_session_handling

Conversation

@manneohrstrom
Copy link
Copy Markdown
Contributor

Adds a constructor parameter session_token which makes it possible to create an API instance without either username/password or script/script key.
A session token can be generated via the new generate_session_token() method.

If a shotgun API instance is constructed using a username/password
combination, a session token is requested from the server and this
session token is used instead of the username/password to authenticate
for the remainder of the session. Furthermore, the password member
variable is cleared, meaning that there is no password string persisting
in memory during the lifetime of the API instance.
manneohrstrom added a commit to shotgunsoftware/tk-core that referenced this pull request Feb 10, 2015
Note that this change requires the modified shotgun API here:
shotgunsoftware/python-api#77
This makes it easier to create new shotgun API instances based on the
same proxy settings as another instance.
This is raised whenever a session id fails to authenticate.
Comment thread shotgun_api3/shotgun.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could use a comment as to how it could be used. Since it isn't used in the API itself it isn't obvious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

heh, initially i had a comment but removed it given the coding style. It's back now! :)

Comment thread shotgun_api3/shotgun.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That feels less fragile. Only thing I'd say is to comment on that 102 to say where it comes from. Otherwise it is just a magic number.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will fix!

@jfboismenu
Copy link
Copy Markdown
Contributor

You reverted your commit where you cleared the username/password after authenticating. Why? Is it just performance? Or it is because it's not part of our current mandate?

@manneohrstrom
Copy link
Copy Markdown
Contributor Author

I reverted the commit about the username/password because there is a subtle regression which makes it trickier to deal with than I first thought: If you have a shotgun session which was instantiated with a login/password, this session is currently operational forever, because it keeps sending the username and password back to the server. It is, however, storing the password as a member variable, which is not good practice.

If we dropped the password at construction time and instead continued with a session token, the API session wouldn't be valid forever anymore -- if you left it idle for 24 hours and then picked it back up again it would raise an AuthenticationFault. We have some places where we have long running loops and stuff going, so although subtle, we probably need to communicate this change to clients explicitly prior to a release. (Where as the rest of the stuff in this pull request is just adding new features!)

@jfboismenu
Copy link
Copy Markdown
Contributor

Gotcha. Totally makes sense.

manneohrstrom added a commit that referenced this pull request Feb 26, 2015
Adds a constructor parameter session_token which makes it possible to
create an API instance without either username/password or script/script
key. A session token can be generated via the new get_session_token()
method.

Closes #77
@manneohrstrom
Copy link
Copy Markdown
Contributor Author

this is now merged into the toolkit security branch.

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.

3 participants