Profile on knowledge base visible on private #4913
Conversation
…e read if explicitly visible
…if the knowledge base is public
WalkthroughThis pull request refines authorization logic across several services and resolvers. It updates method signatures in authorization services by adding new parameters (such as parent credential rules and visibility flags) and introduces conditional checks based on visibility and search criteria. Authorization decorators in resolver methods have been updated (e.g., from READ to READ_ABOUT), and unnecessary service dependencies have been removed. Additionally, new lookup resolver methods are added to handle organization and virtual contributor queries with proper guards and authorization checks, and an admin licensing mutation now references a revised privilege. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KBaseAuthSvc as KnowledgeBaseAuthorizationService
participant AuthPolicySvc as AuthorizationPolicyService
Client->>KBaseAuthSvc: applyAuthorizationPolicy(knowledgeBaseInput, parentAuth, knowledgeBaseVisible)
alt knowledgeBaseVisible is true
KBaseAuthSvc->>AuthPolicySvc: appendCredentialRuleAnonymousRegisteredAccess(authorization, READ, true)
end
KBaseAuthSvc-->>Client: Return updated authorization
sequenceDiagram
participant Client
participant VContributorAuth as VirtualContributorAuthorizationService
participant KBaseAuthSvc as KnowledgeBaseAuthorizationService
Client->>VContributorAuth: applyAuthorizationPolicy(virtualContributorInput, ...)
alt virtual.searchVisibility is not HIDDEN
VContributorAuth-->>Client: Grant access (READ_ABOUT, cascade true)
VContributorAuth->>KBaseAuthSvc: applyAuthorizationPolicy(..., virtual.settings.privacy.knowledgeBaseContentVisible)
else
VContributorAuth-->>Client: Restrict access
end
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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
|
|
@bobbykolev you were right :) The profile on the knowledge base should be visible, it is just the contents of the callouts set should be protected. This PR should adjust the behavior. NOTE: the privilege to check (if any is checked for) on the client is READ_ABOUT for general information about the VC, and READ on the actual KB for if the user will have access to the contents of the CalloutsSet. @SimoneZaza fyi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts (1)
116-129: Add authorization check for organization lookup.The method is missing authorization checks which could expose sensitive organization data. Other lookup methods in this class implement authorization checks.
Add authorization check similar to other methods:
async organization( + @CurrentUser() agentInfo: AgentInfo, @Args('NAMEID', { type: () => NameID }) nameid: string ): Promise<string> { const organization = await this.organizationLookupService.getOrganizationByNameIdOrFail( nameid ); + this.authorizationService.grantAccessOrFail( + agentInfo, + organization.authorization, + AuthorizationPrivilege.READ_ABOUT, + `lookup Organization by NameID: ${organization.id}` + ); return organization.id; }
🧹 Nitpick comments (2)
src/domain/common/knowledge-base/knowledge.base.service.authorization.ts (1)
21-58: LGTM! Visibility-based authorization is properly implemented.The implementation correctly grants READ access to anonymous registered users when the knowledge base is visible. Consider adding JSDoc to document the new parameter.
Add parameter documentation:
public async applyAuthorizationPolicy( knowledgeBaseInput: IKnowledgeBase, parentAuthorization: IAuthorizationPolicy | undefined, + /** Flag indicating if the knowledge base should be visible to all registered users */ knowledgeBaseVisible: boolean ): Promise<IAuthorizationPolicy[]> {src/domain/collaboration/callouts-set/callouts.set.service.authorization.ts (1)
25-56: LGTM! Credential rules inheritance is properly implemented.The implementation correctly handles inheritance of credential rules from the parent authorization policy.
Add parameter documentation:
async applyAuthorizationPolicy( calloutsSetInput: ICalloutsSet, parentAuthorization: IAuthorizationPolicy | undefined, roleSet?: IRoleSet, spaceSettings?: ISpaceSettings, + /** Credential rules to inherit from parent authorization policy */ credentialRulesFromParent: IAuthorizationPolicyRuleCredential[] = [] ): Promise<IAuthorizationPolicy[]> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/domain/collaboration/callouts-set/callouts.set.service.authorization.ts(3 hunks)src/domain/common/knowledge-base/knowledge.base.resolver.fields.ts(2 hunks)src/domain/common/knowledge-base/knowledge.base.service.authorization.ts(3 hunks)src/domain/community/virtual-contributor/virtual.contributor.resolver.fields.ts(7 hunks)src/domain/community/virtual-contributor/virtual.contributor.service.authorization.ts(3 hunks)src/platform/admin/licensing/admin.licensing.resolver.mutations.ts(1 hunks)src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts(2 hunks)src/services/api/lookup/lookup.resolver.fields.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/platform/admin/licensing/admin.licensing.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/community/virtual-contributor/virtual.contributor.service.authorization.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/common/knowledge-base/knowledge.base.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/services/api/lookup/lookup.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/collaboration/callouts-set/callouts.set.service.authorization.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/common/knowledge-base/knowledge.base.service.authorization.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/community/virtual-contributor/virtual.contributor.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (8)
src/domain/common/knowledge-base/knowledge.base.resolver.fields.ts (1)
22-22: LGTM! Authorization privilege change aligns with requirements.The change from READ to READ_ABOUT for the profile field aligns with the PR objective of making knowledge base profiles more accessible.
Let's verify that this is the only occurrence of this authorization change pattern:
✅ Verification successful
Authorization Privilege Update Verified
The change in
src/domain/common/knowledge-base/knowledge.base.resolver.fields.ts
to useAuthorizationPrivilege.READ_ABOUTfor the profile field is correctly applied as intended. Although our search forAuthorizationPrivilege.READreturns many matches, these include instances where the string "READ" appears as a prefix inREAD_ABOUTas well as legitimate cases in other parts of the codebase that are not subject to this change. The profile field update remains isolated and aligned with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential READ to READ_ABOUT changes that might need similar updates rg -A 1 "@AuthorizationAgentPrivilege\(AuthorizationPrivilege\.READ\)" --type tsLength of output: 15662
src/domain/community/virtual-contributor/virtual.contributor.resolver.fields.ts (3)
29-29: LGTM! Dependency removal improves code simplicity.Removing the unused authorizationService dependency improves code maintainability.
57-59: LGTM! Authorization field resolver is simplified.The authorization field resolver is correctly simplified to directly return the parent's authorization.
31-31: Verify the impact of changing multiple READ privileges to READ_ABOUT.The change of multiple fields from READ to READ_ABOUT privilege aligns with the PR objectives but requires careful verification.
Let's verify that all necessary fields are updated consistently:
Also applies to: 51-51, 61-61, 77-77, 90-90, 104-104, 118-118, 129-129
✅ Verification successful
I'll run another script to search for all instances of
READ_ABOUTto verify that updated privileges are used consistently. Let's execute that:
Consistency verified for the privilege changes
All occurrences in the affected directory and file have been updated to useAuthorizationPrivilege.READ_ABOUTwithout any remaining instances of the oldREADprivilege. The update is applied consistently across all targeted fields.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining READ privileges that might need updating rg "AuthorizationPrivilege\.READ\)" --type ts src/domain/community/virtual-contributor/Length of output: 89
Script:
#!/bin/bash # Search for all instances of READ_ABOUT privileges within the virtual-contributor directory rg "AuthorizationPrivilege\.READ_ABOUT" --type ts src/domain/community/virtual-contributor/Length of output: 1392
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts (1)
131-154: LGTM! Authorization check aligns with PR objectives.The implementation correctly uses
AuthorizationPrivilege.READ_ABOUTwhich aligns with the PR objectives of updating authorization logic for knowledge base visibility.src/domain/community/virtual-contributor/virtual.contributor.service.authorization.ts (2)
74-82: LGTM! Well-implemented visibility-based authorization.The implementation correctly:
- Checks searchVisibility before granting access
- Uses READ_ABOUT privilege with cascade
- Aligns with the PR objectives for knowledge base visibility
126-131: LGTM! Proper knowledge base authorization.The implementation correctly passes the knowledge base visibility flag to the authorization policy.
src/services/api/lookup/lookup.resolver.fields.ts (1)
224-229: LGTM! Consistent authorization privilege update.The change from
READtoREAD_ABOUTaligns with the PR objectives and maintains consistency with other virtual contributor related changes.
| this.authorizationService.grantAccessOrFail( | ||
| agentInfo, | ||
| account.authorization, | ||
| AuthorizationPrivilege.TRANSFER_RESOURCE, | ||
| AuthorizationPrivilege.TRANSFER_RESOURCE_OFFER, // TODO: this is wrong, need to create a new privilege | ||
| `create Wingback account for account (${accountID})` | ||
| ); |
There was a problem hiding this comment.
Create a new privilege for Wingback account creation.
The TODO comment indicates that TRANSFER_RESOURCE_OFFER is not the correct privilege. This needs to be addressed properly.
Consider creating a new privilege like CREATE_WINGBACK_ACCOUNT. Would you like me to:
- Create a new enum value in
AuthorizationPrivilege? - Update this code to use the new privilege?
There was a problem hiding this comment.
@coderabbitai raise a story
There was a problem hiding this comment.
🧩 Analysis chain
@valentinyanakiev I've created a new issue to track the implementation of a proper authorization privilege for Wingback account creation. The issue includes the context, requirements, and references to this PR discussion.
(_/)/
(•.•)
/>📝
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for implementing a new authorization privilege
gh issue create \
--title "Implement new authorization privilege for Wingback account creation" \
--body "Currently, the \`createWingbackAccount\` mutation in \`AdminLicensingResolverMutations\` uses \`TRANSFER_RESOURCE_OFFER\` privilege which is not the correct privilege for this operation.
## Background
- The code currently uses \`TRANSFER_RESOURCE_OFFER\` with a TODO comment indicating it's incorrect
- A new privilege specific to Wingback account creation is needed
## Requirements
1. Create a new authorization privilege enum value (e.g., \`CREATE_WINGBACK_ACCOUNT\`)
2. Update the \`createWingbackAccount\` mutation to use the new privilege
3. Update relevant authorization policies and tests
## References
- PR: https://github.com/alkem-io/server/pull/4913
- Comment: https://github.com/alkem-io/server/pull/4913#discussion_r1941759834" \
--assignee valentinyanakiev
Length of output: 913
@valentinyanakiev I've created issue #4918 to track this work: #4918
(_/)
(^.^)
/🎯\
✏️ Learnings added
Learnt from: valentinyanakiev
PR: alkem-io/server#4913
File: src/platform/admin/licensing/admin.licensing.resolver.mutations.ts:0-0
Timestamp: 2025-02-05T11:04:17.764Z
Learning: The `createWingbackAccount` mutation in `AdminLicensingResolverMutations` requires a dedicated authorization privilege instead of using `TRANSFER_RESOURCE_OFFER`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Logic around authorization for VC updated so that if not hidden then all users get READ_ABOUT
Then if the KB is public, then all users get READ
Summary by CodeRabbit
These improvements simplify workflows and enhance security and data visibility for a smoother end-user experience.