feat(flutter_firebase_login): implement sign in with apple

[juzerali]

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

[marcossevilla]

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

Thanks for the contribution! 💯

[marcossevilla]

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]

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]

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]

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]

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]

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

[marcossevilla]

LGTM 🎉

[marcossevilla]

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

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

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

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. 🙏

[juzerali]

@marcossevilla merged

[marcossevilla]

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

[jasonjackson]

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!