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

Allow to set redirect_uri when logging-in thru GitHub (uses OST_GITHUB_CALLBACK_URL) #162

Merged
merged 5 commits into from
Nov 26, 2023

Conversation

spidgorny
Copy link
Contributor

Add NEXT_PUBLIC_APP_URL env

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have a helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added

Copy link

changeset-bot bot commented Nov 25, 2023

🦋 Changeset detected

Latest commit: f3ac05c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 25, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @avitorio on Vercel.

@avitorio first needs to authorize it.

@avitorio
Copy link
Owner

Hey @spidgorny, thanks for the PR!

I'll review it soon.

Would this redirect users to the homepage instead of the dashboard?

Can you elaborate a bit about the use case for this? Thanks!

@spidgorny
Copy link
Contributor Author

spidgorny commented Nov 26, 2023

I have setup a GitHub OAuth authorization. I needed to enter the production redirect_uri during the registration. It works fine in production.
But I could not login on localhost:3000, the GitHub login button would redirect me back to production.
By providing a custom redirect_uri from the .env file on localhost, I've tried to login thru GitHub and make it redirect back to localhost.
It looked like it was working, but later I had to create a completely separate OAuth app just for localhost. This PR may not be that useful in my particular case, but it's a nice feature anyway (if only GitHub supported redirecting to localhost).
Otherwise, just reject it.

@avitorio
Copy link
Owner

avitorio commented Nov 26, 2023

Hi @spidgorny , thanks for the explanation.

Yes, it is indeed annoying that we would need multiple OAuth Apps for different environments.

Apparently GitHub Apps can have multiple callbacks but not OAuth Apps.
We have setup instructions for Github Apps here: https://outstatic.com/docs/github-apps-authentication

It might be the case that your PR is not completely off. We'd need to test with GitHub Apps.

The only thing I'd change is, I'd probably pick a different name for the env var, probably something more explicit like:

OST_GITHUB_CALLBACK_URL

As we need to redirect back to http://localhost:3000/api/outstatic/callback for example, and not to just http://localhost:3000.

Do you want to give this a shot?

@spidgorny spidgorny changed the title Allow to set redirect_uri when logging-in thru GitHub (uses NEXT_PUBLIC_APP_URL) Allow to set redirect_uri when logging-in thru GitHub (uses OST_GITHUB_CALLBACK_URL) Nov 26, 2023
Copy link

vercel bot commented Nov 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
outstatic-dev-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2023 10:25pm
outstatic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2023 10:25pm

@avitorio
Copy link
Owner

@spidgorny Excellent job! Thank you so much for this feature.

@avitorio avitorio self-requested a review November 26, 2023 22:27
Copy link
Owner

@avitorio avitorio left a comment

Choose a reason for hiding this comment

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

LGTM

@avitorio avitorio merged commit 98696b2 into avitorio:canary Nov 26, 2023
3 checks passed
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.

None yet

2 participants