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

FI-3508 Update custom redirect path to include correct suite ID #14

Merged
merged 12 commits into from
Dec 5, 2024

Conversation

alisawallace
Copy link
Collaborator

@alisawallace alisawallace commented Nov 26, 2024

Summary

This PR fixes a bug in which the redirect URI paths for the authorization code tests were incorrectly configured and did not include the correct suite id (udap_security vs the incorrect udap_security_test_kit). This caused authorization code tests with redirects to fail against reference implementations, showing a 404 Not Found error for the URL. I also extracted the URI into a single constant referenced by multiple files, so its value is not duplicated anywhere.

In addition, I made the following small updates:

  • Add a check for a valid HTTP URI for the authorization endpoint to the beginning of the redirect test
  • Add missing unit tests for the redirect test, adapted from those in the SMART App Launch test kit
  • Updated required inferno core version to 0.5.1 to utilize latest unit test improvements, and made small refactors to spec tests as a result

Testing Guidance

Unit tests pass.

This was only one of several bugs discovered and fixed on a debugging branch using a reference preset, so tests will still not pass against a reference implementation. However, you can see the changes made in that branch, where all tests are passing, as a cross-reference to the ones included here.

@@ -0,0 +1,25 @@
require_relative 'spec_helper'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this file or to manually include the helpers in the spec file if you update to the latest inferno_core. You can add :request to a describe block to include the official version of this.

RSpec.describe SomeClass, :request do should automatically include them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I updated inferno core to 0.5.1 in the gemspec file, removed the request_helper.rb file, and modified the spec test accordingly.

Copy link
Contributor

@Jammjammjamm Jammjammjamm Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I still see the request_helper file here.

@@ -0,0 +1,25 @@
require_relative 'spec_helper'
Copy link
Contributor

@Jammjammjamm Jammjammjamm Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I still see the request_helper file here.

@@ -0,0 +1,89 @@
require_relative '../../lib/udap_security_test_kit/authorization_code_redirect_test'

RSpec.describe UDAPSecurityTestKit::AuthorizationCodeRedirectTest, :redirect do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be :request, not :redirect, then you won't need to include the rack test stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, misunderstood that. I re-deleted the request_helper.rb file, updated :redirect to :request, and removed the references to include Rack::Test::Methods, but I'm still missing something because the unit tests are failing now with the following error:

Failure/Error: get "/custom/udap_security/redirect?state=#{state}"
NoMethodError:
       undefined method `get' for #<RSpec::ExampleGroups::UDAPSecurityTestKitAuthorizationCodeRedirectTest

What else do I need to do here? I pushed the most recent updates with failing unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I made that change and now almost all the tests (116 out of 131) fail with the error:

Failure/Error: DatabaseCleaner.cleaning { example.run }
       
       StandardError:
         No suite id defined. Add `let(:suite_id) { 'your_suite_id' }` to the spec

Just want to confirm that I do in fact need to add this to all the rspec tests now before I do it - or is there a different underlying problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, sorry, yes, that too. Basically, this new file defines context which will be included in spec that describes a runnable:

https://github.com/inferno-framework/inferno-core/blob/main/spec/runnable_context.rb

So any lets present in that file, as well as the run method, no longer need to be defined in the individual specs, but it all depends on the suite id being defined in order for everything to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, added suite_id to all the necessary spec files and all unit tests are passing - hopefully we're good to go now.

@alisawallace alisawallace merged commit a992f65 into main Dec 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants