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

Remove SAML IDP feature #2638

Merged
merged 21 commits into from
Jan 24, 2024
Merged

Remove SAML IDP feature #2638

merged 21 commits into from
Jan 24, 2024

Conversation

hsinn0
Copy link
Contributor

@hsinn0 hsinn0 commented Dec 6, 2023

  • In preparation for the SAML library replacement

Approve or disapprove but do not merge to develop yet. We want merge this to develop now. As this is a breaking change, we have to decide which UAA version this change should go with. we also want to bump UAA major version after the merge.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186632650

The labels on this github issue will be updated when the story is started.

@hsinn0 hsinn0 requested a review from strehle December 6, 2023 22:40
@hsinn0 hsinn0 marked this pull request as draft December 6, 2023 23:17
@Tallicia Tallicia added in-flight in progress DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels Dec 8, 2023
@hsinn0 hsinn0 force-pushed the saml-lib-replacement-182118433 branch from a7f4f5a to dc428b5 Compare December 8, 2023 22:05
@swalchemist swalchemist force-pushed the saml-lib-replacement-182118433 branch 2 times, most recently from 804dd95 to aa60f2c Compare December 26, 2023 18:09
@hsinn0 hsinn0 force-pushed the saml-lib-replacement-182118433 branch 2 times, most recently from 07271e7 to 8507289 Compare January 4, 2024 18:31
@peterhaochen47 peterhaochen47 force-pushed the saml-lib-replacement-182118433 branch 3 times, most recently from 9b047f7 to 07c38ee Compare January 10, 2024 00:18
swalchemist and others added 14 commits January 10, 2024 14:02
* Comment out / delete code to get the project to work
* status: all unit tests passing (./gradlew test), local server starts (./gradlew run) successfully with the UAA login page working in browser

[#182118433]

Co-authored-by: Alicia Yingling <alicia.yingling@broadcom.com>
Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
Co-authored-by: Danny Faught <danny.faught@broadcom.com>
Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
- Changed the doc test to use mocked-up data instead of making actual calls to the end-point that does not exist anymore.

[#182118433]

Co-authored-by: Alicia Yingling <alicia.yingling@broadcom.com>
- The unit test was testing the idp endpoint which is removed, so the test is now removed.

[#182118433]

Co-authored-by: Hongchol Sinn <hongchol.sinn@broadcom.com>
- Disabled/removed test cases that depend on the removed SAML IDP feature.

[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
- Replaced the use of the removed SAML IDP endpoint call with mockup
  data.
- Also, some clean-ups and refactoring

[#182118433]

Co-authored-by: Hongchol Sinn <hongchol.sinn@broadcom.com>
Co-authored-by: Danny Faught <danny.faught@broadcom.com>
Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
Co-authored-by: Alicia Yingling <alicia.yingling@broadcom.com>
- The integration test was getting the IDP metadata from actual UAA server, which needs be bypassed as UAA does not have the SAML IDP endpoint anymore.
- With that, the existing integration test basically becomes the same as `TokenEndpointDocs.getTokenUsingSaml2BearerGrant()`.
- So removed the existing integration test cases and created a new mock mvc test based on the `TokenEndpointDocs` code.

[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
- These inputs to the parameterized tests are testing "/saml/idp/metadata" endpoint
which has been removed

[#182118433]

Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- the logic being tested in these two tests are
already covered by Saml2BearerGrantMockMvcTests
- since UAA-as-a-SAML-IDP feature has been removed, the
difference between these two tests (same zone vs two zones) do not
matter so they can both be covered by Saml2BearerGrantMockMvcTests

[#182118433]

Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- this 3 lines have no bearing on these 2 tests

Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- to clarify things
- and correct wrong comments

[#182118433]

Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
hsinn0 and others added 2 commits January 10, 2024 14:41
[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
- Moved a bean definition from `saml-idp.xml`, which is being deleted, to `saml-providers.xml`.

[#182118433]

Co-authored-by: Bruce Ricard <bruce.ricard@broadcom.com>
@hsinn0 hsinn0 force-pushed the saml-lib-replacement-182118433 branch from 2c629c5 to a9b7820 Compare January 10, 2024 22:54
@hsinn0 hsinn0 removed the DO NOT MERGE Internal Test or WIP, please DO NOT MERGE label Jan 10, 2024
@hsinn0 hsinn0 marked this pull request as ready for review January 10, 2024 22:56
@Tallicia Tallicia added DO NOT MERGE Internal Test or WIP, please DO NOT MERGE in progress labels Jan 11, 2024
@hsinn0 hsinn0 removed in progress DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels Jan 11, 2024
@strehle strehle requested review from a team January 12, 2024 06:17
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

Check and change Readme notice but the rest LGTM

@strehle
Copy link
Member

strehle commented Jan 18, 2024

As mentioned in slack , please wait until release 76.31.0, then merge this PR and increase major version, thanks

@hsinn0 hsinn0 requested a review from strehle January 18, 2024 19:44
README.md Outdated Show resolved Hide resolved
- Remove the added section about the removed feature as that should be just mentioned in release notes.

[#182118433]
@hsinn0
Copy link
Contributor Author

hsinn0 commented Jan 18, 2024

Check and change Readme notice but the rest LGTM

Done

@hsinn0 hsinn0 merged commit da75057 into develop Jan 24, 2024
20 checks passed
@hsinn0 hsinn0 deleted the saml-lib-replacement-182118433 branch January 24, 2024 22:42
peterhaochen47 added a commit that referenced this pull request Feb 1, 2024
- This table was added for the UAA-as-SAML-IDP feature
  (b93c87a)
- This feature has been removed:
  #2638. Hence this table is now
  unused.
- The "DROP TABLE IF EXISTS" syntax would not error out if the table
  does not exist, compared to just "DROP TABLE".
- Also clean up docs and a test util that reference this table.

[#182118433]
peterhaochen47 added a commit that referenced this pull request Feb 6, 2024
- This table was added for the UAA-as-SAML-IDP feature
  (b93c87a)
- This feature has been removed:
  #2638. Hence this table is now
  unused.
- The "DROP TABLE IF EXISTS" syntax would not error out if the table
  does not exist, compared to just "DROP TABLE".
- Also clean up docs and a test util that reference this table.

[#182118433]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants