-
Notifications
You must be signed in to change notification settings - Fork 75
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): Use injectable operations and remove serviceprovider #1643
feat(jans-client-api): Use injectable operations and remove serviceprovider #1643
Conversation
…ns-and-remove-serviceprovider
[jans-client-api] SonarCloud Quality Gate failed. |
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 keep authorization check in Filter.
…njectable-operations-and-remove-serviceprovider
[jans-pycloudlib] Kudos, SonarCloud Quality Gate passed! |
[notify] Kudos, SonarCloud Quality Gate passed! |
[orm] Kudos, SonarCloud Quality Gate passed! |
[jans-core] Kudos, SonarCloud Quality Gate passed! |
…Filter to BaseOperation to control IP in all cases
…ns-and-remove-serviceprovider
log.info("\n\n\n AuthorizationFilter::filter() - authorizationHeader = " + authorizationHeader + " , authorizationRpIdHeader = " | ||
+ authorizationRpIdHeader + " \n\n\n"); | ||
try { | ||
authorizationService.processAuthorization(info.getPath(), context.getMethod(), request.getRemoteAddr(), authorizationHeader, authorizationRpIdHeader); |
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.
If we have filter which performs authorization why do we still authorize again here io.jans.ca.server.op.BaseOperation#getObjectForJsonConversion
?
if (getCommandType().isAuthorizationRequired()) {
final ApiAppConfiguration conf = jansConfigurationService.find();
String authorization = httpRequest.getHeader("Authorization");
String authorizationRpId = httpRequest.getHeader("AuthorizationRpId");
validateAccessToken(authorization, safeToRpId((HasRpIdParams) params, authorizationRpId), conf);
}
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.
In case AuthorizationRpId
header is null, then it is read from body params:
jans/jans-client-api/server/src/main/java/io/jans/ca/server/op/BaseOperation.java
Line 84 in e44ed18
validateAccessToken(authorization, safeToRpId((HasRpIdParams) params, authorizationRpId), conf); |
I tried to read the body content in filter, but then the body is not available for io.jans.ca.server.op.BaseOperation#process
method.
io.jans.ca.server.filter.AuthorizationFilter
process the token validation only when AuthorizationRpId
header is not null.
One option could be to establish AuthorizationRpId
header as mandatory.
Otherwise, we can look for a way to read body in filter, without losing it.
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.
Right, lets read authorizationRpId
from header and move it out of body. In general everything was in body because oxd was designed as backchannel app, we should re-think it since now it's web app.
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.
Now, authorizationRpId
is mandatory in header.
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.
It is ready for review
} | ||
|
||
@Override | ||
public CommandType getCommandType() { |
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.
Since now it's injectable operation we don't need commandType because we should not have central processing. Isn't it? Remove CommandType
and all info should go to operation (e.g. return type).
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.
OK I will remove.
…ns-and-remove-serviceprovider
…params, now is mandatory in header
…ns-and-remove-serviceprovider
[jans-cli] Kudos, SonarCloud Quality Gate passed! |
[jans-config-api-parent] Kudos, SonarCloud Quality Gate passed! |
[jans-linux-setup] Kudos, SonarCloud Quality Gate passed! |
[Jans authentication server parent] Kudos, SonarCloud Quality Gate passed! |
Prepare
Description
use injectable operations and remove ServiceProvider class
Target issue
#1532
Implementation Details
Refactoring code
Operations are injected directly in request
Test and Document the changes