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

feat(flutter_firebase_login): implement sign in with apple #2480

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

juzerali
Copy link

@juzerali juzerali commented May 26, 2021

Status

Ready for review

Breaking Changes

NO

Description

Apple fireauth login

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@juzerali juzerali requested a review from felangel as a code owner May 26, 2021 12:07
@juzerali juzerali force-pushed the firebase-apple branch 2 times, most recently from 02d7f79 to 5ce4226 Compare May 30, 2021 10:37
@juzerali juzerali changed the title WIP: Implement firebase Apple Auth Implement firebase Apple Auth May 30, 2021
@felangel felangel added enhancement New feature or request example Example application labels Jun 8, 2021
Copy link
Collaborator

@marcossevilla marcossevilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juzerali looks great! Left some comments about architecture changes so let me know if you agree. 👍

Thanks for the contribution! 💯

@marcossevilla marcossevilla changed the title Implement firebase Apple Auth feat(flutter_firebase_login): implement sign in with apple Sep 29, 2021
@juzerali juzerali removed their assignment Sep 30, 2021
Copy link
Collaborator

@marcossevilla marcossevilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just minor changes for consistency. Also, if you want to make the crypto class a standalone package would be a bonus but it's up to you. 👍

@juzerali
Copy link
Author

juzerali commented Oct 1, 2021

Overall LGTM, just minor changes for consistency. Also, if you want to make the crypto class a standalone package would be a bonus but it's up to you. 👍

I was thinking of renaming the class name. crypto.dart and crypto package name will collide with actual crypto libraries. Moreover we are not really implementing any crypto algorithms, we are merely using them.

So correct name of file and class should be crypto_util.dart and CryptoUtil. WDYT?

@marcossevilla
Copy link
Collaborator

Overall LGTM, just minor changes for consistency. Also, if you want to make the crypto class a standalone package would be a bonus but it's up to you. 👍

I was thinking of renaming the class name. crypto.dart and crypto package name will collide with actual crypto libraries. Moreover we are not really implementing any crypto algorithms, we are merely using them.

So correct name of file and class should be crypto_util.dart and CryptoUtil. WDYT?

Yes, this crypto class would serve us as a wrapper for the crypto package functionality so we don't have private methods to generate a nonce and encrypt it, as it is part of authentication but can be reused and decoupled from the actual authentication logic. 👍

As we use it as an API for the AuthenticationRepository, CryptoApi sounds better to me. Similar to this example (omitting the example folder). Wdyt?

@juzerali
Copy link
Author

juzerali commented Oct 2, 2021

Overall LGTM, just minor changes for consistency. Also, if you want to make the crypto class a standalone package would be a bonus but it's up to you. 👍

I was thinking of renaming the class name. crypto.dart and crypto package name will collide with actual crypto libraries. Moreover we are not really implementing any crypto algorithms, we are merely using them.
So correct name of file and class should be crypto_util.dart and CryptoUtil. WDYT?

Yes, this crypto class would serve us as a wrapper for the crypto package functionality so we don't have private methods to generate a nonce and encrypt it, as it is part of authentication but can be reused and decoupled from the actual authentication logic. 👍

As we use it as an API for the AuthenticationRepository, CryptoApi sounds better to me. Similar to this example (omitting the example folder). Wdyt?

Makes sense @marcossevilla, I have renamed class to CryptoApi. But I think extracting it in another package is an overkill at the moment. We can do that when a use case arises for another package.

@marcossevilla
Copy link
Collaborator

Overall LGTM, just minor changes for consistency. Also, if you want to make the crypto class a standalone package would be a bonus but it's up to you. 👍

I was thinking of renaming the class name. crypto.dart and crypto package name will collide with actual crypto libraries. Moreover we are not really implementing any crypto algorithms, we are merely using them.
So correct name of file and class should be crypto_util.dart and CryptoUtil. WDYT?

Yes, this crypto class would serve us as a wrapper for the crypto package functionality so we don't have private methods to generate a nonce and encrypt it, as it is part of authentication but can be reused and decoupled from the actual authentication logic. 👍
As we use it as an API for the AuthenticationRepository, CryptoApi sounds better to me. Similar to this example (omitting the example folder). Wdyt?

Makes sense @marcossevilla, I have renamed class to CryptoApi. But I think extracting it in another package is an overkill at the moment. We can do that when a use case arises for another package.

That's ok for me. Just solve the merge conflicts and we're good to go. 👍

@juzerali
Copy link
Author

juzerali commented Oct 4, 2021

The MR is rebased on top of master with all conflicts resolved.

marcossevilla
marcossevilla previously approved these changes Oct 4, 2021
Copy link
Collaborator

@marcossevilla marcossevilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@marcossevilla
Copy link
Collaborator

@juzerali it seems you need to run flutter analyze lib on the flutter_firebase_login project so the checks can pass and we merge this PR

@juzerali
Copy link
Author

juzerali commented Oct 7, 2021

@juzerali it seems you need to run flutter analyze lib on the flutter_firebase_login project so the checks can pass and we merge this PR

Fixed all errors reported by flutter analyze.

@marcossevilla
Copy link
Collaborator

@juzerali there are still some lints left to resolve, they probably don't show up because you're using a different version of Flutter than the CI workflow so I suggest you replicate the CI environment config for the lints to show up. 👍

@marcossevilla
Copy link
Collaborator

marcossevilla commented Oct 26, 2021

hi @juzerali, I left you a PR with the lint fixes so we can merge this one. Please accept it so we can approve yours. 🙏

@marcossevilla marcossevilla mentioned this pull request Oct 27, 2021
7 tasks
@juzerali
Copy link
Author

juzerali commented Nov 1, 2021

@marcossevilla merged

@marcossevilla
Copy link
Collaborator

@juzerali can you fix the failing tests? if you need help, let me know and I can open another PR. 👍

@jasonjackson
Copy link
Contributor

hey all,

was wondering if this ever got to a point where it was working? i pulled down the branch, and it does compile but when i click on the sign in with apple button it gives me an authentication error. if there is still some work to be done, happy to help out and see if i can move it forward.

was it actually at a place where it worked and i am missing something in my local setup? i'd love to see a good example of apple auth since apple now requires it.

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request example Example application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants