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

[LIBCLOUD-568] Fixing cross service OAuth scopes for Google Compute Engine / DNS / Storage #302

Closed
wants to merge 3 commits into from

Conversation

@erjohnso
Copy link
Member

erjohnso commented May 30, 2014

Prior to this fix, a user could call "get_driver()" for GCE but authorization was only allowed for the "compute" scope. With the addition of the DNS module, users calling its "get_driver()" would only be authorized to the DNS service.

This change allows scopes to be set as keyword params in get_driver() (or via secrets.py) that get propagated down to the authorization connection class. The default behavior is to grant authorization via scopes to all supported google services (compute, storage, dns).

It should be noted that the storage authorization via oauth scopes is bogus, but sets the stage for future enhancements we expect to contribute over the summer.

@wrigri, @franckcuny - A review would be much appreciated!

@erjohnso
Copy link
Member Author

erjohnso commented May 30, 2014

Hmm... not sure what to do about those travis failures,

WARNING:test command found but not installed in testenv

I don't think it has anything to do with my changes and if it does, I'm at a loss on how to interpret the error.

# Default scopes to read/write for compute, storage, and dns. Can
# override this when calling get_driver() or setting in secrets.py
self.scope = None
if kwargs and type(kwargs) is dict:

This comment has been minimized.

@Kami

Kami May 30, 2014 Member

type(kwargs) is dict check shouldn't be necessary since, afaik, if you use ** kwarg expansion it will always be a dict.

This comment has been minimized.

@Kami

Kami May 30, 2014 Member

Actually, there is no need to use kwargs here (right?). If so, you should just explicitly put scope=None in the method signature. Even better would be to use scopes name since the argument value is a list.

@erjohnso
Copy link
Member Author

erjohnso commented Jun 3, 2014

@Kami - I think this is good to go unless you want to hear from @wrigri or @franckcuny.

# Default scopes to read/write for compute, storage, and dns. Can
# override this when calling get_driver() or setting in secrets.py
self.scopes = scopes
if self.scopes is None or not self.scopes:

This comment has been minimized.

@wrigri

wrigri Jun 3, 2014 Contributor

Is there a reason to specifically test for None here? Wouldn't 'if not self.scopes:' work?

This comment has been minimized.

@erjohnso

erjohnso Jun 3, 2014 Author Member

Thanks for looking it over @wrigri!

Old habits. Back when I used to do a lot python development (circa 2002), this was considered a performance tip to use "is" for singleton comparisons. Since we set None as the default value in the method signature, and this change assumes the user will likely never pass in a non-None value for 'scopes', this would have (in my old brain) better performance.

But, I suspect that's either a) no longer a consideration 12 years later, and b) if there is a performance benefit, it's likely masked out by HTTP calls to remote services anyway. ;-)

As I said, just an "old habit". :)

This comment has been minimized.

@wrigri

wrigri Jun 3, 2014 Contributor

So you made me curious, and as it turns out, it looks like 'if not x:' is actually faster than 'if x is None:' even when x is an empty list.

Using Python 2.7.3 on Linux:

timeit.timeit('if a is None: pass', setup='a=None', number=100000000)
2.322221040725708
timeit.timeit('if not a: pass', setup='a=None', number=100000000)
1.8063910007476807
timeit.timeit('if not a: pass', setup='a=[]', number=100000000)
2.0614678859710693

@wrigri
Copy link
Contributor

wrigri commented Jun 3, 2014

Looks good to me

@erjohnso
Copy link
Member Author

erjohnso commented Jun 9, 2014

@Kami - I think this is all set now. :)

@asfgit asfgit closed this in e02fec6 Jun 9, 2014
@Kami
Copy link
Member

Kami commented Jun 9, 2014

Thanks.

I've merged it into trunk with a minor fix - I've changed the default value for scopes attribute from [] to None and initialized empty list inside the function.

The problem is that default values for arguments are created at the time of the function definition. This means that if you would mutate scope argument inside the function, you would actually be mutating the default parameter value which is almost never something you would want.

@erjohnso
Copy link
Member Author

erjohnso commented Jun 10, 2014

Thank you @Kami, nice catch!

@erjohnso erjohnso deleted the erjohnso:LIBCLOUD-568_google_oauth branch Oct 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.