Skip to content

KNOX-3097 Add more redirect.whitelist Test Cases for KNOXSSO#991

Merged
lmccay merged 4 commits intoapache:masterfrom
lmccay:KNOX-3097
Feb 18, 2025
Merged

KNOX-3097 Add more redirect.whitelist Test Cases for KNOXSSO#991
lmccay merged 4 commits intoapache:masterfrom
lmccay:KNOX-3097

Conversation

@lmccay
Copy link
Contributor

@lmccay lmccay commented Feb 17, 2025

What changes were proposed in this pull request?

Additional unit tests for KNOXSSO and redirect whitelist handling.
These tests are currently in the WebSSOResourceTest class for some reason.
We should move them to a new RegexUtilsTest at some point.

How was this patch tested?

With the new and existing tests.

@lmccay lmccay closed this Feb 17, 2025
@lmccay
Copy link
Contributor Author

lmccay commented Feb 17, 2025

Inadvertently included other changes.

@lmccay lmccay reopened this Feb 17, 2025
@smolnar82
Copy link
Contributor

My two cents:

  • the description suggest tests in the KNOXSSOUT service but those tests are added in the test cases for KNOXSSO (WebSSOutResourceTest vs WebSSOResourceTest)
  • these tests - within WebSSOResourceTest - are actually testing RegexUtils. I wonder if it was better to have those test cases in a new RegExUtilsTest class 💡

@smolnar82 smolnar82 self-requested a review February 18, 2025 12:08
@moresandeep
Copy link
Contributor

My two cents:

  • the description suggest tests in the KNOXSSOUT service but those tests are added in the test cases for KNOXSSO (WebSSOutResourceTest vs WebSSOResourceTest)
  • these tests - within WebSSOResourceTest - are actually testing RegexUtils. I wonder if it was better to have those test cases in a new RegExUtilsTest class 💡

I think so too, i don't see a test for RegexUtils class, so perhaps create a new one and add these tests to it?

@lmccay
Copy link
Contributor Author

lmccay commented Feb 18, 2025

Ahh, very true on the WEBSSOUT point. I'll see if I can change that.
I just tracked down where the RegexUtils tests were already and added on there.
We can move them all in a follow on patch, I think.

@lmccay lmccay changed the title KNOX-3097 Add more redirect.whitelist Test Cases for KNOXSSOUT KNOX-3097 Add more redirect.whitelist Test Cases for KNOXSSO Feb 18, 2025
@lmccay lmccay merged commit 1788cf9 into apache:master Feb 18, 2025
2 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.

4 participants