Multi-tenancy #152
Multi-tenancy #152
Conversation
There was a problem hiding this comment.
Thank you @moander for taking this up. We should probably have a discussion at #149 about what this public API should look like before getting into code. However, based on what's implemented here, I'd like to call out the following points:
- Tenant-scoped custom token creation is not yet supported in Firebase Auth.
- The API should look somewhat similar to their Node.js and Java (work in progress) counterparts. That means something like:
var tenantManager = FirebaseAuth.DefaultInstance.TenantManager;
TenanatAwareFirebaseAuth auth = tenantManager.AuthForTenant(tenantId);
await auth.VerifyIdTokenAsync(idToken);
|
@hiranya911 I have added a TenantManager and TenantAwareAuth class.
Please elaborate. I used the node sdk as a reference adding tenant_id to the payload: |
|
Thanks @moander. I see now that the custom token limitation has been since addressed in firebase/firebase-admin-node#708. So your implementation is probably right. Please use #149 to document the public API resulting from this PR. We need to agree on the names of all public interfaces/methods before we look at code changes. This also has to go through our internal API review process, which I can handle separately. |
|
@hiranya911 PR updated with new class names from the Java SDK |
There was a problem hiding this comment.
Thanks @moander. I haven't dug into the tests. But went over the implementation, and pointed out the changes that should be made.
I'd also advise you to only move CreateCustomToken and VerifyIdToken methods to AbstractFirebaseAuth. Otherwise all user management APIs get exposed on TenantAwareFirebaseAuth, which means we need tests for all of that as well.
| @@ -152,6 +156,13 @@ internal static FirebaseTokenVerifier CreateIDTokenVerifier(FirebaseApp app) | |||
| error = $"{this.shortName} has incorrect audience (aud) claim. Expected {this.ProjectId} " | |||
| + $"but got {payload.Audience}. {projectIdMessage} {verifyTokenMessage}"; | |||
| } | |||
| else if (this.TenantId != payload.Firebase?.TenantId) | |||
| { | |||
| var expectedTenantId = this.TenantId == null ? "null" : this.TenantId; | |||
There was a problem hiding this comment.
| var expectedTenantId = this.TenantId == null ? "null" : this.TenantId; | |
| var expectedTenantId = this.TenantId ?? "null"; |
| @@ -42,6 +42,8 @@ internal class FirebaseUserManager : IDisposable | |||
|
|
|||
| private const string IdTooklitUrl = "https://identitytoolkit.googleapis.com/v1/projects/{0}"; | |||
|
|
|||
| private const string IdTooklitTenantUrl = "https://identitytoolkit.googleapis.com/v1/projects/{0}/tenants/{1}"; | |||
There was a problem hiding this comment.
This needs to be the v2 endpoint. v1 doesn't support tenant management.
| /// <summary> | ||
| /// Gets the current instance's <see cref="TenantManager"/>. | ||
| /// </summary> | ||
| public TenantManager TenantManager => this.IfNotDeleted(() => this.tenantManager.Value); |
There was a problem hiding this comment.
Should only be exposed on FirebaseAuth child class.
| /// <summary> | ||
| /// Gets the tenant ID for this auth instance. | ||
| /// </summary> | ||
| public string TenantId { get; } |
| /// </summary> | ||
| public sealed class TenantAwareFirebaseAuth : AbstractFirebaseAuth | ||
| { | ||
| internal TenantAwareFirebaseAuth(FirebaseAuthArgs args) |
There was a problem hiding this comment.
This constructor should accept a tenant ID as an argument. Tenant ID only makes sense at this level. AbstractFirebaseAuth should be completely unaware of all tenant-scoped operations.
|
|
||
| internal string TenantId { get; set; } | ||
|
|
||
| internal static FirebaseAuthArgs Create(FirebaseApp app, string tenantId) |
There was a problem hiding this comment.
Logic for creating FirebaseAuthArgs should go into FirebaseAuth and TenantAwareFirebaseAuth classes.
| @@ -63,22 +65,23 @@ internal FirebaseUserManager(Args args) | |||
| DeserializeExceptionHandler = AuthErrorHandler.Instance, | |||
| RetryOptions = args.RetryOptions, | |||
| }); | |||
| this.baseUrl = string.Format(IdTooklitUrl, args.ProjectId); | |||
| this.baseUrl = string.Format(args.TenantId != null ? IdTooklitTenantUrl : IdTooklitUrl, args.ProjectId, args.TenantId); | |||
There was a problem hiding this comment.
Expand this out for readability.
| this.baseUrl = string.Format(args.TenantId != null ? IdTooklitTenantUrl : IdTooklitUrl, args.ProjectId, args.TenantId); | |
| if (!string.IsNullOrEmpty(args.TenantId) | |
| { | |
| this.baseUrl = string.Format(IdTooklitTenantUrl, args.ProjectId, args.TenantId); | |
| } | |
| else | |
| { | |
| this.baseUrl = string.Format(IdTooklitUrl, args.ProjectId); | |
| } |
| /// <param name="tenantId">The tenant ID to manage.</param> | ||
| public TenantAwareFirebaseAuth AuthForTenant(string tenantId) | ||
| { | ||
| if (tenantId != null && !System.Text.RegularExpressions.Regex.IsMatch(tenantId, "^[a-zA-Z0-9-]+$")) |
There was a problem hiding this comment.
Checking string.IsNullOrEmpty() should be sufficient here.
| throw new ArgumentException("The tenant ID must be a valid non-empty string.", "tenantId"); | ||
| } | ||
|
|
||
| return this.app.GetOrInit<TenantAwareFirebaseAuth>(nameof(TenantAwareFirebaseAuth) + tenantId, () => |
There was a problem hiding this comment.
You need a Dictionary<string, TenantAwareFirebaseAuth> attribute in this class. Then you should GetOrInit from that dictionary instead of FirebaseApp.
When the TenantManager instance is disposed, it can go through all values in the dictionary and dispose each of them.
| @@ -0,0 +1,60 @@ | |||
| // Copyright 2018, Google Inc. All rights reserved. | |||
There was a problem hiding this comment.
2020
Here and in all other new files.
|
This has been implemented and released in a series of separate PRs. |
Usage: