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

login: Support web-based auth methods #36

Closed
gnprice opened this issue Mar 30, 2023 · 6 comments · Fixed by #600
Closed

login: Support web-based auth methods #36

gnprice opened this issue Mar 30, 2023 · 6 comments · Fixed by #600
Assignees
Labels
a-first-hour Issues specific to using the app for the first time a-login beta feedback Things beta users have specifically asked for
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented Mar 30, 2023

I expect the first iteration of a login flow #11 will offer only email/password auth. We should add support for the rest.

In particular this means web-based auth, which covers Google auth and GitHub auth among others. We can split out other methods as separate issues, if we don't find they're easy to just do at the same time. Other forms of auth are tracked separately:

@chrisbobbe
Copy link
Collaborator

For Apple, there's a "Flutter Favorite" plugin sign_in_with_apple.

Probably it makes sense to start with Apple; as I recall, they'll reject apps in App Store review if they have third-party auth but no Apple auth.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented May 24, 2023

One wrinkle to keep in mind (as of writing): although authentication_methods is marked in the doc as deprecated in 2.1.0, it still seems necessary for writing a correct client. Discussion: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/audit.20for.20change.20entries.20vs.2E.20central.20changelog/near/1404115

@gnprice
Copy link
Member Author

gnprice commented May 24, 2023

For Apple, there's a "Flutter Favorite" plugin sign_in_with_apple.

Good to know, thanks.

Probably it makes sense to start with Apple; as I recall, they'll reject apps in App Store review if they have third-party auth but no Apple auth.

That means we'll need Apple auth before a full launch, or even an open beta in the form of an alternate app in the App Store. But I think we'll need web-based auth before that milestone, too, because lots of people rely on that.

One thing that could cause us to prioritize web-based auth or any other auth method sooner than that milestone is if it's an obstacle for a significant number of users who might join an earlier stage of the beta. I think that's unlikely for Apple auth, because I suspect not many people are using it, but is more likely for web-based auth because of people using Google or GitHub auth.

@gnprice gnprice added the m-beta label May 26, 2023
@gnprice gnprice added this to the Beta milestone May 27, 2023
@gnprice gnprice added a-login and removed m-beta labels May 27, 2023
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 30, 2023
Sets up the Flutter deeplink support for android in order
to capture the browser redirect.

Adds the BrowserLoginWidget which is used to co-ordinate
the auth flow, storing the otp temporarily, and finally handling the
browser redirect to complete the auth flow.

Fixes zulip#36
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Sep 1, 2023
Sets up the Flutter deeplink support for android in order
to capture the browser redirect.

Adds the BrowserLoginWidget which is used to co-ordinate
the auth flow, storing the otp temporarily, and finally handling the
browser redirect to complete the auth flow.

Fixes zulip#36
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Sep 1, 2023
Sets up the Flutter deeplink support for android in order
to capture the browser redirect.

Adds the BrowserLoginWidget which is used to co-ordinate
the auth flow, storing the otp temporarily, and finally handling the
browser redirect to complete the auth flow.

Fixes zulip#36
@gnprice gnprice modified the milestones: Beta 1, Beta 2 Nov 8, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 21, 2023
tl;dr:

When this Flutter issue is resolved:
  flutter/flutter#134897
try adding the Noto Color Emoji font back again (COLRv1 format) and
set it as a fallback font behind some higher-priority font wherever
we want to show text that might contain emojis.

Before we do that, we're at the mercy of the system font, which
might not be updated to handle newer emojis. This seems especially
true of Android devices in the wild.

-----

In zulip#245, we added a COLRv1 version of this font, probably because of
a warning that made us doubt if other formats would work on iOS:
  https://github.com/googlefonts/noto-emoji#using-notocoloremoji

> NotoColorEmoji uses the CBDT/CBLC color font format, which is
> supported by Android and Chrome/Chromium OS. Windows supports it
> starting with Windows 10 Anniversary Update in Chrome and Edge. On
> macOS, only Chrome supports it, while on Linux it will support it
> with some fontconfig tweaking, see issue zulip#36. Currently we do not
> build other color font formats.

(It seems like that part of the README wasn't updated when the
COLRv1 format was added, and hasn't been updated since.)

And I guess there wasn't a clear iOS compatibility problem with the
COLRv1 format, so we used that.

But, in zulip#245, it seems like we didn't do a good manual test that the
COLRv1 version of this font could actually handle emojis in the app
on iOS. That testing shows that it can't (or at least couldn't on my
phone or in a simulator). When the font is in the effective list of
fallbacks, it takes responsibility for rendering the emoji, before
the system font gets a chance to, but then it fails to render it,
and a blank space appears. We suspect this Flutter issue ("Rendering
of COLRv1 fonts is broken"):
  flutter/flutter#134897

We don't know why that Flutter issue doesn't, or at least doesn't
always, prevent emojis from successfully rendering in the font on
*Android*. I followed the issue's reproduction recipe with the
COLRv1 font they specified, and indeed the glyphs weren't showing up
on Android (but they did on web). So apparently the specific COLRv1
font is a variable.

Since we don't know what all the variables are, and we have other
short-term priorities, just take the font out of the picture for
now, with a plan to reinstate it once we're sure it won't cause
emojis to fail to render.

Discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Emoji.20rendering.20issue.20.28iOS.29/near/1683965
@gnprice gnprice added the beta feedback Things beta users have specifically asked for label Dec 23, 2023
@gnprice gnprice changed the title login: Support auth methods other than email/password login: Support web-based auth methods Dec 25, 2023
@gnprice
Copy link
Member Author

gnprice commented Dec 25, 2023

I've split this issue up, as foreshadowed in the original description, and edited the description accordingly.

@fizy069
Copy link

fizy069 commented Feb 22, 2024

Hi! I'd like to implement GitHub auth for the app.
I plan on using url_launcher to launch the auth url. The app will wait for the redirect which will return the login code, and this code is used to send a post request to github to retrieve the user.

It would be great if you could assign this issue to me!

@gnprice
Copy link
Member Author

gnprice commented Feb 22, 2024

@fizy069 Hello, welcome!

This will be a complex issue partly because of the several different APIs the implementation needs to integrate with, and partly because it's security-sensitive and so requires an especially close review and a high level of making the code and commits clear so they're easy to review. So it won't be a good fit for one of the first few issues in Zulip to work on.

Instead, take a look at our project board for the app, and try browsing through the issues that are in the "Beta 2" or "Beta 3" milestones but don't have anyone already assigned. Many of those would be good issues to start with.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Mar 28, 2024
@chrisbobbe chrisbobbe added the a-first-hour Issues specific to using the app for the first time label Mar 28, 2024
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Mar 29, 2024
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-first-hour Issues specific to using the app for the first time a-login beta feedback Things beta users have specifically asked for
Projects
Status: Done
3 participants