Implementing Firebase SDK Initialization and Custom Authentication by hiranya911 · Pull Request #1 · firebase/firebase-admin-python · GitHub

First iteration of the firebase.App and firebase.auth APIs. These enable the user to initialize the SDK (i.e. create an instance of firebase.App) from a certificate credential, and use it to create custom auth tokens, and verify ID tokens signed by Firebase backend servers.
Unit tests (executable via pytest)
Pylint integration for checking code quality
Initial documentation on running unit tests and using pylint

…nstead of two.

Looks good so far! I very briefly skimmed the actual source and test files. If I should take a closer look because things changed drastically, please let me know.

Is it safe to assume this was copied from some Google internal .pylintrc file?

Google's internal configuration has a bunch of Google specific commands and directives that pylint cannot recognize. So I took the default config used by pylint, and slightly modified it to match some of our internal requirements (e.g. disabling reports, line length 80 etc.). Generally speaking this version of .pylintrc is even more stricter than what we use internally. We can use it for a while and relax some of the constraints to fit our needs in the future.

Great, thanks for clarifying.

How do you feel about this? One of my biggest frustrations with google3 is the very small line lengths. I've been using 120 in the Admin Node.js SDK, but even 100 is better than 80. Now that we aren't in google3, we can tailor this to what we like. If 80 is what you prefer, we can of course keep it.

I'd prefer something higher as well. Lets go with 100. That's what pylint recommends by default. I'll just change the pylint config for now. I'll send another PR with the source files reformatted.

Add "The" to the beginning of this sentence.

Done

s/http/https

Done

Let's drop this last sentence. If there's one thing I've learned, it's always better to under-promise and over-deliver :)

Done

Suggestion to aide those who skim docs:

./lint.sh      # Lint locally modified source files
./lint.sh all  # Lint all source files

Done

There is some great info here! I think in the long-term, we are going to want to have the README be more of a short description of what the project is, how to install and use it, link to the release notes, and links to the relevant documentation. The info on running tests and linting will move in the .github/CONTRIBUTION.md file since most people wont' need that info up front. This is how the Admin Node.js SDK and a lot of our existing open source projects are structured. This can be done in a follow-up PR though and we probably should come up with a common template of sorts to share amongst the Admin SDK repos.

Ack

I'm not reviewing the source and test files that closely since I assume they are mostly copy-pasted. If there are important differences, please call them out for me.

Sources haven't changed at all. But I've reimplemented the tests using pytest (as opposed to unittest). Pytest is a lot more flexible and powerful. Some of the other open source libraries (e.g. oauth2client) have also switched to it.

With pytest our test sources became shorter by about 100 lines, while giving us more test cases than we had :)

Awesome! I gave the tests a closer look then since the code changed in a non-trivial way.

Are we okay checking this private key into the repo? This will be public, so we need to make sure this doesn't actually give anyone access to anything. Ideally we can just use mocked private keys, which is what I did in the Admin Node.js SDK.

I replaced this with the private key at https://github.com/firebase/firebase-admin-node/blob/master/test/resources/mock.key.json

Had to generate a self-signed cert for it to update public_certs.json

Ditto on my message above about the private key.

Used the mock private key as above.

Great, thanks! I know this is a bit picky, but can we also switch out project_id, client_email, client_id, and client_x509_cert_url with clearly mock values (e.g. "mock-project-id", "mock-email@mock-project-id.iam.gserviceaccount.com", "1234567890", and "https://www.googleapis.com/robot/v1/metadata/x509/mock-emai%40mock-project-id.iam.gserviceaccount.com")?

… private key from Node SDK tests for testing

Made the suggested changes. Please take a look at the test sources, since they underwent some changes during migration to pytest.

Awesome, I'm really happy with how this is coming along! I took a deeper dive into the new tests and had some cleanup suggestions. So, back to you for another round.

Great, thanks for clarifying.

Awesome! I gave the tests a closer look then since the code changed in a non-trivial way.

Great, thanks! I know this is a bit picky, but can we also switch out project_id, client_email, client_id, and client_x509_cert_url with clearly mock values (e.g. "mock-project-id", "mock-email@mock-project-id.iam.gserviceaccount.com", "1234567890", and "https://www.googleapis.com/robot/v1/metadata/x509/mock-emai%40mock-project-id.iam.gserviceaccount.com")?

I think this should be get_app(), not initialize_app().

Done

Looks like we don't have tests for delete_app(). Can you add them in this PR or put together a follow-up PR for it? I know the API might be changing according to our API proposal, so it's fine to push this to a later PR to avoid throw-away work.

Sounds good. I'll add it in a future PR. I will have to send another PR anyway to implement the changes proposed in the API proposal.

Can we pull these dynamically from tests/data/service_account.json?

Done. Replaced with CREDENTIAL.project_id

I can't seem to wrap my head around how this works. Can you add a comment explaining what this is?

Added some docstrings explaining what's going on here.

Pull project ID dynamically.

Done

I think this block could be slightly cleaned up:

gcloud_project = os.environ.get(auth.GCLOUD_PROJECT_ENV_VAR)
if gcloud_project:
  del os.environ[auth.GCLOUD_PROJECT_ENV_VAR]

Done

Technically ID tokens do not have a uid claim, so I think we should drop it in this test fixture. It also means we need to add code in the future to explicitly copy the sub claim to the uid claim and add a test to ensure this happens.

Done. Removed the uid claim, and updated the affected test cases to not look for it. We will update those test cases once we implement this claim copy functionality.

…ements in PR

All comments addressed

LGTM. Back to you to merge. Let's see if the Jenkins job to sync repos works...

Hiranya Jayathilaka added 7 commits March 17, 2017 13:27

Initial code contribution with auth API implementation and unit tests

1fc8187

Added linter integration

b0a21b9

Added link to pylint docs

6d05c42

Improved linter script and documentation

b5bf2d0

Updated tests to use pytest; Updated lint.sh to use one config file i…

15641ad

…nstead of two.

Updated documentation

73e4814

Updated documentation

1d9b5a2

hiranya911 requested a review from jwngr March 20, 2017 20:58

hiranya911 assigned jwngr Mar 20, 2017

jwngr suggested changes Mar 21, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 21, 2017

Changed max line length to 100; Updated documentation; Using the mock…

66993ff

… private key from Node SDK tests for testing

hiranya911 assigned jwngr and unassigned hiranya911 Mar 22, 2017

jwngr suggested changes Mar 22, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 22, 2017

Hiranya Jayathilaka added 2 commits March 22, 2017 13:47

Changing mock crypto field names; Implementing other suggested improv…

3ce270c

…ements in PR

Reformatted file to get rid of unnecessary whitespace characters

aa80ca9

hiranya911 assigned jwngr and unassigned hiranya911 Mar 22, 2017

jwngr approved these changes Mar 22, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 22, 2017

hiranya911 merged commit 9c60c17 into master Mar 22, 2017

hiranya911 deleted the hkj-auth-api branch March 23, 2017 20:38

+              Firebase Admin Python SDK enables server-side (backend) Python developers
+              to integrate [Firebase](http://firebase.google.com) into their services
+              and applications. Currently this SDK provides Firebase custom authentication
+              support. Other Firebase APIs will be added soon.

+                  @pytest.mark.parametrize('name', invalid_names)
+                  def test_app_get_with_invalid_name(self, name):
+                      with pytest.raises(ValueError):
+                          firebase.initialize_app(OPTIONS, name)

+                      id_token = get_id_token()
+                      gcloud_project = os.environ.get(auth.GCLOUD_PROJECT_ENV_VAR)
+                      try:
+                          os.environ[auth.GCLOUD_PROJECT_ENV_VAR] = 'mg-test-1210'

+                  def test_no_project_id(self, non_cert_app):
+                      id_token = get_id_token()
+                      gcloud_project = None
+                      if os.environ.has_key(auth.GCLOUD_PROJECT_ENV_VAR):

+                      'iat': int(time.time()) - 100,
+                      'exp': int(time.time()) + 3600,
+                      'sub': '1234567890',
+                      'uid': USER,

+              [FORMAT]
+              # Maximum number of characters on a single line.
+              max-line-length=80

		@@ -1,2 +1,84 @@
		# Firebase Admin Python SDK

		Firebase Admin Python SDK enables server-side (backend) Python developers



		class TestFirebaseApp(object):
		"""Test cases for App initialization and life cycle."""

+                      'kid': 'd98d290613ae1468f7e5f5cf604ead38ca9c8358'
+                  }
+                  payload = {
+                      'aud': 'mg-test-1210',

+                  return AuthFixture(request.param)
+              @pytest.fixture
+              def non_cert_app():

		@@ -0,0 +1,410 @@
		[MASTER]

		@@ -0,0 +1,157 @@
		"""Firebase Admin SDK for Python."""

		@@ -0,0 +1,28 @@
		-----BEGIN PRIVATE KEY-----

		@@ -0,0 +1,12 @@
		{

Implementing Firebase SDK Initialization and Custom Authentication by hiranya911 · Pull Request #1 · firebase/firebase-admin-python · GitHub

Uh oh!

Implementing Firebase SDK Initialization and Custom Authentication - #1

Implementing Firebase SDK Initialization and Custom Authentication#1hiranya911 merged 10 commits intomasterfirebase/firebase-admin-python:masterfrom hkj-auth-apifirebase/firebase-admin-python:hkj-auth-apiCopy head branch name to clipboard

Conversation

hiranya911 commented Mar 20, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Mar 22, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Implementing Firebase SDK Initialization and Custom Authentication#1
hiranya911 merged 10 commits into
masterfrom
hkj-auth-api

hiranya911 Mar 21, 2017 •
edited
Loading