Turn SAML year into a path parameter (LG-7554)#7153
Conversation
[skip changelog]
| get "/api/saml/metadata#{suffix}" => 'saml_idp#metadata', format: false | ||
| match "/api/saml/logout#{suffix}" => 'saml_idp#logout', via: %i[get post delete] | ||
| match "/api/saml/remotelogout#{suffix}" => 'saml_idp#remotelogout', via: %i[get post] | ||
| constraints(path_year: SamlEndpoint.suffixes) do |
There was a problem hiding this comment.
This is a pretty cool feature I wasn't aware of, but I'm at a loss for how it even works. I don't see any documentation for it, and the closest I got to thinking I understood it was thinking it was implicitly calling Array#matches?, but matches? doesn't appear to be a method on an array.
There was a problem hiding this comment.
In the Rails Routing guide, I pieced this together from segment constraints and the constraints block form in
So the block form is just a way to avoid repeating the segment constraints for each line
There was a problem hiding this comment.
and in terms of implementation, I bet that it's using our favorite === which for strings and regexes is matches, but for arrays is "contains"
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Conflicts: app/controllers/saml_completion_controller.rb spec/controllers/saml_completion_controller_spec.rb spec/services/saml_endpoint_spec.rb spec/support/saml_auth_helper.rb
|
|
|
Excellent! Suggestion: Can we add a test to the SAML request spec file to check for a 404 being rendered when an unsupported path year is passed for a POST request? And maybe the same kind of test for a SAML GET auth request, to see that the routing constraint works as expected? |
julialeague
left a comment
julialeague
left a comment
There was a problem hiding this comment.
👍 outside of my suggestion!
|
| prepend_before_action :skip_session_expiration, only: [:metadata, :remotelogout] | ||
|
|
||
| skip_before_action :verify_authenticity_token | ||
| before_action :require_path_year |
There was a problem hiding this comment.
Is this necessary? Given the defined constraints, is it possible that a path would be matched without a path year?
If it is necessary, do we have any spec coverage for this?
There was a problem hiding this comment.
I forget
I think that it's basically only required in specs, because in controller tests you can route directly to an action and skip the routers constraints, but in real life the router handles them.
I'll toy around with removing it and see what happens
There was a problem hiding this comment.
Confirmed, it was required because the controller specs let us route directly.
| query_params = Rack::Utils.parse_nested_query url.query | ||
| url = URI.parse(request.original_url) | ||
|
|
||
| # we need to grab just the .path, in case the full value includes query params like ?locale= |
There was a problem hiding this comment.
Do we have spec coverage for this?
There was a problem hiding this comment.
Yeah, the reason I added it was that feature specs in other locales ran into exceptions here
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
* Turn SAML year into a path parameter (LG-7554) * Clean up SamlIdpController to use parsed param instead of parsing URL directly * Add before filter to catch missing path year * Move path_year to a constant, reference it * Fix generated url that includes query params * Review feedback: update saml_requests_spec.rb to check for out-of-bounds years * Review feedback: add specs for get request too * Remove unused method * Bring back stricter route check * Fix usage of domain/host with url helper methods * Fix url helper method call * Simplify auth helper methods changelog: Internal, SAML, Update SAML API URLs to make annual certificate rotation easier --------- Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
4 participants
🎫 Ticket
LG-7554
🛠 Summary of changes
By using Rails router's
constraintsand path params syntax, we can simplify parsing of URLs and also simplify the defintion of our routes📜 Testing Plan
Specs pass