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

feat(amphora-service)!: add opa client and service #74

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sbckr
Copy link
Member

@sbckr sbckr commented Nov 14, 2024

No description provided.

@sbckr sbckr changed the title Authorization feat(amphora-service)!: add opa client and service Nov 14, 2024
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 94.95798% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (4a77d30) to head (d7b55c2).

Files with missing lines Patch % Lines
...a/service/persistence/metadata/StorageService.java 94.95% 5 Missing and 1 partial ⚠️
...io/carbynestack/amphora/service/opa/JwtReader.java 88.00% 2 Missing and 1 partial ⚠️
...a/service/rest/RestControllerExceptionHandler.java 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #74      +/-   ##
============================================
+ Coverage     90.04%   90.07%   +0.02%     
- Complexity      348      397      +49     
============================================
  Files            49       59      +10     
  Lines          1326     1471     +145     
  Branches         70       78       +8     
============================================
+ Hits           1194     1325     +131     
- Misses           80       93      +13     
- Partials         52       53       +1     
Flag Coverage Δ
service 93.05% <94.95%> (-0.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...k/amphora/service/calculation/SecretShareUtil.java 96.87% <ø> (ø)
...carbynestack/amphora/service/config/OpaConfig.java 100.00% <100.00%> (ø)
...ack/amphora/service/exceptions/CsOpaException.java 100.00% <100.00%> (ø)
...hora/service/exceptions/UnauthorizedException.java 100.00% <100.00%> (ø)
...io/carbynestack/amphora/service/opa/OpaClient.java 100.00% <100.00%> (ø)
...ynestack/amphora/service/opa/OpaClientRequest.java 100.00% <100.00%> (ø)
...o/carbynestack/amphora/service/opa/OpaRequest.java 100.00% <100.00%> (ø)
...rbynestack/amphora/service/opa/OpaRequestBody.java 100.00% <100.00%> (ø)
...io/carbynestack/amphora/service/opa/OpaResult.java 100.00% <100.00%> (ø)
...o/carbynestack/amphora/service/opa/OpaService.java 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a77d30...d7b55c2. Read the comment docs.

---- 🚨 Try these New Features:

@sbckr sbckr marked this pull request as ready for review November 18, 2024 14:54
@sbckr sbckr requested a review from a team as a code owner November 18, 2024 14:54
@sbckr sbckr requested a review from strieflin November 18, 2024 14:54
Copy link
Member Author

@sbckr sbckr left a comment

Choose a reason for hiding this comment

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

see comments

Copy link
Member Author

Choose a reason for hiding this comment

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

undo changes

Copy link
Member Author

Choose a reason for hiding this comment

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

check empty line at end of file

Copy link
Member Author

Choose a reason for hiding this comment

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

and others

public static final String SECRET_WITH_ID_EXISTS_EXCEPTION_MSG =
"A secret with the given id already exists.";
"A secret with the given id already exists.";
Copy link
Member Author

Choose a reason for hiding this comment

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

verify pre-commit formatting

Copy link
Member Author

Choose a reason for hiding this comment

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

undo

Copy link
Member

@strieflin strieflin left a comment

Choose a reason for hiding this comment

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

Found only a few smaller things.


opa:
defaultPolicyPackage: "carbynestack.def"
endpoint: "http://opa.default.svc.cluster.local:8081/"
Copy link
Member

Choose a reason for hiding this comment

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

Missing new line at EOF.

OpaResult.class)
.isAllowed();
} catch (CsHttpClientException e) {
log.error("Error occurred while evaluating OPA policy package: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Logging the full exception and not only the message might provide more helpful debugging information in case something goes wrong.

@@ -13,14 +13,17 @@ spring.datasource.username=${POSTGRESQL_USERNAME}
spring.datasource.password=${POSTGRESQL_PASSWORD}
spring.jpa.database=postgresql
spring.jpa.database-platform=org.hibernate.dialect.PostgreSQLDialect
spring.jpa.hibernate.connection.autocommit=true
spring.jpa.hi.hibernate.connection.autocommit=true
Copy link
Member

Choose a reason for hiding this comment

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

is this by purpose?

}

@Test
void givenNoTokenProvided_whenExtractSubject_thenThrowUnauthorizedException() throws UnauthorizedException {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't his be givenInvalidToken or something alike?

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