Update the latest from ryanrath:symfony_migration into ubccr:symfony_migration#2230
Merged
Merged
Conversation
Routes with `{prefix}/` were broken in some cases due to `/` also being
matched by `.*`. Replacing `{prefix}/` with just `{prefix}` means these
routes function as intended.
This is a route that's used by selecting the "Download" button from the "My Reports" section of the Report Generator.
This else-case was missed in initial migration.
So what the old `services.yaml#sso::login_link` property really was meant to be was the referrer value that is included when the Identity Provider makes a call to XDMoD's callback url so that we can specifically identify the "entry point" of SSO authentication as opposed to attempting to authenticate each and every request.
These changes clarify what's actually being done to configure SSO for XDMoD and allow our current setup to work w/ how we actually tell people to setup SimpleSAMLPhp in production as opposed to how we do it for our CI Pipeline. An update to our CI SSO Setup will be forthcoming.
So not only was ErrorController essentially swallowing exceptions, it didn't actually do what we needed it to ( catch thrown exceptions and return 200OK w/ a json body ). To actually implement the handling of Exceptions I've added a RestExceptionListener that hooks into Symfony Kernel's life-cycle by listening for kernel.exception events, which occur when an exception is thrown while processing a request. Manual testing was done by manually throwing a `BadRequestHttpException` in UserInterfaceController.php `index` function. The expected outcome is that an error message is presented to the user in a js modal window.
It looks like ErrorController really is needed, after removing it and adding RestExceptionListener roughly 40-50 integration tests failed due to http status code differences. It looks like identifying the endpoints that provide error messages to a user need to be identified and specifically handled as opposed to a blanket approach.
These changes are in reference to: https://github.com/orgs/ubccr/projects/5/views/1?pane=issue&itemId=142956534&issue=ubccr%7Cxdmod-xsede%7C1048 which replaces usage of the PHP Super Global `$_REQUEST` w/ Symfony's Request.
This change had been removed in a commit previously, but must have been lost somewhere along the way.
These changes came about when investigating what was causing symfony-dev to "hang" with no error thrown. The root cause of the problem was a new group by that had been added to xdmod-dev / mysql-dev, but was not present in symfony-dev's config files. But, it did surface a bug in the Symfony implementation for the old controller endpoints. Specfically how they were ( or were not ) dealing with exceptions. By default Symfony will return a non-200 HTTP Status Code when an exception is thrown. This is all well and good for "modern" front end's, but ExtJS 3.4 was written in a by gone era where apparently web servers were supposed to return 200 OK regardless of exceptions. These changes make it so that we explicitely wrap these old controller routes in try-catch blocks and put a good face on things so that ExtJS can merrily get on with life.
In this commit I've updated the internal dashboard login page so that it is visually consistent with our current internal dashboard login page and fixed the InternalDashboard logout functionality to work as it does now. Manual testing was done to confirm these changes work as intended.
This commit reduces duplicate code by refactoring the user password reset emailing to a helper service, which is then used by both the resetting of a password in internal_dashboard and a regular user requesting a password reset. I also fixed problems w/ the `mode` and the image path that it's based on.
This change fixes a discrepancy found on symfony-dev that is documented here: ubccr/xdmod-xsede#1243 Basically PHP 8.2 is a lot more strict about types and didn't like that we were passing `null` as an argument that was defined as `string` not `?string`. Now that `$requestedFormat` accepts `null` values it all works as expected.
The problem being fixed is that url param `realms` as defined in the route for `WarehouseController.php::searchJobsByAction` contains either `jobs` or `cloud`, which is fine when those are the only two realms being requested. But when you request for the SUPREMM realm the `realms` parameter is still `jobs` and the `realm` query param is `SUPREMM`. So it is not valid to use the variable taht contains `realms` for the `realm`. Yay for being ridiculously confusing!. This fixes this behavior by always using the `realm` query param.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is just updating ubccr's symfony_migration branch from my personal branch where I've been doing all the dev work.
Motivation and Context
Tests performed
Checklist: