Rejuvinate the Stepup Middleware #416

MKodde · 2024-01-17T10:41:26Z

The aim of this PR is to make the Stepup-Middleware project compatible with PHP 8.2 and Symfony 6.4.

This is not simply achieved by installing those requirements and call it a day. Many of the project dependencies need to be made compatible with the new standards Symfony adheres to. And that can quickly become a slippery slope.

In this PR so far I mainly focussed on improving the code quality to modern standards. For example by implementing PHP Stan's rules. Almost the entire app has been made type safe and PHP 8 compatible.

Some major steps to take are:

Ensure the entire app functions in the new Symfony 6.4 app
1. Merge config shattered throughout the app into the /config folder
2. Remove the config/packages/[dev/prod/...] folders and merge them into the main package config
3. Remove the SensioLabs framework extra bundle constructs (mainly router annotations and stuff like that)
Doctrine 3 adjustments:
1. The migrations can be moved to the /migrations folder imho, and be excluded from qa tests?
2. Test and run migrations
3. Ensure that the database unit tests run in the new Doctrine setup
Broadway
1. Test out, and compare outcome of an event replay

For more details on things to do, this private Google Drive document shows more information and an estimation on what's left to do.

They are monitored using Dependabot

The latest and greatest dependencies are installed.

The composer packages have been upgraded. Some code is now broken as proven by tests. This commit aims to eat the low hanging fruits, and get the tests as green as possible. The harder nuts will be fixed in the following commits

In v6, we set the rootnode name in the constructor of the tree builder. And then retrieve the rootnode via a getter

For now they: - Implement the find method (add return types) - Use the bridge ManagerRegistry

1. Added return type definitions 2. When possible; add parameter type definitions 3. Use imports, remove FQN

Simplify certain code constructs like: - Simplify return statement - Make 'is empty' statements more specific - Simplify regex statements

As performed by Rectors SYMFONY_CODE_QUALITY set list

And clean up the code accordingly

Many repos referred to a moved ManagerRegistry class. That was changed in this commit

Type definitions have been added where possible

This is done to prevent failing replays

Fix bugs after manual testing

MKodde

Awesome work @pablothedude. This was quite an undertaking!

I have some questions, submitted as comments in this review thread, but also have a couple of peer review questions I'd like to look at with you IRL.

config/packages/framework.yaml

ci/qa/phpunit.xml

src/Surfnet/Stepup/Identity/Value/NameId.php

src/Surfnet/Migrations/Version20141031144721.php

src/Surfnet/Stepup/DateTime/DateTime.php

bin/doctrine-migrations-migrate.sh

src/Surfnet/StepupMiddleware/ApiBundle/Doctrine/Type/SelfVetOptionType.php

src/Surfnet/StepupMiddleware/GatewayBundle/Entity/InstitutionConfiguration.php

src/Surfnet/StepupMiddleware/ApiBundle/Identity/Projector/SecondFactorProjector.php

src/Surfnet/StepupMiddleware/ApiBundle/Controller/RecoveryTokenController.php

MKodde

Approved ✔️

johanib

I'm about halfway through.
Submitting some minor points. Some might already be fixed, feel free to resolve.

johanib · 2024-12-11T10:43:53Z

ci/qa/phpunit

@@ -5,9 +5,9 @@ set -e

 cd $(dirname $0)/../../

-./ci/qa/create-test-db
+#./ci/qa/create-test-db


can this be removed? or is it needed in some cases. If so, when?

johanib · 2024-12-11T10:47:27Z

composer.json

-        "doctrine/doctrine-migrations-bundle": "^1.2",
-        "doctrine/orm": "^2.5",
+        "ext-openssl": "*",
+        "ext-pdo": "*",


are these exts new deps? Or just made explicit?

johanib · 2024-12-11T10:55:41Z

config/packages/doctrine.yaml

@@ -13,9 +12,6 @@ doctrine:
                password: "%database_middleware_password%"
                server_version: "%database_server_version%"
                charset: utf8
-                options:
-                    # The 1002 constant sets the PDO MYSQL_ATTR_INIT_COMMAND for when the connection is (re)connected.
-                    1002: "%database_driver_options_1002%"


This is not needed anymore? Or only for dev?

johanib · 2024-12-11T13:54:38Z

config/packages/validator.yaml

+when@test:
+    framework:
+        validation:
+            not_compromised_password: false


We don't do anything with passwords right?

🤔 should it be enabled / disabled for prod mode?

johanib · 2024-12-11T14:02:08Z

config/routes/controllers.yaml

+    resource: "@SurfnetStepupMiddlewareManagementBundle/Resources/config/routing.yml"
+    prefix:   /management
+
+openconext_monitor:


I see this is still the old bundle. Not a problem for now, but adding the config just in case.

Suggested change

openconext_monitor:

open_conext_monitor:

resource: "@OpenConextMonitorBundle/src/Controller"

type: attribute

prefix: /

https://github.com/OpenConext/Monitor-bundle/releases/tag/4.3.0

johanib · 2024-12-11T14:26:16Z

src/Surfnet/Migrations/Version20241128131107.php

+/**
+ * Auto-generated Migration: Please modify to your needs!
+ */
+final class Version20241128131107 extends AbstractMigration


Where do these changes come from?

johanib · 2024-12-12T13:29:02Z

src/Surfnet/Stepup/Configuration/Event/ConfigurationEvent.php

    {
-        if ($id !== Configuration::CONFIGURATION_ID) {


Correct that this is removed?

johanib · 2024-12-12T13:49:26Z

src/Surfnet/Stepup/Identity/Entity/SecondFactorCollection.php

+        if ($items !== [] && array_pop($items) instanceof VettedSecondFactor) {
+            return array_reduce(
+                $this->toArray(),
+                fn(VettedSecondFactor $carry, VettedSecondFactor $item): VettedSecondFactor => $service->hasEqualOrHigherLoaComparedTo(


Suggested change

fn(VettedSecondFactor $carry, VettedSecondFactor $item): VettedSecondFactor => $service->hasEqualOrHigherLoaComparedTo(

static fn(VettedSecondFactor $carry, VettedSecondFactor $item): VettedSecondFactor => $service->hasEqualOrHigherLoaComparedTo(

johanib · 2024-12-12T13:56:48Z

src/Surfnet/Stepup/Identity/Value/UnhashedSecret.php

        );
        return new HashedSecret($hashedSecret);
    }

-    public function __construct(string $secret)
+    public function __construct(private readonly string $secret)


Suggested change

public function __construct(private readonly string $secret)

public function __construct(#[\SensitiveParameter] private readonly string $secret)

MKodde marked this pull request as draft January 17, 2024 10:41

MKodde force-pushed the feature/rejuvination branch from 1ec4235 to 78ee79c Compare January 17, 2024 10:52

Base automatically changed from feature/build-and-publish-test-container to main February 20, 2024 15:35

MKodde force-pushed the feature/rejuvination branch 4 times, most recently from ea8aa70 to 4e910cc Compare February 28, 2024 07:54

quartje and others added 23 commits September 16, 2024 13:36

Smoketest env: Set database name to be used in tests

593e7d3

Update test-integration.yml

bc0c1d5

Remove security checks from qa checks

2a45242

They are monitored using Dependabot

Reenable phpcpd

20c1515

Upgrade composer dependencies

4f96b1a

The latest and greatest dependencies are installed.

Repair unit tests after SF6 upgrade

8c92a6e

The composer packages have been upgraded. Some code is now broken as proven by tests. This commit aims to eat the low hanging fruits, and get the tests as green as possible. The harder nuts will be fixed in the following commits

Refrain from using deprecated config options

7b23511

Upgrade Treebuilder creation

ebee370

In v6, we set the rootnode name in the constructor of the tree builder. And then retrieve the rootnode via a getter

Update *Repository classes

9f1c36c

For now they: - Implement the find method (add return types) - Use the bridge ManagerRegistry

Update *Controller, extend AbstractController

836d10c

Update Session framework settings

749685f

Convert Annotations to Attributes

5a64ad2

Improve type safety of Middleware

7a2bc4c

1. Added return type definitions 2. When possible; add parameter type definitions 3. Use imports, remove FQN

Improve code quality

c256522

Simplify certain code constructs like: - Simplify return statement - Make 'is empty' statements more specific - Simplify regex statements

Apply Symfony 6 code changes

aec34cc

As performed by Rectors SYMFONY_CODE_QUALITY set list

Implement PHP 8 conde constructs

ce33d58

And clean up the code accordingly

Repair the ManagerRegistry reference

c3aa947

Many repos referred to a moved ManagerRegistry class. That was changed in this commit

Enable autoc onfigure/wire/public:false defaults

30b5c70

Add doctrine/annotations for jms translations

2094154

Change renamed contracts

849b832

Upgrade Doctrine Types

8554c4e

Type definitions have been added where possible

Adhere to the vetting type loa determination

552da21

Set $possessedSelfAssertedToken with correct type

b8873c7

pablothedude added 3 commits November 20, 2024 15:46

Fix the entitymanager state while replaying

641281f

Fix route controllers

d4b8d6c

Fix mail date format

563c627

pablothedude force-pushed the feature/rejuvination branch from 3175335 to 5e544dc Compare November 20, 2024 14:46

pablothedude added 2 commits November 21, 2024 11:04

Fix deprecations

6954f7d

Refactor remaining Symfony config to when@env

b5071f9

pablothedude force-pushed the feature/rejuvination branch from 5e544dc to b5071f9 Compare November 21, 2024 10:04

pablothedude added 6 commits November 21, 2024 11:20

Fix translations and extract script

ec393ad

Update Doctrine migration scripts

8a0f7c0

Fix deprecated container injection in migrations

f5d08f7

Fix replay errors test2 replay

29bc0bb

Lower dev log level

911f385

Fix production database diff

9d834af

pablothedude force-pushed the feature/rejuvination branch from 9a06585 to 9d834af Compare November 28, 2024 14:02

thijskh and others added 6 commits December 6, 2024 10:12

Bump saml2, xmlseclibs

d10ad3c

Add BC localizeddate twig filter

f0a1b14

Fix typo in doc block

83ef80e

Support deprecated u2f support in projection

a274cc5

This is done to prevent failing replays

Fixes after manual testing

756032a

Fix bugs after manual testing

Bump test image

894632a

pablothedude force-pushed the feature/rejuvination branch from d4cd580 to 894632a Compare December 10, 2024 16:46

pablothedude requested a review from johanib December 11, 2024 08:55

pablothedude assigned MKodde Dec 11, 2024

MKodde commented Dec 11, 2024

View reviewed changes

pablothedude added 4 commits December 11, 2024 16:24

Remove php from bash scripts

9ad9861

Split nameid VO exception

50a1111

Change the mail date format back so timezone is used

c97543d

Remove pagerfanta orm adapter as separate dependency

28d61a2

MKodde commented Dec 12, 2024

View reviewed changes

johanib reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rejuvinate the Stepup Middleware #416

Rejuvinate the Stepup Middleware #416

MKodde commented Jan 17, 2024 •

edited

Loading

MKodde left a comment

MKodde left a comment

johanib left a comment

johanib Dec 11, 2024

johanib Dec 11, 2024

johanib Dec 11, 2024

johanib Dec 11, 2024

johanib Dec 11, 2024

johanib Dec 11, 2024

johanib Dec 12, 2024

johanib Dec 12, 2024

johanib Dec 12, 2024

-openconext_monitor:
+open_conext_monitor:
+  resource: "@OpenConextMonitorBundle/src/Controller"
+  type: attribute
+  prefix: /

	fn(VettedSecondFactor $carry, VettedSecondFactor $item): VettedSecondFactor => $service->hasEqualOrHigherLoaComparedTo(
	static fn(VettedSecondFactor $carry, VettedSecondFactor $item): VettedSecondFactor => $service->hasEqualOrHigherLoaComparedTo(

	public function __construct(private readonly string $secret)
	public function __construct(#[\SensitiveParameter] private readonly string $secret)

Rejuvinate the Stepup Middleware #416

Are you sure you want to change the base?

Rejuvinate the Stepup Middleware #416

Conversation

MKodde commented Jan 17, 2024 • edited Loading

MKodde left a comment

Choose a reason for hiding this comment

MKodde left a comment

Choose a reason for hiding this comment

johanib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MKodde commented Jan 17, 2024 •

edited

Loading