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

Add support for Mozilla Accounts prompt none auth flow #5104

Merged
merged 20 commits into from
Oct 10, 2024
Merged

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Sep 26, 2024

References:

Jira: MNTOR-3492

Description

Adds support for Mozilla Accounts promt=none authentication flow. The initial use cases will be a promotional link from Mozilla Accounts. We expect them to link to us with the following UTM parameters that we can use to initiate the promt=none authentication flow:

  • utm_source=moz-account
  • utm_campaign=settings-promo
  • utm_content=monitor-free

How to test

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added a unit test to test for potential regressions of this bug.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@flozia flozia self-assigned this Sep 26, 2024
Copy link

@flozia flozia requested review from rhelmer and Vinnl and removed request for rhelmer September 26, 2024 16:21
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Code looks good. However, if I visit http://localhost:6060/?utm_campaign=settings-promo&utm_content=monitor-free without being logged into FxA (with the flag enabled), I get the Next-Auth error page:

image

That's me ending up at http://localhost:6060/api/auth/signin?error=Callback.

Same if I'm signed in to Monitor, then sign out, and then visit that URL again. The only way I can get it to work, is by signing in to Relay, and then visiting the URL.

This also seems like exactly the type of use case where a couple of end-to-end tests would be useful, perhaps?

src/app/(proper_react)/layout.tsx Show resolved Hide resolved
src/app/functions/universal/attributions.test.ts Outdated Show resolved Hide resolved
src/constants.ts Show resolved Hide resolved
import { authOptions } from "../../utils/auth";

const handler = NextAuth(authOptions);
// There is currently no support for handling OAuth provider callback errors:
// https://github.com/nextauthjs/next-auth/discussions/8209
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the comment, hopefully they add this eventually.

@flozia
Copy link
Collaborator Author

flozia commented Oct 9, 2024

Code looks good. However, if I visit http://localhost:6060/?utm_campaign=settings-promo&utm_content=monitor-free without being logged into FxA (with the flag enabled), I get the Next-Auth error page:

Thanks for catching this issue @Vinnl! I did not run into this issue myself before, but was able to reproduce it in private browsing mode. If a user is not authenticated with FxA we get a callback error. I’m handling this error now in the NextAuth handler here.

This also seems like exactly the type of use case where a couple of end-to-end tests would be useful, perhaps?

Absolutely! I ran into some limitations around feature flags while adding E2E tests (something we need to introduce a workaround for) for the silent authentication flow. I have an “almost ready to push” branch that addresses the issue around feature flags and I’m adding the E2E tests as a quick follow-up.

Edit: Opened PR #5150 for adding the E2E tests.

@flozia flozia requested a review from Vinnl October 9, 2024 08:00
@flozia flozia merged commit 239e452 into main Oct 10, 2024
16 checks passed
@flozia flozia deleted the mntor-3492 branch October 10, 2024 13:05
Copy link

Cleanup completed - database 'blurts-server-pr-5104' destroyed, cloud run service 'blurts-server-pr-5104' destroyed

`${process.env.SERVER_URL}/api/auth/callback/fxa?error=`,
)
) {
return NextResponse.redirect(`${process.env.SERVER_URL}/user/dashboard`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@flozia Shouldn't this be going to / rather than /user/dashboard/? Now, if I'm not signed in, I get redirected to FxA, then back to Monitor, then to the dashboard, where I see a flash of an error page, before being redirected back to FxA, but this time not a prompt=none flow, so I'm still asked to sign in. (If that's what we want, we could just not do a prompt=none flow, right?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Vinnl Redirecting to / might be safer in this case. The idea behind redirecting to /user/dashboard was to initiate authentication as a fallback, but there might be too many redirects — I’ve opened PR #5192.

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.

3 participants