Defines TenantAwareAuth #551
Conversation
…nk APIs, OIDC/SAML provider config mgmt APIs.
…uth API requests. Adds detailed tenant mismatch error for uploadAccount on tenant Id mismatch for TenantAwareAuth.
| utils.getProjectId(auth.app), | ||
| new TenantAwareAuthRequestHandler(auth.app, tenantId), | ||
| cryptoSignerFromApp(auth.app)); | ||
| utils.addReadonlyGetter(this, 'tenantId', tenantId); |
There was a problem hiding this comment.
Just mark the constructor argument as public readonly
There was a problem hiding this comment.
One problem with this is that if they are not using typescript, they can overwrite this without any error thrown. I tried it and the property can be overwritten. Since this is publicly available, i think we should enforce the readonly.
There was a problem hiding this comment.
I expected the TS compiler to generate a readonly getter for this, but looks like that only happens if we expose this as a getter in the code. So this is ok.
There was a problem hiding this comment.
I expected the TS compiler to add a readonly getter, but looks like that only happens if we define a getter explicitly here. So this is ok.
src/auth/auth.ts
Outdated
| */ | ||
| export class Auth extends BaseAuth implements FirebaseServiceInterface { | ||
| public INTERNAL: AuthInternals = new AuthInternals(); | ||
| private tenantsMap: {[key: string]: TenantAwareAuth}; |
test/unit/auth/auth.spec.ts
Outdated
| mockCredentialAuth.verifyIdToken(mocks.generateIdToken()); | ||
| }).to.throw(expected); | ||
| }); | ||
| it('should not throw on valid tenant ID', () => { |
There was a problem hiding this comment.
This is pretty much the same as the following test case. Consider removing it.
| @@ -47,6 +49,15 @@ chai.use(chaiAsPromised); | |||
| const expect = chai.expect; | |||
|
|
|||
|
|
|||
| interface AuthTest { | |||
There was a problem hiding this comment.
This file is bit complicated to review. Can I assume it's just running the existing tests in 2 test configs, with the tests specific to an auth implementation enclosed in if-blocks?
There was a problem hiding this comment.
Yeah, I completely understand. This is the same GitHub diff issue. This should no longer be an issue in the next PRs.
You are mostly right. Same tests are run for both. There are cases where I add specific tests for tenant specific behavior. For example:
- createCustomToken should throw unsupported tenant operation error
- verifyIdToken with token missing or mismatching tenant ID
- verifySessionCookie with missing or mismatching tenant ID
- importUsers with explicit mismatching tenant IDs
- confirm createSessionCookie calls verifyIdToken before making call to create a session cookie
I do this by checking testConfig.Auth === TenantAwareAuth
| utils.getProjectId(auth.app), | ||
| new TenantAwareAuthRequestHandler(auth.app, tenantId), | ||
| cryptoSignerFromApp(auth.app)); | ||
| utils.addReadonlyGetter(this, 'tenantId', tenantId); |
There was a problem hiding this comment.
I expected the TS compiler to add a readonly getter, but looks like that only happens if we define a getter explicitly here. So this is ok.
|
Thanks for the review! |
Defines TenantAwareAuth and its user management APIs, email action link APIs, OIDC/SAML provider config mgmt APIs.