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

Support authorization in datastore #6

Merged
merged 2 commits into from
Feb 16, 2016
Merged

Conversation

akariv
Copy link
Member

@akariv akariv commented Feb 15, 2016

We change the way the datastore authenticates requests.
It used to be with API keys, now it's with a cryptographic token.

Some other changes here:

  • Use the package owner when generating urls
  • Modify the url for the authentication lib

Also take into account the owner when creating the urls.
return True
return False
global public_key
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use global?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to have a singleton pattern here, so that we load the public_key only once and lazily (i.e. only on first request). How would you do it?

Copy link
Member

Choose a reason for hiding this comment

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

I'd just load it with the module. But global is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - the reason I didn't go that way was to avoid a network request upon importing the module.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@pwalsh
Copy link
Member

pwalsh commented Feb 16, 2016

@akariv I'm done. Some comments to address before merge. Most are trivial points, but I think you should at least create an issue for the refactor of the class pattern.

@akariv akariv force-pushed the feature/datastore-authorization branch from 18d4d28 to 46e37a0 Compare February 16, 2016 09:20
@akariv
Copy link
Member Author

akariv commented Feb 16, 2016

@pwalsh - I fixed some of the issues and created #7

@pwalsh
Copy link
Member

pwalsh commented Feb 16, 2016

@akariv great. looks good. ok to merge.

akariv added a commit that referenced this pull request Feb 16, 2016
@akariv akariv merged commit 39c2293 into master Feb 16, 2016
@akariv akariv deleted the feature/datastore-authorization branch March 24, 2016 20:48
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