Support for creating custom tokens without service account credentials #183

hiranya911 · 2018-05-31T19:55:56Z

Currently the SDK must be initialized with service account credentials in order to be able to call FirebaseAuth.createCustomToken(). With this PR, the SDK will attempt to sign custom tokens by calling the IAM service in the cloud when the service account credentials are not provided.

// Following will work in GCP managed runtimes where the Metadata service is present FirebaseApp.initializeApp(); String token = FirebaseAuth.getInstance().createCustomToken(uid);

// Following will work anywhere FirebaseOptions options = new FirebaseOptions.Builder() // need to only specify the service account email here... .setServiceAccount("test-service-account@project-id.iam.gserviceaccount.com") .build(); FirebaseApp.initializeApp(options); String token = FirebaseAuth.getInstance().createCustomToken(uid);

go/firebase-admin-sign
go/firebase-admin-iam-sign

hiranya911 · 2018-05-31T19:56:38Z

Resolves #59

schmidt-sebastian

This basically looks good to me. I left some comment nits.

schmidt-sebastian · 2018-06-13T17:45:33Z

src/main/java/com/google/firebase/FirebaseOptions.java

+ /**
+ * Returns the service account email.
+ *
+ * @return The service account email set via {@link Builder#setServiceAccount(String)}


Suggestion: s/service account email/client email address of the service account/

schmidt-sebastian · 2018-06-13T17:46:01Z

src/main/java/com/google/firebase/FirebaseOptions.java

 }

+ /**
+ * Sets the service account email that should be associated with an app.


I would also suggest to say "client email address of the service account" here.

schmidt-sebastian · 2018-06-13T17:46:33Z

src/main/java/com/google/firebase/FirebaseOptions.java

+ * Sets the service account email that should be associated with an app.
+ *
+ * <p>This is used to <a href="https://firebase.google.com/docs/auth/admin/create-custom-tokens">
+ * create custom auth tokens</a>, when service account credentials are not available. Service


Nit: "The email address of a service account can..."

schmidt-sebastian · 2018-06-13T17:57:27Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+ * with that email to sign tokens remotely.
+ * <li>If the code is deployed in the Google App Engine standard environment, uses the
+ * <a href="https://cloud.google.com/appengine/docs/standard/java/appidentity/">App Identity
+ * service</a> to sign tokens.


This seems a little long and contains a lot of detail that I would expect to see in the Admin SDK usage docs, but not necessarily in the API documentation. I tried to reduce this down a bit, let me know what you think:

<p>This method generates a token: <li>Using the private key of {@link FirebaseApp}'s service account credentials if provided at initialization. <li>Using the <a href="https://cloud.google.com/iam/reference/rest/v1/projects.serviceAccounts/signBlob">IAM service</a> if a service account email was provided via ({@link com.google.firebase.FirebaseOptions.Builder#setServiceAccount(String)}) <li>Using the <a href="https://cloud.google.com/appengine/docs/standard/java/appidentity/">App Identity service</a> if the code is deployed in the Google App Engine standard environment. <li>Using the <a href="https://cloud.google.com/compute/docs/storing-retrieving-metadata"> local Metadata server</a> If the code is deployed in a different GCP-managed environment (e.g. Google Compute Engine). </ol> <p>This method throws an exception when all the above fails.

I don't feel super strongly about this if you want to keep it detailed.

schmidt-sebastian · 2018-06-13T17:59:25Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+ } catch (IOException e) {
+ throw new IllegalStateException(
+ "Failed to initialize FirebaseTokenFactory. Make sure to initialize the SDK "
+ + "with a service account credential. Alternatively specify a service account "


s/. Alternatively specify/or/

schmidt-sebastian · 2018-06-13T18:01:56Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+ FirebaseTokenFactory result = this.tokenFactory.get();
+ if (result == null) {
+ synchronized (lock) {
+ result = this.tokenFactory.get();


If I remember my Java 101 class correctly, then you don't need to use an AtomicReference here since you are grabbing a lock already.

http://www.cs.umd.edu/users/pugh/java/memoryModel/jsr-133-faq.html#synchronization
"Synchronization ensures that memory writes by a thread before or during a synchronized block are made visible in a predictable manner to other threads which synchronize on the same monitor. "

Then we'd have to make the variable volatile, to get the double-checked locking semantics to work correctly: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

I don't really have a preference. Drop the AtomicRef and switch to a volatile variable?

schmidt-sebastian · 2018-06-13T18:03:52Z

src/main/java/com/google/firebase/auth/internal/CryptoSigner.java

+ byte[] sign(@NonNull byte[] payload) throws IOException;
+
+ /**
+ * Returns the name of the service account used to sign payloads.


Is this the service account email?

Yes. Updated comments.

schmidt-sebastian · 2018-06-13T18:09:40Z

src/main/java/com/google/firebase/auth/internal/CryptoSigners.java

+ // with the IAM service to sign bytes.
+ HttpRequest request = requestFactory.buildGetRequest(new GenericUrl(METADATA_SERVICE_URL));
+ request.getHeaders().set("Metadata-Flavor", "Google");
+ HttpResponse response = request.execute();


Do we have to to call response.disconnect() here as well?

schmidt-sebastian · 2018-06-13T18:14:50Z

src/test/java/com/google/firebase/auth/internal/CryptoSignersTest.java

+ assertEquals(url, interceptor.getResponse().getRequest().getUrl().toString());
+
+ // Should fall back to signing-enabled credential
+ transport = new MultiRequestMockHttpTransport(


Should this be split into two separate tests?

hiranya911 · 2018-06-14T18:17:14Z

Thanks for the review. Made the suggested changes. Over to @schmidt-sebastian for another look.

schmidt-sebastian · 2018-06-15T21:10:43Z

src/main/java/com/google/firebase/FirebaseOptions.java

+ * Returns the client email address of the service account.
 *
- * @return The service account email set via {@link Builder#setServiceAccount(String)}
+ * @return The client email of service account set via {@link Builder#setServiceAccount(String)}


"of THE service account"

schmidt-sebastian · 2018-06-15T21:11:08Z

src/main/java/com/google/firebase/FirebaseOptions.java

 * <p>This is used to <a href="https://firebase.google.com/docs/auth/admin/create-custom-tokens">
- * create custom auth tokens</a>, when service account credentials are not available. Service
- * account email can be found in the {@code client_email} field of a service account JSON.
+ * create custom auth tokens</a>, when service account credentials are not available. The email


Remove comma.

hiranya911 · 2018-06-25T18:53:17Z

@schmidt-sebastian PTAL at my last commit. This renames serviceAccount to serviceAccountId due to an amendment made to the API proposal.

hiranya911 added 12 commits May 15, 2018 17:22

Ability to sign tokens without a service account credential

b10c89a

Temp fixes for unit tests

0a138f3

Cleaned up the new JWT sign logic

bc7ae70

Cleaned up the new JWT sign logic

664e55d

Updated changelog

be79bb7

Moved token issuer to CyptoSigner interface

3ec0e41

Merged with master

8183e7f

Modifications to the token sign protocol (go/firebase-admin-sign)

a2a20b2

Caching the token factory after first call

eac02ca

Updated documentation

9c648b7

Merged with master

3a72f63

Merged with master

5912990

Updated API docs

9eb007e

hiranya911 requested a review from schmidt-sebastian June 1, 2018 21:16

hiranya911 assigned schmidt-sebastian Jun 1, 2018

hiranya911 added 2 commits June 1, 2018 14:20

Fixing style error

9567164

Updated error message

9504161

schmidt-sebastian reviewed Jun 13, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Jun 13, 2018

Updated documentation

d577e49

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Jun 14, 2018

schmidt-sebastian approved these changes Jun 15, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Jun 25, 2018

Renamed serviceAccount to serviceAccountId

3c4c391

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Jun 25, 2018

Fixed lint error

0f05b65

schmidt-sebastian approved these changes Jun 25, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Jun 25, 2018

hiranya911 added 3 commits July 3, 2018 10:49

Added snippet; updated error message to sync with documentation

eb465ad

Minor API doc updates

e45add3

Merged with master

8e3f4ec

hiranya911 merged commit cd213e0 into master Jul 12, 2018

hiranya911 deleted the hkj-jwt-sign branch July 12, 2018 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support for creating custom tokens without service account credentials #183

Support for creating custom tokens without service account credentials #183

Uh oh!

hiranya911 commented May 31, 2018

hiranya911 commented May 31, 2018

schmidt-sebastian left a comment

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018 •

edited

Loading

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

schmidt-sebastian Jun 13, 2018

hiranya911 Jun 14, 2018

hiranya911 commented Jun 14, 2018

schmidt-sebastian Jun 15, 2018

schmidt-sebastian Jun 15, 2018

hiranya911 Jun 25, 2018

hiranya911 commented Jun 25, 2018

Labels

2 participants

Support for creating custom tokens without service account credentials #183

Support for creating custom tokens without service account credentials #183

Uh oh!

Conversation

hiranya911 commented May 31, 2018

hiranya911 commented May 31, 2018

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented Jun 25, 2018

Labels

2 participants

schmidt-sebastian Jun 13, 2018 •

edited

Loading