-
Notifications
You must be signed in to change notification settings - Fork 132
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
Implemented CustomUserClaims #17
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
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.
Thanks for doing this @dominikfoldi. I think you have all the basic elements. The details need a bit of polishing.
…t to string during UserRecord serialization
0371ae8
to
7d98076
Compare
CLAs look good, thanks! |
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.
Thanks @dominikfoldi. This is looking pretty good. I've pointed out few changes. While you're working on them. I'm going to start our internal API review process for this, so we can get it merged and released soon.
FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
…rver to SDK error message mapping
… tests and add error handling if the response of the update API contains incorrect data
…moved unused parts of the tests, simplify claims initialization for TooLargeClaimsPayload test and introduced unit tests for UpdateUserAsync by mocking the httpclient
@hiranya911 I think I am resolved all your requests. Thanks to you I found a bug in the |
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.
Just 2 more comments. I also have submitted this API for internal review. I should hear back in a couple of days. Hopefully we can get this released next week.
…e strings on exception
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. Waiting API review approval for merge.
Thank you for this super fast process! Looking forward to it! |
@dominikfoldi this is released: https://firebase.google.com/support/release-notes/admin/dotnet#1.1.0 Hope you continue to contribute. |
@hiranya911 thank you for the release! Our next step is to integrate Firebase Authentication into our project. But after I am sure that I will contribute more! |
I have implemented the Custom User Claims feature. I do not checked out the contributing guidelines yet but I want to get feedback from you about my changes. I will implement the tests after everything else looks good.
My main guideline was the Java SDK.
@hiranya911 could you please check this out?