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

Replace basic authentication with credentials authentication provider #3136

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Jan 26, 2024

We don't have to merge this one as not sure if it's a good idea and apparently the current basic authentication works well on iframes but this one might not work as well? Anyway the idea is here, if we close it it's ok.
We also don't necessarily have to remove the previous basic authentication, these features could coexist...

As has been briefly discussed before, here's how we could replace our current basic authentication feature with using the new authentication features, with a "Credentials" authentication provider that I already had to set up for authentication tests anyway.

Screen.Recording.2024-01-26.at.12.33.27.mov

@apedroferreira apedroferreira added docs Improvements or additions to the documentation new feature New feature or request labels Jan 26, 2024
@apedroferreira apedroferreira self-assigned this Jan 26, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
@apedroferreira apedroferreira marked this pull request as ready for review February 22, 2024 17:58
@apedroferreira apedroferreira requested a review from a team February 22, 2024 17:59
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 26, 2024
@Janpot
Copy link
Member

Janpot commented Feb 27, 2024

As per last grooming meeting:

  • to test whether this would break iframe embedded apps like we use in notion

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2024
@apedroferreira
Copy link
Member Author

apedroferreira commented Feb 28, 2024

As per last grooming meeting:

  • to test whether this would break iframe embedded apps like we use in notion

Looks like that even if I whitelist requests from iframes, which are all being blocked by the auth middleware right now, the CSRF protection doesn't allow the sign-in to work from a different domain. Maybe I could get around it by removing the same-site headers in the cookies Auth.js sets but that's less secure...
So I guess we either ditch this PR or we don't use it as a replacement for the basic auth we already have.

nextauthjs/next-auth#1151

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation new feature New feature or request PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants