The Wayback Machine - https://web.archive.org/web/20201124234444/https://github.com/ltb-project/self-service-password/pull/172
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

Opening branch 2.0 by a complete refactor #172

Open
wants to merge 46 commits into
base: 2.0-proposal
from

Conversation

@plewin
Copy link
Member

@plewin plewin commented Dec 17, 2017

Major changes

  • Adoption of composer
  • Everything was refactored, it includes a new code structure.
  • Adoption of Symfony framework 2.8. but using Symfony 4.0 conventions and frontend. Still use Symfony 2.8 version to keep compatibility with php 5.4. Upgrade to symfony 3.4 (min php PHP 5.5.9) or 4.x (7.1.3) can be decided later.
  • Adoption of Symfony standards
  • Frontend uses now webpack + npm for js package management. It is completely overkill for now because we have only 15 lines of jquery code but it will be justified soo as I expect to do multiple PR to enhance end users experience, with as you type policy checks and in browser password generation.
  • Testing is now done by codeception. See travis of this PR. There is a lot of tests now and the code coverage today is 53% of effective lines. There are 4 suites of tests: Acceptance (Written, they contain scenarios tested in fake browser), Functional (test couples things in the app, still need to add more), Unit (unit tests, almost complete), Integration (TODO test our ldap client vs ldap servers).
  • Phpmailer was dropped for Swiftmailer. I could not manage to automate tests with phpmailer so I switched to swiftmailer which is better integrated with symfony.
  • New command line utility "php bin/console" from Symfony, we can build command line tools with the code of ssp
  • Logging is now done with Monolog
  • Templates were refactored and are using Twig
  • The docroot of the application is not at the root of the file structure but in the public folder (this will require configuration on the http server)
  • URL were changed to be self explanatory (index.php?action=sendtoken -> /reset-password-by-email) (this will require configuration on the http server)
  • The previous configuration file is no more supported. Configuration is done in multiple files and in yaml (in the /config directory), most of the parameters have been renamed

Things still missing

  • Update package scripts
  • Rewrite a script for requirements checks
  • Fix some translations
  • Automate testing LDAP functions vs LDAP servers (there have a working poc of openldap on travis but there are still things to do)
  • Add more documentation in the code to help people who want to extend SSP
  • Fancy new badges in the README.md
@plewin plewin added the enhancement label Dec 17, 2017
@plewin plewin self-assigned this Dec 17, 2017
@bilbo-the-hobbit
Copy link

@bilbo-the-hobbit bilbo-the-hobbit commented Dec 18, 2017

Hello,

i don't understand why all those changes are needed for a simple self service password application. Bringing on board symphony + npm for js package management

For me all those change will mean i will stop recommanding it, npm is a show stopper for me, its not maintenable from a sysadmin point of view and is bloated.

Cheers

@coudot
Copy link
Member

@coudot coudot commented Dec 18, 2017

Hello @bilbo-the-hobbit, we discussed wiht @plewin some days ago, and he is building a 2.0 branch to show how create a more testable and standard PHP application. As far as I understand, npm will not be needed to run the application, but only to develop it.

As always, your remarks are important and we can discuss here of what we want for the future of this tool. I'm also learning PHP modern development and I'm curious to see what it can bring to SSP.

@Jitsusama
Copy link
Contributor

@Jitsusama Jitsusama commented Dec 20, 2017

It looks pretty exciting to me. One worry I have is that with the YAML configuration files, are we still going to be able to pull in environment variables to set values? I currently import a custom PHP configuration script that uses function calls to pull in environment variables for deploying the application via Docker. I would hate to lose that functionality.

@plewin
Copy link
Member Author

@plewin plewin commented Dec 21, 2017

Hi,

As for npm, future users of ssp should not even know about it unless they want to develop or install it from source instead of packages.
There should be no major differences in installations between 1.1 and this 2.0 proposal for sysadmins.
But there will be still some :
rpm/deb packages in 1.1 weighs 1.5 Mio compressed, 2.0 packages should be around 9 Mio compressed, 30 Mio uncompressed
The configuration file is different and users will have to reconfigure if they are upgrading.
The js in the package will be minified and because there is much more php code (in /vendor) to review if security need to review ssp code before adoption.
There is a new var directory holding cache files for symfony and logs (by default, logs will go here instead of the php log).

I do not know what is the future of SSP. Maybe it has already achieved feature perfection and no new features should be accepted. We already have self service ssh public key, maybe the tool is already doing to much for some ? I guess SSP will continue to accept new changes and if there are new changes, we need to secure our features by tests and reorganise the code to stop duplications.

I wanted to use Twig, I did not want to use symfony in SSP at first. I am using symfony in SSP now mostly because it improves testability, removes the need of a homemade framework, and gives us conventions. It is not necessary but I believe it is a good move for SSP.
Silex was considered but it is going end of life in the middle of 2018 because of symfony 4. symfony 4 got so decoupled that it made Silex not worth using other symfony.

I am not confident working with the branch master. It was easier, at least for me, to refactor and write tests for a 2.0 than testing 1.1 directly.

About Docker, I would like docker to have first class support in SSP 2.0 but I would not look into it before I am confident of the rpm/deb packaging of 2.0 and that the ldapclient is tested in travis with openldap, possibly apache directory server too.

About environment variables to set values in a dockerized SSP config, it is not possible right now. It is supported in symfony >= 3.2 but not in symfony 2 (not including 2 specials one APP_ENV, APP_DEBUG). And this branch use symfony 2.8 to keep php 5.4 compatibility until decided otherwise. As php 5.5 already reached end of security support since a year and a half I think it is okay to require users to use 5.6 minimum and to not allow users to use php versions considered unsecure. but I would wait to have the first rpm/deb of this branch to see how is the installation experience. I guess there is a high chance that I propose a requirement of php 5.6 and a upgrade symfony 2.8 -> 3.4.
If there is an upgrade and a support of env variables, the parameters can look like this :

parameters:
    ldap_url: '%env(LDAP_URL)%'
    env(LDAP_URL): ldap://localhost
    ldap_binddn: '%env(LDAP_BINDDN)%'
    env(LDAP_BINDDN): "cn=manager,dc=example,dc=com"

Meaning parameters can be set in the config file and overriden by environment variables.
I would consider this with caution :
There cannot be any logic between the environnement variable and the config file (like if there is X env var and also Y env var set Z var to this, or any king of env variable processing)
There is so much options in SSP that it is going to be overwhelming. I would prefer users to mount a config directory.

On a side note there will surely be 2 environnement variables anyway APP_ENV & APP_DEBUG (that's how symfony works).
APP_ENV choose which config file get used in the config directory, it will default in ssp to prod or maybe i will propose demo instead so unconfigured dockers will display a demo of ssp with a virtual ldap.
APP_DEBUG enable debugging (which is different from logging)

I plan to maintain this 2.0 branch until it is ready and is accepted for merge into master (or not accepted). I will be making PRs like this one instead of pushing directly commits to explain changes being made and to discuss about them. I will merge them myself in the 2.0 to allow new PRs to come but we can still debate on them.

@bilbo-the-hobbit
Copy link

@bilbo-the-hobbit bilbo-the-hobbit commented Dec 24, 2017

hello,

i don't understand this phrase

As for npm, future users of ssp should not even know about it unless they want to develop or install it from source instead of packages.

does this mean we will have dependancy on npm from the package system dependancy point of view ?? Seems rather strange that source install is not equal to packages install.

i agreed to PHP 5.6 at least but symfony only 2.8 is supported on debian strech

https://packages.debian.org/search?suite=stretch&section=all&arch=any&searchon=names&keywords=symfony

so for the time being a migration is not possible if we want proper support on debian stretch. Framework could be great but it means lots of install from sources and not up-to-date packaging in the distributions.

i also regret that the testing of the code is not done on 1.1 who is the mainstream version everybody use, i know testing can be hard as i fight with my dev for that :)

as a result i have mixed feelings for all of this

Cheers

@plewin plewin changed the base branch from 2.0 to 2.0-proposal Jan 4, 2018
@coudot coudot added this to the 2.0 milestone Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.