-
Notifications
You must be signed in to change notification settings - Fork 61
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(ojoi): Add auth to client endpoint #16090
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on integrating user context into GraphQL resolvers and service methods. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (1)
150-181
: Remove theconsole.log
statement.The
addApplicationAttachment
,getApplicationAttachments
, anddeleteApplicationAttachment
methods are updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API calls. This ensures that the application attachments are managed securely with the provided authentication.However, the
console.log
statement in theaddApplicationAttachment
method should be removed as it is likely a debugging artifact and should not be present in production code.Apply this diff to remove the
console.log
statement:- console.log(auth)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (7 hunks)
- libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/official-journal-of-iceland/official-journal-of-iceland.service.ts (1 hunks)
- libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.config.ts (2 hunks)
- libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.provider.ts (3 hunks)
- libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (7 hunks)
Additional context used
Path-based instructions (6)
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/official-journal-of-iceland/official-journal-of-iceland.service.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.config.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.provider.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (33)
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.config.ts (3)
9-9
: LGTM! Thescope
property is now a top-level property in the schema.Moving the
scope
property to the top level of the schema changes the structure of the configuration object, making it directly accessible as a top-level property. The Zod definition ensures thatscope
is an array of strings, providing type safety and validation.Consider updating any code that accesses the
scope
property to reflect the new schema structure.
22-22
: Verify the impact of the updated default value for thescope
property.The default value for the
scope
property has been changed from an empty array to['api_resource.scope']
. This change signifies a shift in how the application will handle thescope
parameter, potentially impacting the way permissions or resource access are defined within the application.Please ensure that the
api_resource.scope
value aligns with the intended functionality and security requirements of the application. Consider the following:
- Does
api_resource.scope
represent a valid and appropriate scope for the application?- Are there any dependencies or configurations that need to be updated to support this new default scope?
- Have the necessary permissions and access controls been implemented to handle the
api_resource.scope
?
Line range hint
1-28
: The code file adheres to TypeScript usage and configuration reusability.The code file defines a configuration schema using Zod, which promotes type safety and validation. The configuration is defined using the
defineConfig
function from@island.is/nest/config
, indicating a reusable configuration pattern.However, the code file does not contain any components or hooks, so the reusability aspect cannot be fully assessed in this context. Additionally, the code file does not directly involve bundling or tree-shaking practices.
libs/application/template-api-modules/src/lib/modules/templates/official-journal-of-iceland/official-journal-of-iceland.service.ts (1)
37-42
: LGTM!The changes align with the PR objective of implementing authentication by including the
auth
parameter in the call tothis.ojoiApplicationService.postApplication
. The method is correctly propagating theauth
parameter to the service layer for handling authentication.Error handling is also implemented by catching exceptions and returning
false
.libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.provider.ts (3)
9-9
: LGTM!The import statement correctly adds the necessary configurations for integrating an Identity Service (IDS) client and XRoad, which aligns with the PR objective. The code follows the expected pattern and TypeScript usage.
19-33
: Looks good!The changes effectively integrate the Identity Service (IDS) client configuration into the
useFactory
function. The conditional configuration ofautoAuth
based onidsClientConfig.isConfigured
allows flexibility in enabling/disabling the authentication mechanism. The code follows TypeScript best practices for defining the configuration type.
47-47
: Looks good!Adding
IdsClientConfig.KEY
to theinject
array is necessary to make the IDS configuration available for dependency injection. This change ensures that theuseFactory
function can access theidsClientConfig
as expected.libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (8)
39-43
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
49-53
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
59-60
: LGTM!The method correctly passes the id and user context to the service layer. The changes are consistent with the other methods in the file.
66-67
: LGTM!The method correctly passes the id and user context to the service layer. The changes are consistent with the other methods in the file.
76-78
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
87-89
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
98-100
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
109-111
: LGTM!The method correctly passes the input and user context to the service layer. The changes are consistent with the other methods in the file.
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (9)
19-19
: LGTM!The import statement for the
User
type is correct and follows the proper syntax.
31-32
: LGTM!The
getComments
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.getComments
method. This allows for user-specific functionality or authorization checks.
35-44
: LGTM!The
postComment
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.postComment
method. This allows for user-specific functionality or authorization checks.
51-57
: LGTM!The
getPdfUrl
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.getPdfUrl
method. This allows for user-specific functionality or authorization checks.
60-64
: LGTM!The
postApplication
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.postApplication
method. This allows for user-specific functionality or authorization checks.
67-73
: LGTM!The
getPrice
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.getPrice
method. This allows for user-specific functionality or authorization checks.
78-92
: LGTM!The
getPresignedUrl
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.getPresignedUrl
method. This allows for user-specific functionality or authorization checks.
97-116
: LGTM!The
addApplicationAttachment
method is correctly updated to accept auser: User
parameter, which is then passed to theojoiApplicationService.addApplicationAttachment
method. This allows for user-specific functionality or authorization checks.
133-157
: LGTM!The
getApplicationAttachments
anddeleteApplicationAttachment
methods are correctly updated to accept auser: User
parameter, which is then passed to the respective methods of theojoiApplicationService
. This allows for user-specific functionality or authorization checks.libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (9)
21-21
: LGTM!The import statement for
Auth
andAuthMiddleware
from@island.is/auth-nest-tools
is correct and necessary for the authentication changes in this service.
33-35
: LGTM!The new private method
ojoiApplicationApiWithAuth
is implemented correctly. It encapsulates the logic of applying theAuthMiddleware
to the API, which can be reused across other methods in the service. This is a good practice for code reusability and maintainability.
39-42
: LGTM!The
getComments
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the comments are retrieved securely with the provided authentication.
55-57
: LGTM!The
postComment
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the comments are posted securely with the provided authentication.
69-74
: LGTM!The
postApplication
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the application is posted securely with the provided authentication.
88-93
: LGTM!The
getPdfUrl
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the PDF URL is retrieved securely with the provided authentication.
95-101
: LGTM!The
getPdf
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the PDF is retrieved securely with the provided authentication.
124-129
: LGTM!The
getPrice
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the price is retrieved securely with the provided authentication.
143-145
: LGTM!The
getPresignedUrl
method is updated correctly to accept theauth
parameter and use the newojoiApplicationApiWithAuth
method for the authenticated API call. This ensures that the presigned URL is retrieved securely with the provided authentication.
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.
Mostly cool, one comment
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16090 +/- ##
==========================================
- Coverage 36.65% 36.65% -0.01%
==========================================
Files 6749 6748 -1
Lines 138766 138774 +8
Branches 39422 39423 +1
==========================================
- Hits 50863 50862 -1
- Misses 87903 87912 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
What
Adding auth to include bearer token to third party services
Why
To allow third party services to extract information from the bearer token
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation