RFC Move public accessible code into a separate directory

[usox]

Hi there

This is a small (ok, not so small) request for comments.
What happend?

• I moved all files (php, assets) which are supposed to be public accessible into a newly created src/public directory.
• All composer dependencies which contain assets (js libs) will be installed to src/public/lib/components/

Why?

• Security considerations
• Better repository organization (best practice to have src/ and tests/ directories)
• Generel modernization

There is still much work to do, so I'd like to request some feedback from you first.

[lachlan-00]

Impressively large! thanks for taking your time to put this change up!

I'm sure we have some experts around but I am not sure about the why or the hows about this. can you elaborate on why this should happen?

I am very happy to make changes like this but i just don't really do anything besides Ampache so i'd just like to understand this better. I'm also sure others will be able to weigh in on this too.

[usox]

@lachlan-00 Sure, I'll explain it a bit. It's about best practises in the first place.

PHP Repository structure

Most php repositories follow a certain structure. This is described here https://github.com/php-pds/skeleton
Modern IDEs like phpstorm support ("understand") those structures what makes it easier for developers which are mostly familiar with that structure.

public-Folder

Having a separate public directory is a common way to prevent non-public files to be served by the webserver (The DocumentRoot of the webserver has to be src/public). This is mostly done for security reasons - just imagine one of the project dependencies has a security vulnerability which is remotly triggerable by providing url-parameters (This already happened in the past in other projects).

In example, an attacker can call https://ampache-instance/lib/vendor/composer/installed.json - this provides him with a lot of informations (installed packages, versions). A good start, if someone wants to do something bad (and this could be done fully automatically).
Sure, .htaccess may help - as long, as you use the apache webserver (and allow .htaccess to be parsed). But from a security-point-of-view, no one should be bothered with placing and reading .htaccess files - just prevent those files from being served and everyone is happy (except attaclers :-) )

A common goal is having just a single php-file (entryscript) in the public-folder which takes care of every request.

Yep, this one is huge - it's a RFC for now. I consider making the needed changes in an iterative way to keep the PRs as small as possible.

[wagnered]

The frameworks such as Laravel, symfony, etc approach the organization In this manner. They are "content management systems". They need to be more concerned about security. For it's purpose, I think Ampache is adequately secure.

But I think the organization is more for increased development efficiency not security. That's why I push migrating to Laravel. For example, most developers consider images, Audio, etc as media not sources.

Also, I would need to know that the new architecture is 100% functional, or at leastfunctionally equivalent to the current architecture.

[kuzi-moto]

I'm a big fan of this change. I have thought about how nice it would be to have all the web content separate from everything else. Thanks for the work, I would be happy to help test as well to see if there are any issues. Given how messy the code is, it should get a through look over.

It will be a breaking change, so a major version would be required for this. I wonder if there is a way to include additional notes in the update notifications for when it is implemented. Maybe Ampache can show the git commit message?

[usox]

Thanks for your feedback, @wagnered !

For it's purpose, I think Ampache is adequately secure.

Uhm well, that's your POV. From my POV, separating the core from all those assets is the easiest way to achieve more security with zero effort.
No one knows, in which environment ampache runs, which webserver are used and how the systems are configured. This puts the "normal" user in danger (e.g. https://www.acunetix.com/vulnerabilities/web/phpunit-remote-code-execution/).

Frameworks like laravel urge the developer to use such an approach, but this is also standard in most custom enterprise-grade projects I've seen so far.

Sure, regarding ampache this means work in the first place, but imho everyone will benefit from it.

Also, I would need to know that the new architecture is 100% functional, or at leastfunctionally equivalent to the current architecture.

Well, I'm happy to prove that :-)

But if you consider switching to standard frameworks like laravel, it's ok for me, too (although I'm not a big fan of using frameworks in big projects). Just let me know, I'm happy to help.

[usox]

I'm a big fan of this change. I have thought about how nice it would be to have all the web content separate from everything else. Thanks for the work, I would be happy to help test as well to see if there are any issues. Given how messy the code is, it should get a through look over.

It will be a breaking change, so a major version would be required for this. I wonder if there is a way to include additional notes in the update notifications for when it is implemented. Maybe Ampache can show the git commit message?

@kuzi-moto I think, this will take a while. As said, I prefer some kind of iterative approach as there are many changes needed to make this really work (More autoloading, Dependency injection, Unit-Testing, ...).

[wagnered]

The DocumentRoot of the webserver has to be src/public).

Just to play Devil's advocate, Not according to The document your refer to or Laravel's architecture. The public folder is at document root in both.

Any way, l like Laravel because it is very active, has many built in tools, helpers and packages to ease development. It is also arguably the most popular framework.

If you are curious, you can checkout the legacy-laravel branch and see what has been done so far. The work done was during my learning period so you can make light of it. 😇

[lachlan-00]

Thank you for the detailed comments @usox

Oh god, unit testing is something we need in here so bad.

@kuzi-moto we do have "view changes" but it might be good to start putting notes into the update area when notifying. Since changing the changelog style it's actually been the best source for info so it might be a good link to add as well

It might be good to branch this into a repo branch that other people can use/pull to as well.

[usox]

Thank you for the detailed comments @usox

Yw.

It might be good to branch this into a repo branch that other people can use/pull to as well.

Sure, that could work - seems to be the best way to test it out. Some preparations have to be done in develop, though - e.g. separate prefixes for the internal and external paths, removal of all those require calls, ...
I'd like to keep the real breaking change as small as possible to avoid huge changesets - this could be PITA to merge with the ongoing progress in develop.

If you consider creating a new major version (as @kuzi-moto did), maybe it's time to raise the minimum php version as well?

[lachlan-00]

The next steps for me are working on the api, so i can slow down on the other features. I've fixed a lot of the code issues i wanted to touch first so i think we branch this and then i start work on the api.

That way we have a clean point in time break to work from where i'll only be bugfixing non-api code. so uploads outside the api class should be minimal.

I can edit the branch for this pull so i'll make the change tomorrow after work and that's where we go. I will also give you merge into that branch similar to the api client branch we have for @AshotN

also i think @wagnered mentioned it but is src denoted anywhere as a required folder or just public? i've only seen public sources in my (minor) experience with other projects.

unless there are other folders in src would it be simpler to just leave it as /public?

[wagnered]

Pleae understand that https://github.com/php-pds/skeleton is not a standard. I just installed 7 of the 10 frameworks recommended by DZone. All but YII2 has the public folder at the root. YII has the index.html in the root folder.

As for an attacker finding and using a JavaScript file adversely, a non conforming framework would increase defence of this type of attack, especially if directory listing is disabled in the web server. With a conforming framework, the attacker would more likely be able to guess the location of important files. What is highly protective is the use of middleware.

Considering you seem highly reluctant to use the word Laravel in your comments that you are concerned about a steep learning curve. I won't deny the fact that for me it was very steep. I am one who requires explicit examples and although there is an abundance of help and examples, I didn't find any explicitly explained.

My Laravel branch might fill in any gaps of understanding.

[usox]

Furthermore, I would like to apologize for mixing up two things here.

First: The public-folder

Imho it doesn't directly matter whether it's located within src or not. Moving the public-directory out of the src-folder can also have some advantages (I've seen it in some projects, too). The important thing is to have a separate place for public accessible files.
In each scenario, the entry-scripts (e.g. artist.php) have to be stripped down to something like that:

<?php

// header, copyright, yada yada

require_once 'path/to/init.php';

(new \Ampache\Application\ArtistApplication())->run();

This will ensure, the Application-Logic is located outside of public.

Second: The repository structure

My goal here is to adapt some common rules regarding the structure of (php) repositories - this will help developers to understand and maintain the code and will help to establish a more strict structure of the code itself.

For example the repository root:

/
/ bin / <-- Current bin folder
/ config / <-- New folder for the ampache config-File
/ docs / <-- Current docs folder
/ public / <-- The folder for public accessiable code; php-files in here will have no namespace
/ resources / <-- Common resourced for bootstrapping (e.g. the sql-file), developer scripts, ...
/ src / <-- The root-folder for the php sources and for the (new) Ampache PSR4-namespace
/ tests / <-- Root root-folder for all test sources
/ vendor / <-- composer vendor dir
README.md
...

The content of the src folder may look as follows:

/ src / Application / <-- Application related code (e.g. the current content of the entryscripts like `artist.php` encapsulated in classes)
/ src / Config / < -- Config/Bootstrap/Setup related code
/ src / Lib / <-- Library code which doesn't fit into a module
/ src / Model / <-- Database model code (currently `lib/class/Metadata/Model`)
/ src / Module / <-- The application logic splitted up in handy modules
/ src / Repository / <-- Database repository code (currently `lib/class/Metadata/Repository`)

The content of the tests-folder will be 1:1 the same as the src-Folder, e.g. a FooBarRepository.php will be located in src/Repository and it's test in tests/Repository - both sharing the same Namespace Ampache\Repository. According to PSR-4 rules, the namespaces of php files within src and tests must match the directory structure.

In addition: This is not pro/con the introduction of frameworks - from my pov it's necessary to establish structures which can be easily migrated to a framework in the future.

[lachlan-00]

(lol @wagnerd open close from a different phone.)

anyway, I like the idea of us adopting a clear structure like this and my personal preference is to keep pushing regular php without frameworks because ampache is really one of the only simple music projects left.

@usox these comments are what we want to see. rules and process are something ampache needs.

again I think we start this in a new branch and @usox I will invite you into the org to help make this change happen. @wagnerd I think your knowledge of moving to laravel will be really important here because of how much you had to do to even get that branch usable.

[wagnered]

Yea I have a phone with a sx inch display and size 10 1/2 hands. The buttons are too close together.☹️.

[lachlan-00]

okay, @usox i've invited you to the Ampache org and if you want to join i'll give you push access to the source-changes branch to allow you to make changes or fixes there.

I read over the php-pds/skeleton standard and i think it's a good change for us to adopt so lets get it moving.

I'll keep the code changes in develop to a minimum outside the API and break/fixes so allow us time to bake this branch as long as possible.

[usox]

Hi @lachlan-00
Thanks, I joined the organization. Please let me know if you have granted me push access to the branch :)

I'll try to keep up with the recent changes in develop and will merge it into source-changes regularly.

[lachlan-00]

I've got source-changes set up as a secondary on for testing. Haven't got past the installer yet but also haven't had time to sit on this branch.

All I have to do is reorganise the teams and apply it to that branch. Give me 2 hours to fix that up after I get to work.

[lachlan-00]

okay @usox i've set your branch to source-changes, you have free reign there without pull requests.

[lachlan-00]

@usox one thing I will do in develop is standardise the require lib/init ,etc calls so when you update src/public it can be a find/replace

[usox]

@lachlan-00 Thanks, I already pushed the first set of changes.
I decided to use /public as the public dir for now. All path changes have already been done - after checking out, updating composer and setting the public path in the webserver as root, the software will already work.

As I stated in your comment in your commit, I would prefer the usage of DIR instead of prefixes. I already changed that in the source-changes branch in all affected files.

[lachlan-00]

using dir is fine with me and i was going to suggest it was done in source-changes anyway.

Happy with all those changes, i just did it that way in develop to make them all the same for find and replace.