Symfony Take 3! #2097

ryanrath · 2025-10-06T18:56:02Z

Description

Motivation and Context

Tests performed

Checklist:

The pull request description is suitable for a Changelog entry
The milestone is set correctly on the pull request
The appropriate labels have been added to the pull request

jpwhite4 · 2025-10-15T19:44:34Z

config/bundles.php

+    Symfony\Bundle\SecurityBundle\SecurityBundle::class => ['all' => true],
+    Symfony\Bundle\MonologBundle\MonologBundle::class => ['all' => true],
+    Symfony\Bundle\TwigBundle\TwigBundle::class => ['all' => true],
+    Symfony\Bundle\WebProfilerBundle\WebProfilerBundle::class => ['dev' => true, 'test' => true],


Needs to default to production mode

because the CI tests need to check the production code works not the dev code.

jpwhite4 · 2025-10-15T20:25:35Z

.circleci/config.yml

      - store_artifacts:
          path: /tmp/screenshots
      - store_artifacts:
-          path: /var/log/xdmod


This code still outputs data to /var/log/xdmod so we still want to see the files in there on error.

Also the CI checks ignore the contents of /usr/share/xdmod/var/log when checking for PHP errors.

Suggest code change is for symponhy to log to the existing xdmod logs directory and not a brand new one.

jpwhite4 · 2025-10-15T20:58:13Z

src/Errors/ErrorController.php

+            $headers = $exception->getHeaders();
+        }
+
+        $message = $exception->getMessage();


This seems like a very clunky way to implement this logic. Why are we parsing the message text in the catch all handler? Surely we should just ensure that the code that created the "User already exsits" message sends the redirct response.

jpwhite4 · 2025-10-15T20:59:16Z

src/Errors/ErrorController.php

+     */
+    public function __invoke(Throwable $exception): Response
+    {
+        $this->logger->error('Error Controller Erroring!');


This message is not useful in its current form. Suggest you either remove it or change it to log something that we could use to debug the exception.

jpwhite4 · 2025-10-15T21:06:05Z

src/Controller/AuthenticationController.php

+    #[Route('/rest/auth/logout', name: 'xdmod_rest_auth_logout', methods: ['POST'])]
+    public function formLogout(Request $request): Response
+    {
+        $this->logger->error('*** FormLogout ***');


Suggested change

$this->logger->error('*** FormLogout ***');

this is not an error. I assume this is just some debug that needs to be removed.

jpwhite4 · 2025-10-15T21:15:12Z

html/index.php

+# These are only here temporarily,
+# put together which headers / values are set based on config options.
+header('Access-Control-Allow-Origin: *');
+header("Access-Control-Allow-Headers: *");
+header("Access-Control-Allow-Methods: *");


These need to be removed.

jpwhite4 · 2025-10-15T21:22:09Z

src/Controller/HomeController.php

+
+        $features = $this->getFeatures();
+
+        $isSSOConfigured = false;


This logic is different that the original code. We used to do

if ($auth && $auth->isSamlConfigured()) {

to decide whether to enable the sso login. This needs to be changed to work the same way it did before.

jpwhite4 · 2025-10-15T21:23:22Z

src/Controller/HomeController.php

+        $isSSOConfigured = false;
+        $ssoLoginLink = [
+            'organization' => [
+                'en' => 'Test Organization',


This looks like debug code that should not be here.

jpwhite4 · 2025-10-16T12:53:31Z

templates/twig/about/xdmod.html.twig

@@ -0,0 +1,47 @@
+<div>
+    <img src="/gui/images/about/xdmod_main.png"></img>


``

Suggested change

<img src="/gui/images/about/xdmod_main.png"></img>

<img src="/gui/images/xdmod_main.png"></img>

jpwhite4 · 2025-10-16T13:08:06Z

templates/index.html.twig

+        <script type="text/javascript">Ext.onReady(xdmodviewer.init, xdmodviewer);</script>
+    {% endif %}
+    {% if captcha_site_key|length > 0 %}
+        <script src="https://www.google.com/recaptcha/api.js?render=explicit" integrity="KdR3IRU6FaMwxwxpr20d4OAa/SFlwTtroyPQ91TE4WubaFXASqGzq1YKUmToQbOg"></script>


We didn't use to have an integrity parameter here with some sort of base64 encoded parameter - whats up?

jpwhite4 · 2025-10-16T13:11:06Z

templates/index.html.twig

+    {% endif %}
+
+    <script type="text/javascript" src={{ asset('gui/js/PortalModule.js') }}></script>
+    {% if user is not null and user_dashboard %}


This logic is not uptodate - we now check to see if the user is mapped to a real person before showing the dashboard

jpwhite4 · 2025-10-16T13:20:07Z

src/Kernel.php

+
+declare(strict_types=1);
+
+namespace Access;


Why is the namespace called this?

is it some sort of symphony defined name that we have to use?

jpwhite4 · 2025-10-16T13:26:32Z

templates/index.html.twig

+    <link rel="stylesheet" type="text/css" href="{{ asset('gui/css/MetricExplorer.css') }}"/>
+    <link rel="stylesheet" type="text/css" href="{{ asset('gui/css/common.css') }}"/>
+
+    {% if user is null %}


When can user be null? The code as I read it will always create a user object -this will be a "Public User" if noone is logged in or will be the actual user if they are.

Looking at the src/Controller/HomeController.php there is no way that user can be null.

I think that all of the user is null, user is not null conditionals should actually be tests checkingis_public_user and is_logged_in respectivly.

jpwhite4 · 2025-10-16T13:34:03Z

html/index.php

-
-    <script type="text/javascript" src="gui/js/RESTProxy.js"></script>
-    <script type="text/javascript">
-        <?php \xd_rest\printJavascriptVariables(); ?>


If you are removing the call to this function you also need to remove the function definition too.

jpwhite4 · 2025-10-16T13:40:41Z

html/about/roadmap.php

@@ -1,60 +0,0 @@
-<?php


This file still exists but is empty - is this correct?

jpwhite4 · 2025-10-16T17:23:07Z

config/packages/security.yaml

+        # - { path: ^/admin, roles: ROLE_ADMIN }
+        # - { path: ^/profile, roles: ROLE_US1ER }
+
+when@test:


Ii don't think we should have this directive - its not used.

jpwhite4 · 2025-10-16T17:23:48Z

config/packages/doctrine.yaml

@@ -0,0 +1,50 @@
+doctrine:


This file should be deleted as its not used/needed.

jpwhite4 · 2025-10-16T17:24:04Z

config/packages/doctrine_migrations.yaml

@@ -0,0 +1,6 @@
+doctrine_migrations:


This file should be deleted as its not used/neeeded

jpwhite4 · 2025-10-16T17:24:45Z

config/packages/framework.yaml

+    #fragments: true
+    php_errors:
+        log: true
+when@test:


This seems like it is incorrect and should be removed.

jpwhite4 · 2025-10-16T17:25:16Z

config/packages/dev/monolog.yaml

+    handlers:
+        main:
+            type: stream
+            path: "%kernel.logs_dir%/%kernel.environment%.log"


need to change this to log to the existing exceptions.log file.

jpwhite4 · 2025-10-16T17:26:10Z

config/packages/google_recaptcha.yaml

@@ -0,0 +1,21 @@
+services:


Is this used? If not then please remove. If so then we'll need to update the documentation about how to configure it at a site.

jpwhite4 · 2025-10-16T17:26:44Z

config/packages/monolog.yaml

@@ -0,0 +1,61 @@
+monolog:


How come there are two monolog.yaml files? One here and one in dev/

jpwhite4 · 2025-10-16T17:28:36Z

config/services.yaml

@@ -0,0 +1,71 @@
+parameters:
+    sso:
+        login_link: https://xdmod:7000/


This looks like configuration settings that are unique to our test harness. So should not be in the deployed configuration file.

jpwhite4 · 2025-10-16T17:29:22Z

config/services.yaml

+            system_username:
+                attribute: 'system_username'
+                default: '$username'
+    google_recaptcha_site_key: '%env(GOOGLE_RECAPTCHA_SITE_KEY)%'


What is this used for?

jpwhite4 · 2025-10-16T17:30:24Z

config/services.yaml

+                attribute: 'system_username'
+                default: '$username'
+    google_recaptcha_site_key: '%env(GOOGLE_RECAPTCHA_SITE_KEY)%'
+    center_related_acls:


What is this used for?

jpwhite4 · 2025-10-16T17:37:05Z

config/services.yaml

+    Access\:
+        resource: '../src/'
+        exclude:
+            - '../src/DependencyInjection/'


This directory does not exist. So no need to exclude it???

jpwhite4 · 2025-10-16T17:39:59Z

.circleci/config.yml

            parameters:
              os: [rocky8]
-              install-type: ["fresh_install", "upgrade"]
+              install-type: ["fresh_install"]


needs to not be changed.

jpwhite4 · 2025-10-16T17:40:13Z

.env

@@ -0,0 +1,8 @@
+# Default ENV file


This file needs to be removed.

The functionality contained in this file has been moved to HomeController / index.html.twig.

So, these changes should have been included when removing the last couple of files.

So this is kind of a half-step commit, ideally there should be one basic way (process) to get / authenticate a user. To get to this point will require some additional work. As a step in the right direction I've pulled out the `detectUser` function ( and the funcitons it depends on ) from `libraries/security.php` and moved them to `src/BaseController.php` so that authentication related code is in one place ( that place being the "new" Symfony code ) and the Symfony controllers that still used `detectUser` have been updated to use new `BaseController::detectUser` function. A future commit ( or maybe PR ) will contain the migration of this code to be more Symfony specific.

These changes should address the following asana tasks: - https://app.asana.com/1/14740318781074/project/1211850781356424/task/1211881442164997?focus=true - https://app.asana.com/1/14740318781074/project/1211850781356424/task/1211881442164990?focus=true The problems that were encountered were that certain css files were not present when viewing the page before logging in. The ultimate cause of the problem was that the logic used to determine whether or not a user was logged in had changed. Previously it was based on the presence of the `xdUser` session variable, in the new code it was based on if an XDUser was returned from the `getXDUser` function ( i.e. was a null returned ), which was always the case. The fix was to add a new template variable `user_logged_in` and set it based on if there was an `xdUser` session variable ( and if the user is not public, since the public user can't be logged in ) and then use this new boolean variable where we had previously tested for `user is not null`. After the above fix went in there was a problem w/ SSO Login. The observed behevaior was that SSO Login would be successful, but the sign in link would not change to the logged in users name. The fix for this was to make sure that the `xdUser` session variable is set in `BaseController.php::getXDUser`.

This is caused by `$request->get('logLevels');` returning an array as opposed into a string, so when we requested it as a string it freaked out. I've gotten around this by just using request->get('logLevels'); instead of any of the the get<type>param functions.

Or at least, they do not cause issues when running our normal suite of automated tests or when logging in via SSO locally.

We no longer need to pre-auth a user before serving them the information, the authentication / authorization is a function of the backend route and if a user isn't authorized to view the information then the route is responsible for handling that.

There are some warnings / deprecations that end up here during the upgrade of PHP and related librarires that was causing the CI build to fail. Since these messages are not actually related to the running of XDMoD I've chosen to truncate the file.

APP_SECRET population will be moved so that it's handled as part of the General Setup ( option 1 in xdmod-setup ).

These changes update the way that the `.env` file is generated. - `.env` is now excluded from being included when building an rpm or source install. - `.env` is now created using a new template `templates/env.template` at the end of the `1. General Setup` process in `xdmod-setup`. - We also use Symfony to pre-compile the `.env` file so that it's not read on every request. The xdmod-setup-start.tcl file has been updated to take this second file generation into account.

This change allows for the `bin/console` file to be used while installed via RPM or Source *and* while doing development work within the XDMoD git repo.

SRI is subresource integrity and informs the browser to check the requested script resource against the provided cryptographic hash. Here is the page used as a reference for changes: https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/Subresource_Integrity and here is the url where the error can be seen: https://sonarcloud.io/project/security_hotspots?id=ubccr_xdmod&pullRequest=2097&sinceLeakPeriod=true

Fixing issue found here: https://app.circleci.com/pipelines/github/ubccr/xdmod/4752/workflows/29b1a380-8ab1-4cb5-872d-943b90d9ca53/jobs/12063 by replacing `strpos` usage ( which does not allow null haystacks ) to `str_contains` which does.

ryanrath force-pushed the symfony_take3 branch from 94c8b94 to 4b90b5b Compare October 7, 2025 13:51

jpwhite4 reviewed Oct 15, 2025

View reviewed changes