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

Jetty 12.0.x Simplify tests with OpenId by using an exising openid provider and avoid extra not necessary maven modules #12274

Merged
merged 12 commits into from
Sep 17, 2024

Conversation

olamy
Copy link
Member

@olamy olamy commented Sep 16, 2024

  • use keycloak as openid provider for testing
  • remove extra modules not needed
  • checkstyle
  • start keycloak only once
  • fix openid module for ee10
  • fix typo

Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

I don't think using keycloak is necessary to merge these modules, as we have an implementation of OpenIdProvider with jetty-core. But I do think its an improvement, now we test with an actual OIDC provider implementation and not just a mockup.

@olamy
Copy link
Member Author

olamy commented Sep 16, 2024

The idea is to remove code we maintain and using proven existing solution.
Why having code to implement an openid provider as some solutions exist already ;)

@olamy
Copy link
Member Author

olamy commented Sep 16, 2024

@lachlan-roberts init of keycloak done via some API usage. remove of this huge json import file.

result of the cleanup

image

//"jetty.server.dumpAfterStart=true",
};

try (JettyHomeTester.Run run2 = distribution.start(args2); WebClient webClient = new WebClient();)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have preferred use of the jetty HttpClient instead of the gargoylesoftware one.

Copy link
Member Author

@olamy olamy Sep 16, 2024

Choose a reason for hiding this comment

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

bear in mind this is using now a real openid provider so there is a redirection to web page for login.
And not sure we really want to spend time parsing the returned HTML to find all the information such: cookie, all ids pass, session etc....
it;s not only an http client but most important a HTML parser and browser like library
it looks the easiest way to mimic a browser.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Jetty client will handle the session cookies, and can follow redirects, and here it is always reading the full content as a string and doing containsString(...) checks on it, so no HTML parsing needed. And then there is a FORM submission which is also easy to do with the Jetty HttpClient.

Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure the rules adding new dependencies, but if you're sure we can add these for testing then LGTM.

Although I still prefer to use Jetty HttpClient for the testing if possible.

@olamy
Copy link
Member Author

olamy commented Sep 16, 2024

I'm not 100% sure the rules adding new dependencies, but if you're sure we can add these for testing then LGTM.

I don't understand the problem. It's just a test module not even deployed/distributed so adding dependency doesn't have any impact.

Although I still prefer to use Jetty HttpClient for the testing if possible.

The problem is to extract data from the returned HTML. It's not like if parsing HTML was like parsing structured/well-formed data :)
We can do it and I will have a try as it's probably not too complicated, but I feel like re-inventing the wheel.

@olamy
Copy link
Member Author

olamy commented Sep 16, 2024

@lachlan-roberts using our own Jetty HttpClient done

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy merged commit e52bd7a into jetty-12.0.x Sep 17, 2024
11 checks passed
@olamy olamy deleted the jetty-12.0.x-openid-test branch September 17, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants