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

[ELY-2362] Add support for the bearer-only option when using the OIDC HTTP mechanism #1735

Merged
merged 8 commits into from Jul 27, 2022

Conversation

fjuma
Copy link
Contributor

@fjuma fjuma commented Jul 25, 2022

https://issues.redhat.com/browse/ELY-2362

This PR adds support for the bearer-only option when using OIDC. This includes the following:

  • The ability to pass the bearer token using the Authorization header
  • The ability to pass the bearer token using an access_token query parameter
  • The ability to specify credentials via HTTP BASIC auth that will be used to obtain the bearer token

Note that this matches the functionality that was present when using the OIDC mechanism provided by the Keycloak OIDC adapter.

log.debugv("User ''{0}'' invoking ''{1}'' on client ''{2}''", principal.getName(), facade.getRequest().getURI(), deployment.getResourceName());
}

protected boolean isAutodetectedBearerOnly(OidcHttpFacade.Request request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@fjuma Just a minor, the request is unused. The below calls to facade.getRequest() can be substituted for the request passed to the method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I've updated the method to remove the parameter since it's not needed.

throw log.noMessageEntity();
}
InputStream is = entity.getContent();
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor, this could be chaged to try with resources:

try (InputStream is = entity.getContent()) {
tokenResponse = JsonSerialization.readValue(is, AccessAndIDTokenResponse.class);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

public static final String ERROR = "error";
public static final String ERROR_DESCRIPTION = "error_description";
public static final String INVALID_TOKEN = "invalid_token";
public static final String STALE_TOKEN = "Stale token";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the specs / prior implementation that gives this one a different format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the format used by the prior adapter. STALE_TOKEN is different because it's a description of the error. As an example,
this would look something like:

"error=invalid_token,error_description=Stale token"

/**
* Bearer token pattern.
*/
public static final Pattern BEARER_TOKEN_PATTERN = Pattern.compile("^Bearer *([^ ]+) *$", Pattern.CASE_INSENSITIVE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is only a fairly simple pattern but could be useful to add to the comment the intent for the pattern so anyone debugging issues later can cross check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added.

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

A couple of minor comments, nothing to block merging.

A few new classes were added which are public, do these all need loading from classes outside the package?

@fjuma
Copy link
Contributor Author

fjuma commented Jul 27, 2022

A couple of minor comments, nothing to block merging.

A few new classes were added which are public, do these all need loading from classes outside the package?

This is private API but none of these classes are needed outside the package anyway so have updated them so they are no longer public. Thanks!

@darranl
Copy link
Contributor

darranl commented Jul 27, 2022

@fjuma thanks for the update, sounds like this can merge once CI completes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants