search

wordpress-mobile / WordPress-Login-Flow-Android

Pluggable WordPress login flow for Android
GNU General Public License v2.0
14 stars 3 forks source link

Encode email delimiter chars defined in RFC 3986 #73

Closed jd-alexander closed 1 year ago

jd-alexander commented 2 years ago

Fixes https://github.com/wordpress-mobile/WordPress-Android/issues/15480

Related PRs:

Description

Encodes delimiters in email addresses when calling the auth-options endpoint, based on the RFC 3986 specification.

See: Similar fix in iOS.

More info in this issue comment: https://github.com/wordpress-mobile/WordPress-Android/issues/15480#issuecomment-1202342661.

🚀 It also updates the Kotlin Version to 1.6.10 to restore Buildkite CI checks success when building this Repo.

To Test: Follow the testing steps from the integrating PR: https://github.com/wordpress-mobile/WordPress-Android/pull/15526.

Merging Strategy

Notice ✅ One review should be enough to merge these PRs. Check the merging strategy in the WordPress-Android PR.

ovitrif commented 1 year ago

@ParaskP7 I'm requesting your review since this is also bumping the Kotlin version, as we've agreed early today (ref: p1659510263238549/1659449153.829029-slack-C02QANACA) 👋 🙇 .

I didn't run into issues when manually testing the login & signup flow, though your feedback on what to check is needed before proceeding with this 💯 .

ParaskP7 commented 1 year ago

👋 @ovitrif !

Thank you for the ping and for adding me as a reviewer here! 🙏

@ParaskP7 I'm requesting your review since this is also bumping the Kotlin version, as we've agreed early today (ref: p1659510263238549/1659449153.829029-slack-C02QANACA) 👋

Ah, I thought this would be a separate PR, where we will just update the Kotlin version, smoke test that change in isolation to any other change, then after merging this change first, you would update the PR and use this new Kotlin version here.

But, if that all sounds too complicated, we can do it this way too, especially if you are in a hurry... 😅

PS: Is it okay if I do the review tomorrow?

ovitrif commented 1 year ago

PS: Is it okay if I do the review tomorrow?

That's perfectly fine for me @ParaskP7, no rush 👍 .

Ah, I thought this would be a separate PR, where we will just update the Kotlin version, smoke test that change in isolation to any other change, then after merging this change first, you would update the PR and use this new Kotlin version here.

That's probably a better idea, the reason I started with this approach is because I wasn't 💯 sure the problem was caused by the older Kotlin version, and this came as I was investigating why both the CI environment and myself couldn't build this branch 😄 .

I could prepare a PR only for the Kotlin version bump, although the changes in this PR helped find out the issue, and apparently the problem appears when we're referencing a dependency compiled with a Kotlin version newer than what's currently on trunk for this repo.

What do you suggest? I'm fine with both.

If I don't have to prepare another PR I promise to spend the saved time wisely, investigating another user-facing Groundskeeping issue 😅 (the stakes have been raised 👀).

ParaskP7 commented 1 year ago

👋 @ovitrif !

That's perfectly fine for me @ParaskP7, no rush 👍 .

Coolio 👍 !

That's probably a better idea, the reason I started with this approach is because I wasn't 💯 sure the problem was caused by the older Kotlin version, and this came as I was investigating why both the CI environment and myself couldn't build this branch 😄 .

😄

I could prepare a PR only for the Kotlin version bump, although the changes in this PR helped find out the issue, and apparently the problem appears when we're referencing a dependency using a Kotlin version newer than what's currently on trunk for this repo.

👍

What do you suggest? I'm fine with both.

If I don't have to prepare another PR I promise to spend the saved time wisely, investigating another Groundskeeping issue 😅.

😅 Let's keep it simple then this time, but let's be aware that for next time it might be beneficial if we have separate PRs and testing for such changes, especially when that involves Gradle, AGP, Kotlin, Dagger or other such major libraries.

In this case, the version bump is also significant, from 1.4.32 to 1.6.10, 2 major versions up, thus this might also require a bit more focused review and testing. As long as you are comfortable with this change, and the testing you did shows that everything is okay, I am okay to review it too.

PS: If I could chose, I would change your promise to instead spend the saved time testing this Kotlin change instead of another Groundskeeping issue... 😛

ovitrif commented 1 year ago

PS: If I could chose, I would change your promise to instead spend the saved time testing this Kotlin change instead of another Groundskeeping issue... 😛

Sounds fair to me ! 🧘🏻 🚦

ovitrif commented 1 year ago

👋 @ovitrif !

I have reviewed this PR, mainly focusing on the Kotlin change and any side effects (didn't find any), everything LGTM and thank you for making the upgrade and also for adding the extra OVERRIDE_DEPRECATION suppression to get rid of this new warning. 💯

It it a 👍 from my side on the Kotlin change. I also tested it with composite builds to verify that nothing weird is happening on that side too, that is, when enabling both localFluxCPath and localLoginFlowPath as local builds. ✅

FYI: I am going to let the testing and merging part for someone else (more fit for it, thinking @mkevins) since this change involves multiple PRs, and I am seeing Matthew has already reviewed most of them.

Many thanks for reviewing & testing thoroughly the Kotlin version upgrade @ParaskP7 🙇 , you are awesome 🥇 💯 🚀

I'm glad everything went ok also in your testing 👍

I'm afraid @mkevins might have forgotten to approve this one when testing the whole changeset integrated in the WordPress-Android PR https://github.com/wordpress-mobile/WordPress-Android/pull/15526, we'll probably will only be able to merge this on Monday since his weekend started a bit earlier this week ✈️ 🌎 🇺🇸, 😃.

ovitrif commented 1 year ago

Indeed, I hadn't realized the WordPress-Login-Flow was also part of this since the ref update was not in the changeset at the time I reviewed it 😅 .

True, I made a mistake there not updating the integrating PR to reference to the Login-Flow version of this PR 🥲.

I have retested this (via the main WordPress-Android app PR) and this is working as expected. Also, these code changes look good. Nice work Ovi!

Thanks a lot for testing again the changes 🙇 , I've merged with trunk and updated the FluxC-Android version to the new one that includes the PR we've merged to FluxC last week 🎉

One last version juggling and then I can merge this & the WordPress Android PR 🚀 :shipit: