-
Notifications
You must be signed in to change notification settings - Fork 374
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.
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
fix(auth): Improve customClaims Typing #768
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Looks reasonable to me. @bojeil-google what do you think? Another aspect to consider is, does this change constitute a breaking change for some developers. |
Looks good to me. |
Anything more I can do to get this pushed through? |
hiranya911
requested review from
bojeil-google
and removed request for
rachelmyers
February 14, 2020 18:21
hiranya911
approved these changes
Feb 14, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
hiranya911
changed the title
Improve customClaims Typing
fix(auth): Improve customClaims Typing
Feb 14, 2020
Thank you! |
This was referenced Apr 4, 2020
hiranya911
added a commit
that referenced
this pull request
Apr 16, 2020
* Custom Action for sending Tweets (#784) * Experimental custom Action for sending Tweets * Added license headers * Added README file * Updated package descriptions * Improve customClaims Typing (#768) * chore: Experimental release flow based on Actions (#780) * chore: Experimental release flow based on Actions * Added tarball verification step; Simplified CI trigger * Splitting staging and publish phases into separate jobs * Fleshed out the full workflow * Trigger RC build * chore: Migrated to ESlint (#790) * chore: Migrated to ESlint * Added licesne header * Enabling additional ESLint checks (#794) * chore: Enabling more ESLint checks and fixing errors (#797) * Fix compilation error in integration tests (#798) Introduced by #790 * Build integration tests during CI (and release) (#800) Note that this won't actually run them. Additionally, the *unit* tests are also built, impying that we're building them twice (once during this step, and possibly again when running the unit tests.) * Fix revokeRefreshTokens to round consistently with the other platforms. (#801) This also makes it consistent with the comments a few lines above, as well as the integration test. * feat(auth): Multi-factor Auth support with SMS for Google Cloud Identity Platform (#804) Defines multi-factor auth administrative APIs for Google Cloud Identity Platform. * Defines new MFA types in toc.yaml. (#807) * Removes special char from index.d.ts. (#808) This is causing errors in the reference generation process. * Defines MultiFactor{Create|Update}Settings interfaces. (#809) * Defines MultiFactor{Create|Update}Settings interfaces. * chore: Adding a .npmrc file to the root of the repo (#810) * chore: Adding a .npmrc file to the root of the repo * Removing the root-level .npmrc file * [chore] Release 8.10.0 (#811) * [chore] Release 8.10.0 (take 2) (#812) * Bump acorn from 6.1.1 to 6.4.1 (#815) Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@6.1.1...6.4.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fixing Android notification options descriptions (#820) * Fixing doc bug that conflated sound and tag options for Android notifications. * Removing duplicate documentation for tag. * Adding tag details in the right place this time, hopefully. * chore: Splitting the index.d.ts file into smaller files (#751) * Splitting the index.d.ts file into smaller files * Database return type fixed * chore: Cleaning up package verification scripts (#822) * chore: Cleaning up package verification scripts * Added package metadata to test package.json file * fix(auth): Fixing UserImportRecord typings declaration (#835) * fix(auth): Fixing UserImportRecord typings declaration * Fixing more integration test compilation errors * Trigger CI * Removed redundant line * Bump minimist from 1.2.0 to 1.2.3 (#839) Bumps [minimist](https://github.com/substack/minimist) from 1.2.0 to 1.2.3. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.0...1.2.3) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remerge conflict Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: William Sedlacek <wsedlacekc@gmail.com> Co-authored-by: rsgowman <rgowman@google.com> Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <egilmore@google.com>
hiranya911
added a commit
that referenced
this pull request
Apr 20, 2020
* Firebase ML Node.js SDK Structure (#778) * Firebase ML Node.js SDK Structure * added tests * Added GetModel functionality and tests (#781) * Added GetModel functionality and tests * Added DeleteModel functionality and tests (#782) * Added DeleteModel functionality and tests * Added CreateModel functionality and tests (#788) * Added CreateModel functionality and tests * Added UpdateModel, publishModel,and unpublishModel functionality + tests (#791) * Added UpdateModel, publishModel,and unpublishModel functionality plus tests * Added ListModels functionality for Firebase ML (#795) * Added ListModels functionality for Firebase ML * Firebase ML changed endpoint (#813) * Add ML APIs to docgen toc (#847) * Docstring fixes and additions (#848) * Docstring fixes and additions * A few edits * Ml merge (#851) * Custom Action for sending Tweets (#784) * Experimental custom Action for sending Tweets * Added license headers * Added README file * Updated package descriptions * Improve customClaims Typing (#768) * chore: Experimental release flow based on Actions (#780) * chore: Experimental release flow based on Actions * Added tarball verification step; Simplified CI trigger * Splitting staging and publish phases into separate jobs * Fleshed out the full workflow * Trigger RC build * chore: Migrated to ESlint (#790) * chore: Migrated to ESlint * Added licesne header * Enabling additional ESLint checks (#794) * chore: Enabling more ESLint checks and fixing errors (#797) * Fix compilation error in integration tests (#798) Introduced by #790 * Build integration tests during CI (and release) (#800) Note that this won't actually run them. Additionally, the *unit* tests are also built, impying that we're building them twice (once during this step, and possibly again when running the unit tests.) * Fix revokeRefreshTokens to round consistently with the other platforms. (#801) This also makes it consistent with the comments a few lines above, as well as the integration test. * feat(auth): Multi-factor Auth support with SMS for Google Cloud Identity Platform (#804) Defines multi-factor auth administrative APIs for Google Cloud Identity Platform. * Defines new MFA types in toc.yaml. (#807) * Removes special char from index.d.ts. (#808) This is causing errors in the reference generation process. * Defines MultiFactor{Create|Update}Settings interfaces. (#809) * Defines MultiFactor{Create|Update}Settings interfaces. * chore: Adding a .npmrc file to the root of the repo (#810) * chore: Adding a .npmrc file to the root of the repo * Removing the root-level .npmrc file * [chore] Release 8.10.0 (#811) * [chore] Release 8.10.0 (take 2) (#812) * Bump acorn from 6.1.1 to 6.4.1 (#815) Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@6.1.1...6.4.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fixing Android notification options descriptions (#820) * Fixing doc bug that conflated sound and tag options for Android notifications. * Removing duplicate documentation for tag. * Adding tag details in the right place this time, hopefully. * chore: Splitting the index.d.ts file into smaller files (#751) * Splitting the index.d.ts file into smaller files * Database return type fixed * chore: Cleaning up package verification scripts (#822) * chore: Cleaning up package verification scripts * Added package metadata to test package.json file * fix(auth): Fixing UserImportRecord typings declaration (#835) * fix(auth): Fixing UserImportRecord typings declaration * Fixing more integration test compilation errors * Trigger CI * Removed redundant line * Bump minimist from 1.2.0 to 1.2.3 (#839) Bumps [minimist](https://github.com/substack/minimist) from 1.2.0 to 1.2.3. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.0...1.2.3) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remerge conflict Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: William Sedlacek <wsedlacekc@gmail.com> Co-authored-by: rsgowman <rgowman@google.com> Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <egilmore@google.com> * Added Firebase ML API requirements to Contributing doc (#853) Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com> Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: William Sedlacek <wsedlacekc@gmail.com> Co-authored-by: rsgowman <rgowman@google.com> Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <egilmore@google.com>
This was referenced Apr 21, 2020
Open
This was referenced May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the Type Definition of Object it leaves it on the consumer of
firebase-admin
to override the type definition to access things likecustomClaims.admin
, many users may opt to use any here breaking type safety.With that in mind would it be possible to use something like this typing in order to still allow type safety to some extent without needing the user to override the typing to consume it?
Error without this change
Example code overriding the typing
RELEASE NOTE: Updated typings of the
UserRecord.customClaims
attribute to allow indexing by string keys.