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

Refactor authority check functions in API #180

Closed
rajdip-b opened this issue Apr 8, 2024 · 6 comments · Fixed by #189
Closed

Refactor authority check functions in API #180

rajdip-b opened this issue Apr 8, 2024 · 6 comments · Fixed by #189
Assignees
Labels
good first issue Good for newcomers scope: api Everything related to the API type: enhancement New feature or request

Comments

@rajdip-b
Copy link
Member

rajdip-b commented Apr 8, 2024

Description

We use a set of functions to determine whether a user has authority over a certain entity or not. Currently, we are having a few difficulties in extending the functionality of the function which is bad DevEx. Here are the issues:

  • The naming convention of the functions fit a very specific purpose
  • The parameter type needs to be updated.

Solution

Firstly, we would like to change the names of the functions. Here's what the new names will look like:

  • getWorkspaceWithAuthoritycheckAuthorityOverWorkspace
  • getProjectWithAuthoritycheckAuthorityOverProject
  • getEnvironmentWithAuthoritycheckAuthorityOverEnvironment
  • getVariableWithAuthoritycheckAuthorityOverEnvironment
  • getSecretWithAuthoritycheckAuthorityOverSecret
    Note that the return type shouldn't change

Second, we would want to change the entity parameter that is passed down to the functions. Currently, we can only pass the id. For example, workspaceId, projectId, etc. We would want to refactor it to have a structure like this:

export default async function checkAuthorityOverWorkspace(
 userId: User['id'],
 workspace: {
   id?: Workspace['id']
   name?: Workspace['name']
 },
 authority: Authority,
 prisma: PrismaClient
): Promise<Workspace> {}

This will allow us to pass varying parameters to fetch the data. Note that, the implementation will also change. In the try block of every function, you will need to check the key that has been passed, and based upon that you will need to fetch the resource. For id, you will use findUnique and for name, you will use findFirst.

It goes without saying that the dependent code will also change, meaning you will need to update the existing code that uses these functions.

Additional context

The files can be found under the apps/api/src/common directory. These are the files:

  • get-environment-with-authority.ts
  • get-project-with-authority.ts
  • get-secret-with-authority.ts
  • get-variable-with-authority.ts
  • get-workspace-with-authority.ts
@rajdip-b rajdip-b added type: enhancement New feature or request good first issue Good for newcomers scope: api Everything related to the API labels Apr 8, 2024
@rajdip-b rajdip-b moved this to Todo in keyshade-api Apr 8, 2024
@szabodaniel995
Copy link
Contributor

Hi,
Wanted to grab this issue, but I am not able to set up the project based on the provided docs.

1 - Trying to run the application

[17:10:45] Starting compilation in watch mode...
api:dev:
api:dev: [17:10:58] Found 0 errors. Watching for file changes.
api:dev:
api:dev: Debugger attached.
api:dev: secp256k1 unavailable, reverting to browser version
api:dev: [WARN] 2024-04-19 17:11:06 - Missing one or more Sentry environment variables. Skipping initialization...
api:dev: [INFO] 2024-04-19 17:11:06 - Starting Nest application...
api:dev: [ERROR] 2024-04-19 17:11:06 - Redis credentials are not set. Stopping the application.
api:dev: Waiting for the debugger to disconnect...

-Sentry env variables are said to be unnecessary
-There was no mention at all about Redis credentials (the code seems to try to read it from the .env file)

2 - Trying to run the Unit Tests

warn Versions of [email protected] and @prisma/[email protected] don't match.
This might lead to unexpected behavior.
Please make sure they have the same version.
Waiting for the debugger to disconnect...
Waiting for the debugger to disconnect...
Waiting for the debugger to disconnect...

-tried installing both versions using pnpm, keeps throwing the same error

3 - Trying to run the Integration Tests

docker compose up -d

docker : The term 'docker' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if
a path was included, verify that the path is correct and try again.
At line:1 char:1

  • docker compose up -d
  •   + CategoryInfo          : ObjectNotFound: (docker:String) [], CommandNotFoundException
      + FullyQualifiedErrorId : CommandNotFoundException
    
    
    

-Installed docker as per instructions, no further necessity was mentinoed

@rajdip-b
Copy link
Member Author

Hey @szabodaniel995, thanks for trying to attempt this! I will try to address your issues based on the info you shared.

  1. You are correct. We absolutely missed this part. I have opened up Update docs for Redis #183 to address this issue. All you will need to do is add REDIS_URL=redis://localhost:6379 to your .env file
  2. This is a valid issue aswell. I overlooked it till date because it didn't cause me any issues. But still, I opened up another issue to get this fixed: Fix prisma-cli and prisma client version mismatch #184
  3. This looks like an issue on your end. I feel that your HOME variable isn't configured properly to include docker. And hence, you are facing this error.

I hope this helps to solve some aspects of your issues. Please feel free to reach out to us over Discord or over here in case you find any more issues

@szabodaniel995
Copy link
Contributor

Thank you for the quick reply.
Had to properly configure and start up Redis & Docker on my PC, but its good now.

/attempt

@rajdip-b
Copy link
Member Author

Glad to hear that!

@rajdip-b rajdip-b moved this from Todo to In progress in keyshade-api Apr 21, 2024
@rajdip-b
Copy link
Member Author

@szabodaniel995

One thing that I forgot to mention in the issue, you would have to change the file names aswell! Something similar to how the file names and function names are now.

@github-project-automation github-project-automation bot moved this from In progress to Done in keyshade-api Apr 22, 2024
@rajdip-b rajdip-b moved this from Done to Queued for release in keyshade-api Apr 22, 2024
@rajdip-b
Copy link
Member Author

🎉 This issue has been resolved in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@rajdip-b rajdip-b moved this from Queued for release to Done in keyshade-api May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers scope: api Everything related to the API type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants