Skip to content

feat: customizable session driver - #3610

Merged
SychO9 merged 13 commits into
mainfrom
sm/session-driver
Sep 14, 2022
Merged

feat: customizable session driver#3610
SychO9 merged 13 commits into
mainfrom
sm/session-driver

Conversation

@SychO9

@SychO9 SychO9 commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Part of the security roadmap

Changes proposed in this pull request:

  • Makes the session driver interchangeable (currently, a driver can be selected by setting session.driver in config.php, in the future probably through an advanced admin page UI).
  • Introduces the needed extension API.
  • Adds tests.
  • Adds session driver to info command.

Reviewers should focus on:

  • Currently if the driver is configured to a driver that doesn't exist an exception is thrown, would we rather silently fallback to the default? I feel like that might be more convenient as an admin might disable the extension that provides the driver and crashing with an exception wouldn't be nice.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

SychO9 added 3 commits August 22, 2022 18:46
Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
@SychO9 SychO9 self-assigned this Aug 22, 2022
Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
@luceos

luceos commented Aug 23, 2022

Copy link
Copy Markdown
Contributor
  • Currently if the driver is configured to a driver that doesn't exist an exception is thrown, would we rather silently fallback to the default? I feel like that might be more convenient as an admin might disable the extension that provides the driver and crashing with an exception wouldn't be nice.

I think that makes the most sense. It would however be necessary to add an entry to the logs (error/critical) that provides feedback about this failure to the webmaster.

luceos
luceos approved these changes Aug 23, 2022
…ilable.

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
Comment thread framework/core/src/User/SessionServiceProvider.php
Comment thread framework/core/src/User/SessionManager.php Outdated
Comment thread framework/core/src/User/SessionManager.php Outdated
Comment thread framework/core/src/User/SessionServiceProvider.php
Comment thread framework/core/src/User/SessionManager.php Outdated
Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>
@SychO9
SychO9 requested a review from askvortsov1 August 24, 2022 10:22
Comment thread framework/core/src/Foundation/Console/InfoCommand.php Outdated
Comment thread framework/core/src/Foundation/Console/InfoCommand.php Outdated
Comment thread framework/core/src/User/SessionManager.php Outdated
@SychO9 SychO9 added this to the 1.6 milestone Sep 6, 2022
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9
SychO9 requested a review from a team as a code owner September 6, 2022 21:48
StyleCIBot and others added 2 commits September 6, 2022 21:48
@SychO9
SychO9 requested a review from askvortsov1 September 6, 2022 21:52

@askvortsov1 askvortsov1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@SychO9
SychO9 merged commit f676184 into main Sep 14, 2022
@SychO9
SychO9 deleted the sm/session-driver branch September 14, 2022 17:10
@luceos luceos mentioned this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ completed

Development

Successfully merging this pull request may close these issues.

4 participants