-
Notifications
You must be signed in to change notification settings - Fork 0
IAM first version #1
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
base: master
Are you sure you want to change the base?
Conversation
1c19b14 to
45d52a2
Compare
Mortega5
left a comment
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.
lgtm
| @@ -0,0 +1,7 @@ | |||
| # did-helper/Chart.yaml | |||
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 think we are at a point where it makes sense to create a dedicated chart for the helper. What do you think?
| ], | ||
| "client": { | ||
|
|
||
| "${CLIENT_DID}": [ |
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 think we should clean this up a little, to only those that are required in that repo.
|
|
||
| - name: iam | ||
| namespace: iam | ||
| chart: ../charts/decentralized-iam |
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.
This would require manual update of the file on each version increase of the chart. Since this file will mostly(always?) be used by maven, a less manual alternative would be replacing a version-variable by resource-filtering
| ingress: | ||
| tir: | ||
| annotations: | ||
| kubernetes.io/ingress.class: "apisix" # Workaround since chart does not set className properly |
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.
Could you create a PR on the chart please?
| fullInclusion: true | ||
|
|
||
| odrl-authorization: | ||
| apisix: |
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.
As far as I seen, no extension of the opa-plugin is loaded. That means no http-body is provided to opa, which breaks a number of policies.
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.
My mistake, thats most likely done by the odrl-authorization chart, right?
| Request request = new Request.Builder() | ||
| .url(issuerHost + CREDENTIAL_OFFER_URI_PATH + "?credential_configuration_id=" + credentialConfigId) | ||
| .get() | ||
| .header("Authorization", "Bearer " + keycloakJwt) |
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.
"Authorization" and "Bearer" are multi times used "magic" constants. Would be better to make real constants of them.
| /** | ||
| * SD-JWT-Credentials {@see https://drafts.oauth.net/oauth-sd-jwt-vc/draft-ietf-oauth-sd-jwt-vc.html} | ||
| */ | ||
| SD_JWT_VC("vc+sd-jwt"); |
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.
Could you extend the enum to also support dc+sd-jwt? vc+sd-jwt is already deprecated and we will most likely need it in the very short future.
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <groupId>org.fiware</groupId> |
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.
Would also better be org.fiware.dataspace
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <groupId>org.fiware</groupId> | ||
| <artifactId>decentralizedIam</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.
should be kebab-case
| <configuration> | ||
| <skip>true</skip> | ||
| <portBindings> | ||
| <!-- NGINX ingress controller http & https --> |
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 I understand correctly, we are now using apisix directly as an ingress controller? Then at least the comment should be updated.
No description provided.