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

Temporal: Certification changes #1786

Closed
wants to merge 1 commit into from
Closed

Conversation

Milton-Ch
Copy link
Contributor

These are the changes you made during Gluu 4.5 certification

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@@ -289,6 +290,11 @@ private Response requestAuthorization(
isPromptFromJwt = true;
}

if (jwtRequest.getResponseMode() != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -208,7 +208,7 @@ private Response registerClientImpl(String requestParams, HttpServletRequest htt
log.debug("The Initiate Login Uri is invalid. The initiate_login_uri must use the https schema: " + r.getInitiateLoginUri());
throw errorResponseFactory.createWebApplicationException(
Response.Status.BAD_REQUEST,
RegisterErrorResponseType.INVALID_CLAIMS_REDIRECT_URI,
RegisterErrorResponseType.INVALID_CLIENT_METADATA,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -306,6 +307,10 @@ public boolean validateRedirectUris(List<GrantType> grantTypes, List<ResponseTyp
} catch (Exception e) {
log.debug(e.getMessage(), e);
valid = false;
} finally {
if (!valid) {
noRedirectUriInSectorIdentifierUri = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we can directly throw exception here and remove noRedirectUriInSectorIdentifierUri variable.

throw errorResponseFactory.createWebApplicationException(Response.Status.BAD_REQUEST, RegisterErrorResponseType.INVALID_CLIENT_METADATA, "Failed to validate redirect uris. No redirect_uri in sector_identifier_uri content.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved the changes from Jans to oxAuth.
This is the Jans reference issue: JanssenProject/jans#3639.

Now that we can improve it like this would be ideal.
But we should test the certification again for that case.
If the certification comes out successful, we can take it to Jans as well.

@@ -143,7 +143,7 @@ public Response requestRegister(String requestParams, HttpServletRequest httpReq
}

private Response registerClientImpl(String requestParams, HttpServletRequest httpRequest, SecurityContext securityContext) {
Response.ResponseBuilder builder = Response.ok();
Response.ResponseBuilder builder = Response.status(201);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need backward compatibility support.

Response.ResponseBuilder builder = Response.status(CREATED);
if (appConfiguration.getReturn200OnClientRegistration()) {
    builder = Response.ok();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is excellent, I can work on it.

|| (responseTypes.contains(ResponseType.ID_TOKEN) && responseTypes.size() == 1)
|| (responseTypes.contains(ResponseType.ID_TOKEN) && responseTypes.contains(ResponseType.TOKEN))
|| (responseTypes.contains(ResponseType.TOKEN) && responseTypes.size() == 1))) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should retire this validateParams, add new validateResponseType and throw error directly

public void validateResponseType(List<ResponseType> responseTypes, String nonce) {
     if (!existsNonce && ((responseTypes.contains(ResponseType.CODE) && responseTypes.contains(ResponseType.ID_TOKEN))
                || (responseTypes.contains(ResponseType.ID_TOKEN) && responseTypes.size() == 1)
                || (responseTypes.contains(ResponseType.ID_TOKEN) && responseTypes.contains(ResponseType.TOKEN))
                || (responseTypes.contains(ResponseType.TOKEN) && responseTypes.size() == 1))) {
        throw new WebApplicationException(...);
}

@Milton-Ch
Copy link
Contributor Author

This PR has already been separated into multiple PRs for review by parts, I will proceed to close.

@Milton-Ch Milton-Ch closed this Mar 13, 2023
@Milton-Ch Milton-Ch deleted the temp/certification branch March 13, 2023 01:23
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