Skip to content
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

Merged
merged 14 commits into from
Jan 23, 2019

Conversation

dominikfoldi
Copy link
Contributor

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?

@dominikfoldi dominikfoldi changed the title Implement CustomUserClaims Implemented CustomUserClaims Jan 11, 2019
@googlebot
Copy link

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

Copy link
Contributor

@hiranya911 hiranya911 left a 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.

FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseUserManager.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseUserManager.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/FirebaseException.cs Outdated Show resolved Hide resolved
@hiranya911 hiranya911 self-assigned this Jan 11, 2019
@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor

@hiranya911 hiranya911 left a 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.

Dominik Földi added 4 commits January 15, 2019 23:44
… 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
@dominikfoldi
Copy link
Contributor Author

@hiranya911 I think I am resolved all your requests. Thanks to you I found a bug in the FirebaseUserManager's PostAsync method when the server responds with empty uid, I don't know if could ever happen but I covered that just in case.

Copy link
Contributor

@hiranya911 hiranya911 left a 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.

FirebaseAdmin/FirebaseAdmin/Auth/FirebaseUserManager.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Auth/FirebaseUserManager.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@hiranya911 hiranya911 left a 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.

@dominikfoldi
Copy link
Contributor Author

Thank you for this super fast process! Looking forward to it!

@hiranya911 hiranya911 merged commit 88bcb1b into firebase:master Jan 23, 2019
@hiranya911
Copy link
Contributor

@dominikfoldi this is released: https://firebase.google.com/support/release-notes/admin/dotnet#1.1.0

Hope you continue to contribute.

@dominikfoldi
Copy link
Contributor Author

@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!

@dominikfoldi dominikfoldi deleted the feature-custom-claims branch January 24, 2019 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants