Skip to content

Refactor LSP to use Symfony with test coverage#459

Open
osbre wants to merge 1 commit into
LMMS:masterfrom
osbre:feat/lsp-refactor
Open

Refactor LSP to use Symfony with test coverage#459
osbre wants to merge 1 commit into
LMMS:masterfrom
osbre:feat/lsp-refactor

Conversation

@osbre

@osbre osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

In order to make LSP the best it can be, we have to first make the code "maintainable". This PR refactors LSP and adds test coverage to ensure things keep working in the future. Passwords are now properly hashed using Bcrypt. Whenever someone logs-in, we re-hash the password thanks to Symfony's config.

Live environment if you would like to test it yourself: https://lmmsio-production-lj6ate.laravel.cloud/lsp (note this hosting uses ephemeral storage so files won't last long - not a code bug)

If you have a dedicated .env for production, set LSP_DATA_DIR="%kernel.project_dir%/tmp" there.

I decided to split the previously "all-in-one" route into separate paths, ie /lsp/{id}, but there's now LegacyRedirectResolver for compatibility redirects.

There's now GitHub Actions config to run tests on every change/other PRs. Feel free to ask any follow-up questions.

Don't forget to run composer i locally.

@osbre

osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Dependency lockfile has greatly contributed to the length of this PR changes stats, so don't be too scared 👀 😅

image

@osbre

osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

One more note about passwords: the re-hashed password gets stored in the pw column, because current password column is too small for bcrypt, and since the pw column was previously unused - sounds like finally an opportunity to put it into use without having to modify the existing db schema.

@tresf

tresf commented Jun 28, 2026

Copy link
Copy Markdown
Member

One more note about passwords: the re-hashed password gets stored in the pw column, because current password column is too small for bcrypt, and since the pw column was previously unused - sounds like finally an opportunity to put it into use without having to modify the existing db schema.

My advice is to update the schema and check the hash length before comparing so we can support both old and new. I think pw once stored the temporary unencrypted version. If it's not needed, it should be removed.

@osbre osbre force-pushed the feat/lsp-refactor branch from 0615cfb to 16cdb68 Compare June 30, 2026 19:00
@osbre

osbre commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. Now there's 05-change-password-size.sql and 06-drop-legacy-pw-column.sql migrations attached.

I couldn't find any usage of the pw column in previous codebase, so it's safe to drop.

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.

2 participants