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 AuthCookie for Login with SameSite cookie option #129

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

alexookah
Copy link
Contributor

@alexookah alexookah commented Aug 3, 2024

What

This PR enhances the loginCookie functionality by introducing a new AuthCookie class. This class adds options for configuring the cookie's expiration and SameSite attribute. This should resolve this issue.

Why

Currently, the authentication cookie being set is session-only. This means that when a user closes their browser, they need to re-authenticate upon reopening. This change allows for persistent authentication sessions, reducing the need for frequent logins.

How

This PR introduces the AuthCookie class which replaces the default behavior for setting authentication cookies. The AuthCookie class:

Provides the option to set a custom expiration time for the authentication cookie.
Supports the SameSite attribute for cookies, which enhances security by controlling how cookies are sent with cross-site requests.
Allows users to set the cookie as persistent if they opt for the "remember me" functionality, which is currently not supported by default.

Testing Instructions

Login using a Login provider. Verify cookies and check that SameSite is set to None.

Additional Info

Things to improve: Add options in admin for samesite configuration & domain cookie.

Checklist:

  • My code is tested to the best of my abilities.
  • My code follows the WordPress Coding Standards.
  • My code has proper inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I included the relevant changes in CHANGELOG.md

@alexookah alexookah changed the title Add AuthCookie Add AuthCookie for Login with Cookie SameSite option Aug 3, 2024
@alexookah alexookah changed the title Add AuthCookie for Login with Cookie SameSite option Add AuthCookie for Login with SameSite Cookie Aug 3, 2024
@alexookah alexookah changed the title Add AuthCookie for Login with SameSite Cookie Add AuthCookie for Login with SameSite Cookie option Aug 3, 2024
@alexookah alexookah changed the title Add AuthCookie for Login with SameSite Cookie option Add AuthCookie for Login with SameSite cookie option Aug 3, 2024
Copy link
Member

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Reviewing from my phone, but issue seems to be method name youre using.

(Feel free to ingore the other feedback if you want, happy to handle myself when I'm implementing docs/tests)

src/Auth/AuthCookie.php Outdated Show resolved Hide resolved
src/Auth/AuthCookie.php Outdated Show resolved Hide resolved
src/Auth/AuthCookie.php Outdated Show resolved Hide resolved
src/Auth/AuthCookie.php Outdated Show resolved Hide resolved
src/Auth/Auth.php Outdated Show resolved Hide resolved
@justlevine
Copy link
Member

@alexookah I've rebased this on the current develop branch to fix some issues with ci and testing in WP 6.6. Please make sure to pull --force before committing/pushing any additional changes to this PR.

@coveralls
Copy link

coveralls commented Aug 25, 2024

Coverage Status

coverage: 81.787% (-0.1%) from 81.924%
when pulling 1eed7c4 on alexookah:custom_wp_auth_cookie
into 72936f4 on AxeWP:develop.

src/Auth/AuthCookie.php Outdated Show resolved Hide resolved
@justlevine
Copy link
Member

I've begun implementing a more scalable UX that should provide a path forward on this in #141 .

So far, settings have been broken up into different screens, so we can scale horizontally instead of vertically:
image

Once I clean up the provider settings and conditional logic, we should be able to rebase this paying closer attention to what Login/ Cookie settings should be available on a provider level versus globally.

@justlevine justlevine mentioned this pull request Oct 20, 2024
14 tasks
@justlevine
Copy link
Member

Rebased on #141 #144 .

There's still more work to be done on the admin refactor (e.g. stop saving state onChange() ), but that can now be handled independently of this, since the settings registry (PHP) is now fully decoupled from the frontend package.

I think the next step for this PR (beyond the remaining unresolved comments) is to either see if we can more sensibly organize the order of the Access Control settings, or if we should relocate them to a new Cookies tab (or similar).

@justlevine
Copy link
Member

Refactored cookie settings to their own group in 7024674

image

@alexookah
Copy link
Contributor Author

@justlevine New UI looks awesome!
Whats pending now?

@justlevine
Copy link
Member

Integration (WPUnit) tests are the big one. Everything else I believe can be handled iteratively after this is merged/released.

I also need to fix the setting value denouncing before triggering a release, but that's outside the scope of this PR.

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