-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat(jans-client-api): migration to Weld/Resteasy and Jetty 11 - Issue 260 #1319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check inlined comments
jans-client-api/server/pom.xml
Outdated
<version>1.4</version> | ||
<groupId>io.jans</groupId> | ||
<artifactId>jans-client-api-common</artifactId> | ||
<version>1.0.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<version>${project.version}</version>
may fit better because we want to rely on current projects module.
jans-client-api/server/pom.xml
Outdated
<groupId>com.google.inject.extensions</groupId> | ||
<artifactId>guice-servlet</artifactId> | ||
<groupId>io.jans</groupId> | ||
<artifactId>jans-orm-spanner</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: since below there is jans-core-service
it already have all jans-orm-* libs as cross-dependencies. I guess you can skip explicit declaration. Same story with resteasy
.
jans-client-api/server/pom.xml
Outdated
<dependency> | ||
<groupId>org.jboss.arquillian.container</groupId> | ||
<artifactId>arquillian-container-test-impl-base</artifactId> | ||
<version>1.4.0.Final</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it jakarta based? I believe it should be 1.7.0.Alpha10. Move arquillian version into property/variable. (Or even use from jans-bom).
@@ -0,0 +1,6 @@ | |||
# LDAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should drop oxd
from file name.
"enableJwksGeneration": true, | ||
"jwksExpirationInHours": 2400, | ||
"jwksRegenerationIntervalInHours": 720, | ||
"cryptProviderKeyStorePath": "F:/ADMIN-UI/jans-client-api1/server/src/main/resources/client-api-server-jwks.keystore", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is wrong here, forgot to drop it after testing?
import jakarta.ws.rs.core.MediaType; | ||
|
||
@Path("/") | ||
public class UMA2ReourceServerResource extends BaseResource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Reource -> Resource
@@ -0,0 +1,24 @@ | |||
package io.jans.ca.server.utils; | |||
|
|||
public enum ErrorResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop or otherwise put actual errors.
@@ -0,0 +1,141 @@ | |||
<ehcache xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="ehcache.xsd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't need ehcache configuration on jans-client-api, do we ?
@@ -1,2886 +0,0 @@ | |||
# raw swagger spec link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is new swagger file located? I somehow can't find it. We should be able to call jans-client-api with swagger and be able see updated docs.
|
||
import static org.testng.AssertJUnit.*; | ||
|
||
public class SpontaneousScopeAuthTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see many tests are removed (like this one), what is the replacement for them? How are we going to track coverage of existing functionality if we are dropping them now?
…e remove site tests working
[JanssenProject_super-jans] Kudos, SonarCloud Quality Gate passed! |
[notify] Kudos, SonarCloud Quality Gate passed! |
…nal client-api url
[jans-pycloudlib] Kudos, SonarCloud Quality Gate passed! |
[SCIM API] Kudos, SonarCloud Quality Gate passed! |
[jans-config-api-parent] Kudos, SonarCloud Quality Gate passed! |
[Fido2 API] Kudos, SonarCloud Quality Gate passed! |
[Jans authentication server parent] Kudos, SonarCloud Quality Gate passed! |
[jans-cli] Kudos, SonarCloud Quality Gate passed! |
[jans-client-api] SonarCloud Quality Gate failed. |
[jans-linux-setup] Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There still good amount of work to be done but I've created separate tickets for it. This PR is big and we should let it go. It makes main thing, migrates jans-client-api to jetty 11. It is good enough.
@JsonProperty(value = "error") | ||
private String error; | ||
@JsonProperty(value = "error_description") | ||
private String errorDescription; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening separate ticket to improve copy/paste of same fields
#1530
<!-- <classes>--> | ||
<!-- <class name="io.swagger.client.api.SetUpTest"/>--> | ||
<!-- </classes>--> | ||
<!-- </test>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding ticket to enable these tests back based on swagger generated client.
logger.info("Read backend properties: {}", backendProperties); | ||
i++; | ||
entryManager = factory.createEntryManager(backendProperties); | ||
logger.info("Trató de leer: {}", entryManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like english, translation is welcome.
private void clearRPTestData() { | ||
try { | ||
String val = System.getProperty("clearTestData"); | ||
if (val != null && !val.isEmpty() && Boolean.valueOf(val).booleanValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean.parseBoolean(val)
rpService.load(); | ||
logger.info("Finished removeExistingRps successfullly."); | ||
} else { | ||
logger.info("Invalid value clearTestData."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not print it because normally operational jans-client-api should not have that property
@Consumes(MediaType.APPLICATION_JSON) | ||
public Response getAccessTokenByRefreshToken(@HeaderParam("Authorization") String authorization, @HeaderParam("AuthorizationRpId") String authorizationRpId, String params) { | ||
logger.info("Api Resource: /get-access-token-by-refresh-token Params: {}", params); | ||
String result = process(CommandType.GET_ACCESS_TOKEN_BY_REFRESH_TOKEN, params, GetAccessTokenByRefreshTokenParams.class, authorization, authorizationRpId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scheduled improvement to has each operation as injectable
#1532
@jmunozherbas please check comments and open new PR(s) for it. |
Migration to Weld/Resteasy and Jetty 11
Target issue:
#260
Test implementation was based o jans-auth-server