Skip to content

Allow createCustomToken() to work with tenant-aware auth #708

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Dec 16, 2019
Merged

Allow createCustomToken() to work with tenant-aware auth #708

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8b02f7a
Allow createCustomToken() to work with tenant-aware auth
rsgowman Nov 28, 2019
4648c08
Missed an await()
rsgowman Dec 5, 2019
08dfb17
remove unnecessary test
rsgowman Dec 6, 2019
53e6e6a
Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…
rsgowman Dec 10, 2019
565989d
Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…
rsgowman Dec 10, 2019
e749059
review feedback
rsgowman Dec 10, 2019
a971e4a
oops
rsgowman Dec 10, 2019
f9d8d66
Option c: Tenant aware TokenGenerator.
rsgowman Dec 11, 2019
cf4f87a
Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…
rsgowman Dec 11, 2019
f6937e4
more review feedback
rsgowman Dec 12, 2019
0bccb66
firebase.auth!() -> client_auth()
rsgowman Dec 12, 2019
3f33a5e
Merge branch 'master' into rsgowman/tenant_customtoken
rsgowman Dec 12, 2019
0067272
client_auth => clientAuth
rsgowman Dec 13, 2019
c70bc3b
Merge branch 'master' into rsgowman/tenant_customtoken
rsgowman Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/auth/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,18 +626,16 @@ export class TenantAwareAuth extends BaseAuth<TenantAwareAuthRequestHandler> {

/**
* Creates a new custom token that can be sent back to a client to use with
* signInWithCustomToken().
* signInWithCustomToken(). The tenant id will be embedded in the token and
* will be verified during the call to signInWithCustomToken().
*
* @param {string} uid The uid to use as the JWT subject.
* @param {object=} developerClaims Optional additional claims to include in the JWT payload.
*
* @return {Promise<string>} A JWT for the provided payload.
*/
public createCustomToken(uid: string, developerClaims?: object): Promise<string> {
// This is not yet supported by the Auth server. It is also not yet determined how this will be
// supported.
return Promise.reject(
new FirebaseAuthError(AuthClientErrorCode.UNSUPPORTED_TENANT_OPERATION));
return this.tokenGenerator.createCustomTokenWithTenantId(uid, this.tenantId, developerClaims);
}

/**
Expand Down
37 changes: 33 additions & 4 deletions src/auth/token-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ interface JWTBody {
exp: number;
iss: string;
sub: string;
tenant_id?: string;
}

/**
Expand Down Expand Up @@ -254,13 +255,38 @@ export class FirebaseTokenGenerator {
* service account key and containing the provided payload.
*/
public createCustomToken(uid: string, developerClaims?: {[key: string]: any}): Promise<string> {
return this.createCustomTokenInternal(uid, null, developerClaims);
}

/**
* Creates a new Firebase Auth Custom token.
*
* @param {string} uid The user ID to use for the generated Firebase Auth Custom token.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the current pattern but we should move away from re-stating the types, eg. string in the javadocs.
This is according to the internal typescript readability guideline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very happy to do this. (Done.) Obviously, I've only done this for the methods I've touched in this PR (createCustomToken*).

* @param {string} tenantId The tenant ID to use for the generated Firebase Auth Custom token.
* @param {object} [developerClaims] Optional developer claims to include in the generated Firebase
* Auth Custom token.
* @return {Promise<string>} A Promise fulfilled with a Firebase Auth Custom token signed with a
* service account key and containing the provided payload.
*/
public createCustomTokenWithTenantId(
uid: string, tenantId: string, developerClaims?: {[key: string]: any}): Promise<string> {
if (!validator.isNonEmptyString(tenantId)) {
throw new FirebaseAuthError(
AuthClientErrorCode.INVALID_ARGUMENT,
'`tenantId` argument must be a non-empty string uid.');
}
return this.createCustomTokenInternal(uid, tenantId, developerClaims);
}

private createCustomTokenInternal(
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively to having two separate createCustomToken*() methods, we could have one with an options object, i.e.:

interface CreateCustomTokenOptions {
  tenantId?: string;
  developerClaims?: {[key: string]: any};
}
public createCustomToken(uid: string, options?: CreateCustomTokenOptions) { ... }

uid: string, tenantId: string | null, developerClaims?: {[key: string]: any}): Promise<string> {
let errorMessage: string;
if (typeof uid !== 'string' || uid === '') {
errorMessage = 'First argument to createCustomToken() must be a non-empty string uid.';
if (!validator.isNonEmptyString(uid)) {
errorMessage = '`uid` argument must be a non-empty string uid.';
} else if (uid.length > 128) {
errorMessage = 'First argument to createCustomToken() must a uid with less than or equal to 128 characters.';
errorMessage = '`uid` argument must a uid with less than or equal to 128 characters.';
} else if (!this.isDeveloperClaimsValid_(developerClaims)) {
errorMessage = 'Second argument to createCustomToken() must be an object containing the developer claims.';
errorMessage = '`developerClaims` argument must be a valid, non-null object containing the developer claims.';
}

if (typeof errorMessage !== 'undefined') {
Expand Down Expand Up @@ -296,6 +322,9 @@ export class FirebaseTokenGenerator {
sub: account,
uid,
};
if (tenantId) {
body.tenant_id = tenantId;
}
if (Object.keys(claims).length > 0) {
body.claims = claims;
}
Expand Down
18 changes: 18 additions & 0 deletions test/integration/auth.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,24 @@ describe('admin.auth', () => {
expect(userRecord.uid).to.equal(createdUserUid);
});
});

it('createCustomToken() mints a JWT that can be used to sign in tenant users', () => {
return tenantAwareAuth.createCustomToken('uid1')
.then((customToken) => {
firebase.auth().tenantId = createdTenantId;
return firebase.auth().signInWithCustomToken(customToken);
})
.then(({user}) => {
return user.getIdToken();
})
.then((idToken) => {
return tenantAwareAuth.verifyIdToken(idToken);
})
.then((token) => {
expect(token.uid).to.equal('uid1');
expect(token.firebase.tenant).to.equal(createdTenantId);
});
});
});

// Sanity check OIDC/SAML config management API.
Expand Down
82 changes: 34 additions & 48 deletions test/unit/auth/auth.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

'use strict';

import * as jwt from 'jsonwebtoken';
import * as _ from 'lodash';
import * as chai from 'chai';
import * as sinon from 'sinon';
Expand All @@ -28,7 +29,6 @@ import * as mocks from '../../resources/mocks';
import {Auth, TenantAwareAuth, BaseAuth, DecodedIdToken} from '../../../src/auth/auth';
import {UserRecord} from '../../../src/auth/user-record';
import {FirebaseApp} from '../../../src/firebase-app';
import {FirebaseTokenGenerator} from '../../../src/auth/token-generator';
import {
AuthRequestHandler, TenantAwareAuthRequestHandler, AbstractAuthRequestHandler,
} from '../../../src/auth/auth-api-request';
Expand Down Expand Up @@ -328,63 +328,49 @@ AUTH_CONFIGS.forEach((testConfig) => {
}

describe('createCustomToken()', () => {
let spy: sinon.SinonSpy;
beforeEach(() => {
spy = sinon.spy(FirebaseTokenGenerator.prototype, 'createCustomToken');
});

afterEach(() => {
spy.restore();
it('should return a jwt', async () => {
const token = await auth.createCustomToken('uid1');
const decodedToken = jwt.decode(token, {complete: true});
expect(decodedToken).to.have.property('header').that.has.property('typ', 'JWT');
});

if (testConfig.Auth === TenantAwareAuth) {
it('should reject with an unsupported tenant operation error', () => {
const expectedError = new FirebaseAuthError(AuthClientErrorCode.UNSUPPORTED_TENANT_OPERATION);
return auth.createCustomToken(mocks.uid)
.then(() => {
throw new Error('Unexpected success');
})
.catch((error) => {
expect(error).to.deep.equal(expectedError);
});
it('should contain tenant_id', async () => {
const token = await auth.createCustomToken('uid1');
expect(jwt.decode(token)).to.have.property('tenant_id', TENANT_ID);
});
} else {
it('should throw if a cert credential is not specified', () => {
const mockCredentialAuth = testConfig.init(mocks.mockCredentialApp());

expect(() => {
mockCredentialAuth.createCustomToken(mocks.uid, mocks.developerClaims);
}).not.to.throw;
it('should not contain tenant_id', async () => {
const token = await auth.createCustomToken('uid1');
expect(jwt.decode(token)).to.not.have.property('tenant_id');
});
}

it('should forward on the call to the token generator\'s createCustomToken() method', () => {
const developerClaimsCopy = deepCopy(mocks.developerClaims);
return auth.createCustomToken(mocks.uid, mocks.developerClaims)
.then(() => {
expect(spy)
.to.have.been.calledOnce
.and.calledWith(mocks.uid, developerClaimsCopy);
});
});
it('should throw if a cert credential is not specified', () => {
const mockCredentialAuth = testConfig.init(mocks.mockCredentialApp());

it('should be fulfilled given an app which returns null access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return nullAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});
expect(() => {
mockCredentialAuth.createCustomToken(mocks.uid, mocks.developerClaims);
}).not.to.throw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be a mismatch with the test description (should throw if...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh; that's odd. Seems to go back to #285 which is literally called 'Support for creating custom tokens without service account credentials'. So the test was adjusted as part of that, but the test description was not. I've fixed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

But wait! That's wrong too, since this test can't call "the IAM service in the cloud when the service account credentials are not provided." So instead of throwing immediately, it now returns a failed future. Fixed (for real this time) by ensuring it does fail, but not immediately. (Might be worth checking with @hiranya911 to see if this captures the original intent.)

});

it('should be fulfilled given an app which returns invalid access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return malformedAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});
it('should be fulfilled given an app which returns null access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return nullAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});

it('should be fulfilled given an app which fails to generate access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return rejectedPromiseAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});
}
it('should be fulfilled given an app which returns invalid access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return malformedAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});

it('should be fulfilled given an app which fails to generate access tokens', () => {
// createCustomToken() does not rely on an access token and therefore works in this scenario.
return rejectedPromiseAccessTokenAuth.createCustomToken(mocks.uid, mocks.developerClaims)
.should.eventually.be.fulfilled;
});
});

it('verifyIdToken() should throw when project ID is not specified', () => {
Expand Down
Loading