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

Support Attribute-Based Access Control (ABAC) in Permit Check #24

Merged
merged 6 commits into from
Nov 30, 2024

Conversation

ARYPROGRAMMER
Copy link
Contributor


This PR introduces support for Attribute-Based Access Control (ABAC) to the permit-cli, addressing Issue #14.

Key Changes:

  • Attribute Support:
    • Added the ability to use user and resource attributes in a flexible key:value format, enabling ABAC for the first time.
  • Backward Compatibility:
    • Existing Role-Based Access Control (RBAC) functionality remains intact, ensuring no breaking changes for current users.
  • Comprehensive End-to-End (e2e) Tests:
    • Implemented extensive e2e tests covering both ABAC and RBAC workflows to ensure stability and correctness in all scenarios.

Additional Features:

  • Support for Multiple Data Types:
    • The new ABAC feature supports different value types, including string, number, and boolean, making it flexible for diverse use cases.
  • Improved Error Handling:
    • Enhanced error handling with robust validation to prevent invalid attribute types and configurations. Clear, actionable error messages guide users in case of mistakes.
  • Expanded Test Coverage:
    • Test coverage increased significantly, ensuring that all edge cases, such as missing attributes or invalid types, are covered.

Future Enhancements:

  • Dynamic Attribute Validation:
    • This PR lays the foundation for future improvements, including dynamic validation of user and resource attributes against a predefined schema.

Screenshots:

Here's an example of the new functionality in action:

Sample Run

Testing:

  • e2e Tests: All new and existing tests pass, and additional scenarios have been covered to ensure no regressions occur.
  • Manual Testing: Performed on various environments to confirm compatibility with both small and large datasets.

This PR not only resolves the core issue but also makes the permit-cli more versatile for a wider range of access control scenarios.


By emphasizing backward compatibility, expanded test coverage, and robust error handling, this PR should make a strong case for acceptance.

Copy link

quest-bot bot commented Oct 30, 2024

Quest PR submitted! image Quest PR submitted!

@ARYPROGRAMMER, you are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Oct 30, 2024
@gemanor
Copy link
Collaborator

gemanor commented Oct 31, 2024

It looks like a good start overall. When you're ready with it, let me know so I can review it in detail.

The only thing I'm a little worried about is nested attributes, maybe worth to add also possibility to add json as file.

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 31, 2024

It looks like a good start overall. When you're ready with it, let me know so I can review it in detail.

The only thing I'm a little worried about is nested attributes, maybe worth to add also possibility to add json as file.

hey @gemanor you can start reviewing and merging, all addition ideas of mine seem to break the system further on

@gemanor
Copy link
Collaborator

gemanor commented Nov 3, 2024

Hey @ARYPROGRAMMER can you please upload a video of it working? I'm trying to run it but it has some ink-text errors

@ARYPROGRAMMER
Copy link
Contributor Author

Hey @ARYPROGRAMMER can you please upload a video of it working? I'm trying to run it but it has some ink-text errors

Here's a sample video for your kind reference: https://vimeo.com/1025929148?share=copy#t=0

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Hey @ARYPROGRAMMER , can you please merge with Main and ensure all the lint checks is passing?

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Nov 24, 2024

Hey @ARYPROGRAMMER , can you please merge with Main and ensure all the lint checks is passing?

Resolved Conflicts, LGTM for merging

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Hey, the code indeed looks good.
I'm only asking for a merge of another PR that uses vitest and adopt the tests to pass on vitest before merging (shouldn't be too much changes). Will update when it merges.

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Nov 24, 2024

Hey, the code indeed looks good. I'm only asking for a merge of another PR that uses vitest and adopt the tests to pass on vitest before merging (shouldn't be too much changes). Will update when it merges.

Hey, just having a doubt - so after the other PR for vitest gets merged, this can be merged right? Also do I need to create that another PR for vitest or you guys are doing it.

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Assuming the tests are 100% compatible with vitest, you can indeed merge it right after (and another CR). If not, you'll have to do the little adoption to support the vitest runner.

@ARYPROGRAMMER
Copy link
Contributor Author

Assuming the tests are 100% compatible with vitest, you can indeed merge it right after (and another CR). If not, you'll have to do the little adoption to support the vitest runner.

Here's what I get : I have to raise a PR to support vitest runner and make sure the existing tests and tests in this PR are 100% compatible with it. If that is the case then in either way we should first merge this once and then raise the other PR to support vitest runner, I think that will be more effective.

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

The following PR is adding support in vitest. You have nothing to do now but wait for this one to get merged (in the next 48 hours, probably). #36

Only after then, you'll have to verify that your tests is passing in vitest runner.

Is that make sense?

@ARYPROGRAMMER
Copy link
Contributor Author

The following PR is adding support in vitest. You have nothing to do now but wait for this one to get merged (in the next 48 hours, probably). #36

Only after then, you'll have to verify that your tests is passing in vitest runner.

Is that make sense?

Thanks a lot, got the reference finally. Eager to contribute more to the project.

@ARYPROGRAMMER
Copy link
Contributor Author

@gemanor updates on this one?

@gemanor
Copy link
Collaborator

gemanor commented Nov 29, 2024

Hey, please install the same vitest packages that in the following package.json: https://github.com/Abiji-2020/permit-cli/blob/gitops-github-create-wizard/package.json

After that, make sure the command npx vitest run is passing (you'll probably have to modify a little the imports`

@gemanor
Copy link
Collaborator

gemanor commented Nov 29, 2024

Please also merge from main, and run the tests there.

@ARYPROGRAMMER
Copy link
Contributor Author

@gemanor i think its done now
npx vitest run:

image

@ARYPROGRAMMER
Copy link
Contributor Author

@gemanor lgtm for merging, let me know further

@gemanor gemanor self-requested a review November 30, 2024 20:46
Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

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

Nice Work! Thanks!

@gemanor gemanor merged commit d9f09cf into permitio:main Nov 30, 2024
3 checks passed
@Abiji-2020
Copy link
Contributor

@ARYPROGRAMMER currently after updating i had run the tests my tests are currently failing.
initialy it showed an timeout error, so i increased the timeout for 10000ms now this assertion error occurs.
image

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Dec 1, 2024

@ARYPROGRAMMER currently after updating i had run the tests my tests are currently failing. initialy it showed an timeout error, so i increased the timeout for 10000ms now this assertion error occurs. image

Yeah might be due to environment setup error, not a bug or issue

@Abiji-2020
Copy link
Contributor

https://github.com/permitio/permit-cli/actions/runs/12102149170/job/33742888763

I have tried to run in the CI as well, could you help me in setting up the environment, like what is missing ?

@ARYPROGRAMMER
Copy link
Contributor Author

https://github.com/permitio/permit-cli/actions/runs/12102149170/job/33742888763

I have tried to run in the CI as well, could you help me in setting up the environment, like what is missing ?

Looks like your cached modules have affected some files or maybe you have made some change that interferes. Trying running locally first and setting up policy env key as per documentation

@Abiji-2020
Copy link
Contributor

In test the env keys are mocked right?

Copy link

quest-bot bot commented Dec 9, 2024

🧚 @ARYPROGRAMMER congratulations for completing Quest #14

💰 A reward of $150 has been credited to you.

To claim your $150 reward follow the instructions here.

Questions? Check out the docs.

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

@ARYPROGRAMMER the tests are failing now. Also in local with a fresh environment. Can you check that please?

@ARYPROGRAMMER
Copy link
Contributor Author

@ARYPROGRAMMER the tests are failing now. Also in local with a fresh environment. Can you check that please?

is this behaviour happening due to some new PR merging? I dont think it was happening earlier, I'll anyways test once and let you know meanwhile if you could provide some insights on how this happened or when it was discovered then i can debug further

@Abiji-2020
Copy link
Contributor

@ARYPROGRAMMER the tests are failing now. Also in local with a fresh environment. Can you check that please?

@gemanor i had modified it them in the next pr

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

@Abiji-2020, yeah, saw that now. Let's communicate it there then

@ARYPROGRAMMER
Copy link
Contributor Author

@ARYPROGRAMMER the tests are failing now. Also in local with a fresh environment. Can you check that please?

checked that , I don't think i should raise a PR for this since it is actually not a failure. @Abiji-2020 must have increased the timeout and it should work then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚔️ Quest Tracks quest-bot quests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants