From c7a6bdf4d3d3599dd51712d38c40a90dc3b696ad Mon Sep 17 00:00:00 2001 From: hitesh balyan Date: Mon, 30 Dec 2024 23:46:01 +0530 Subject: [PATCH] Enable permissions and enhance permission handling in the backend (#219) ## Description Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. Fixes # (issue) ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Intermediate change (work in progress) ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration - [ ] Test A - [ ] Test B ## Checklist: - [ ] Performed a self-review of my own code - [ ] npm test passes on your machine - [ ] New tests added or existing tests modified to cover all changes - [ ] Code conforms with the style guide - [ ] API Documentation in code was updated - [ ] Any dependent changes have been merged and published in downstream modules --- app-config.yaml | 2 +- packages/backend/src/plugins/permission.ts | 129 ++++++++++++++------- 2 files changed, 85 insertions(+), 46 deletions(-) diff --git a/app-config.yaml b/app-config.yaml index 56efeeb..8a24ad4 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -314,4 +314,4 @@ catalog: ## setting this to `false` will disable permissions permission: - enabled: false + enabled: true diff --git a/packages/backend/src/plugins/permission.ts b/packages/backend/src/plugins/permission.ts index 7755374..22e43d0 100644 --- a/packages/backend/src/plugins/permission.ts +++ b/packages/backend/src/plugins/permission.ts @@ -1,6 +1,6 @@ import { AuthorizeResult, - isPermission, + isResourcePermission, type PolicyDecision, } from '@backstage/plugin-permission-common'; import { @@ -8,7 +8,14 @@ import { type PolicyQuery, } from '@backstage/plugin-permission-node'; import { createCatalogConditionalDecision } from '@backstage/plugin-catalog-backend/alpha'; -import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common/alpha'; +import { + catalogEntityCreatePermission, + catalogEntityDeletePermission, + catalogEntityReadPermission, + catalogEntityRefreshPermission, + catalogEntityValidatePermission, + RESOURCE_TYPE_CATALOG_ENTITY, +} from '@backstage/plugin-catalog-common/alpha'; import type { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; import type { PaginatingEndpoints } from '@octokit/plugin-paginate-rest'; import { parseEntityRef } from '@backstage/catalog-model'; @@ -30,7 +37,11 @@ class RequestPermissionPolicy implements PermissionPolicy { PaginatingEndpoints['GET /orgs/{org}/repos']['response']['data'] >; readonly catalogRepos: Promise>; - readonly userRepoPermissions: Record> = {}; + readonly userRepoPermissions: Record< + string, + //type "(string & {})" used to provide auto complate in the typescript + Record //NOSONAR + > = {}; constructor( protected readonly tokenManager: AuthService, @@ -48,6 +59,10 @@ class RequestPermissionPolicy implements PermissionPolicy { request: PolicyQuery, user?: BackstageIdentityResponse, ): Promise { + this.logger.debug('Permission request received', { + requestedPermission: JSON.stringify(request), + }); + if (!user?.identity) { this.logger.error('not able to found the name', { user: JSON.stringify(user), @@ -156,37 +171,28 @@ class RequestPermissionPolicy implements PermissionPolicy { return 'null'; } - protected async resolveAuthorizedRepoList( - userEntityRef: string, - ): Promise { + protected async resolveAuthorizedRepoList(userEntityRef: string) { const usernameEntity = parseEntityRef(userEntityRef); + const startTimeBenchmark = performance.now(); - if (userEntityRef in this.userRepoPermissions) { + if (userEntityRef in this.userRepoPermissions) return this.userRepoPermissions[userEntityRef]; - } - this.userRepoPermissions[userEntityRef] = []; + //! There can be case where user which is not valid get multiple time requests + this.userRepoPermissions[userEntityRef] = {}; const catalogRepos = await this.catalogRepos; const orgRepos = await this.orgRepositories; // Filtering out repo which is logged in the Catalog meta with just name of the repo const repositories = orgRepos.filter(r => catalogRepos.has(r.name)); - const privateCatalogRepos = repositories.filter(r => r.private); - const publicCatalogRepos = repositories.filter(r => r.private === false); - - for (const repo of publicCatalogRepos) { - this.userRepoPermissions[userEntityRef].push(repo.name); - } - const userRole = await this.fetchUserRole(userEntityRef); if (['member', 'admin', 'maintainer'].includes(userRole)) { - for (const repos of privateCatalogRepos) { - this.userRepoPermissions[userEntityRef].push(repos.name); + for (const repo of repositories) { + this.userRepoPermissions[userEntityRef][repo.name] = 'write'; } } else { - // will fetch individual repo permissions - for (const repos of _.chunk(privateCatalogRepos, 10)) { - const permissions = await Promise.all( + for (const repos of _.chunk(repositories, 10)) { + await Promise.all( repos.map(repo => this.octokit.rest.repos .getCollaboratorPermissionLevel({ @@ -194,10 +200,19 @@ class RequestPermissionPolicy implements PermissionPolicy { repo: repo.name, username: usernameEntity.name, }) - .then(resp => ({ - repo, - ...resp.data, - })) + .then(permission => { + this.userRepoPermissions[userEntityRef][repo.name] = + permission.data.permission; + }) + .catch(e => { + //! Handling the issue of the missing repo if there is any issue + this.logger.error("Issue while fetching user's permission", { + error: e, + owner: String(process.env.GITHUB_ORGANIZATION), + repo: repo.name, + userEntityRef, + }); + }) .finally(() => { this.logger.info( '***GithubRequest GET /repos/{owner}/{repo}/collaborators/{username}/permission', @@ -209,14 +224,12 @@ class RequestPermissionPolicy implements PermissionPolicy { }), ), ); - - for (const permission of permissions) { - this.userRepoPermissions[userEntityRef].push(permission.repo.name); - } } } - //! log any meta name which is not include that means there is issue in the meta name + this.logger.debug('Permission resolution benchmark', { + totalTimeInMilliSeconds: startTimeBenchmark - performance.now(), + }); return this.userRepoPermissions[userEntityRef]; } @@ -225,36 +238,62 @@ class RequestPermissionPolicy implements PermissionPolicy { request: PolicyQuery, user: BackstageIdentityResponse, ): Promise { - if (!isPermission(request.permission, catalogEntityReadPermission)) { + const currentOperation = { + [catalogEntityReadPermission.name]: ['admin', 'write', 'read'], + [catalogEntityCreatePermission.name]: ['admin', 'write'], + [catalogEntityDeletePermission.name]: ['admin', 'write'], + [catalogEntityRefreshPermission.name]: ['admin', 'write'], + [catalogEntityValidatePermission.name]: ['admin', 'write'], + }[request.permission.name]; + + if (currentOperation === undefined) { + this.logger.info('Non catalog permission type request received', { + catalogEntityReadPermission: JSON.stringify( + catalogEntityReadPermission, + ), + requestedPermission: JSON.stringify(request.permission), + }); return; } - const startTimeBenchmark = performance.now(); - const userPermission = await this.resolveAuthorizedRepoList( + + if ( + !isResourcePermission(request.permission, RESOURCE_TYPE_CATALOG_ENTITY) + ) { + this.logger.info( + 'Basic type permission will pass it as user have access atleast one repo', + { + user: user.identity.userEntityRef, + }, + ); + return { result: AuthorizeResult.ALLOW }; + } + + const userRepoPermission = await this.resolveAuthorizedRepoList( user.identity.userEntityRef, ); - - if (!userPermission?.length) { + if (_.size(userRepoPermission) === 0) { // permission not resolved from the Github API this.logger.info( "Not able to fetch user Permission or Github didn't have repos for the user", { user: user.identity.userEntityRef, - resolvedPermission: JSON.stringify(userPermission), + resolvedPermission: JSON.stringify(userRepoPermission), }, ); return { result: AuthorizeResult.DENY }; } - this.logger.debug('Permission resolution benchmark', { - totalTimeInMilliSeconds: startTimeBenchmark - performance.now(), - }); - this.logger.debug('Permission resolution benchmark', { - totalTimeInMilliSeconds: startTimeBenchmark - performance.now(), - }); - const condition = createCatalogConditionalDecision( + + const repos = _.map( + _.pickBy(userRepoPermission, permission => + currentOperation.includes(permission), + ), + (_, repo) => repo, + ); + + return createCatalogConditionalDecision( request.permission, - repositoryAccessCondition({ repos: userPermission }), + repositoryAccessCondition({ repos }), ); - return condition; } }