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.

Sign up for GitHub

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

Jump to bottom

Introduce is_owner permissions to Glossary mutations + add new integration tests #1721

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Introduce is_owner permissions to Glossary mutations + add new integration tests #1721

merged 2 commits into from
Nov 29, 2024

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Nov 25, 2024

Feature or Bugfix

  • Feature
  • Bugfix
  • Refactoring

Detail

In the frontend Glossary operations that involve creating, modifying or deleting (WRITE) glossary resources are limited to the Glossary admins. To mimic this behavior in the backend this PR introduces permission checks that ensure that only the glossary admins can execute mutations on the glossary.

In addition, the PR includes integration tests for the unauthorized testing scenarios.

Testing

deployed Lambda in real AWS account

  • tested glossary owners can create, update and delete nodes
  • tested unauthorized users cannot execute API mutations programatically. They obtain errors of the type: An error occurred (UnauthorizedOperation) when calling GLOSSARY MUTATION operation:\n User [email protected] is not the admin of the glossary Sesssion glossary1.\n ", "locations": [{"line": 2, "column": 3}], "path": ["updateCategory"]}]}%

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)?
    • Is the input sanitized?
    • What precautions are you taking before deserializing the data you consume?
    • Is injection prevented by parametrizing queries?
    • Have you ensured no eval or similar functions are used?
  • Does this PR introduce any functionality or component that requires authorization?
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
    • Are you logging failed auth attempts?
  • Are you using or adding any cryptographic features?
    • Do you use a standard proven implementations?
    • Are the used keys controlled by the customer? Where are they stored?
  • Are you introducing any new policies/roles/users?
    • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

petrkalos
petrkalos reviewed Nov 28, 2024
View reviewed changes
backend/dataall/modules/catalog/services/glossaries_service.py Outdated
@@ -15,6 +17,30 @@ def _session():
return get_context().db_engine.scoped_session()


class GlossariesResourceAccess:
@staticmethod
def is_owner(f):
Copy link
Contributor

@petrkalos petrkalos Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer if we didn't use no args decorators (@is_owner vs @is_owner()) even though we don't require any args for consistency accross the project.
Also a small nit: After python3.2 we can write decorators using the contextlib.contextmanager which makes it much more simpler.
edit: I don't think it's possible to use contextmanager here because we extract the uri from the kwargs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the args decorators in 1d4cc2c

@dlpzx dlpzx merged commit 08f24f2 into main Nov 29, 2024
9 checks passed
@dlpzx dlpzx mentioned this pull request Dec 3, 2024
Merged
dlpzx added a commit that referenced this pull request Dec 3, 2024
@dlpzx
Avoid infinite loop in glossaries checks (#1725)
688bd0b 
### Feature or Bugfix
- Feature
- Bugfix

### Detail
In some edge cases where a category and term is orphan and does not have
a Glossary as parent we would run into an infinite loop in the
glossaries permission check. This PR adds a maximum depth level (which
in reality is lower, categories can only host terms, the
REAL_MAX_DEPTH=3)


### Relates
- #1721 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
This was referenced Dec 4, 2024
Merged
Closed
dlpzx added a commit that referenced this pull request Dec 5, 2024
@dlpzx
Introduce is_owner permissions to Glossary mutations + add new integr…
6df8588 
…ation tests (#1721)

<!-- please choose -->
- Feature
- Bugfix
- Refactoring

In the frontend Glossary operations that involve creating, modifying or
deleting (WRITE) glossary resources are limited to the Glossary admins.
To mimic this behavior in the backend this PR introduces permission
checks that ensure that only the glossary admins can execute mutations
on the glossary.

In addition, the PR includes integration tests for the unauthorized
testing scenarios.

deployed Lambda in real AWS account
- tested glossary owners can create, update and delete nodes
- tested unauthorized users cannot execute API mutations
programatically. They obtain errors of the type: `An error occurred
(UnauthorizedOperation) when calling GLOSSARY MUTATION operation:\n User
[email protected] is not the admin of the glossary Sesssion
glossary1.\n ", "locations": [{"line": 2, "column": 3}], "path":
["updateCategory"]}]}% `

Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
dlpzx added a commit that referenced this pull request Dec 5, 2024
@dlpzx
Avoid infinite loop in glossaries checks (#1725)
5c2b167 
### Feature or Bugfix
- Feature
- Bugfix

### Detail
In some edge cases where a category and term is orphan and does not have
a Glossary as parent we would run into an infinite loop in the
glossaries permission check. This PR adds a maximum depth level (which
in reality is lower, categories can only host terms, the
REAL_MAX_DEPTH=3)


### Relates
- #1721 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@dlpzx dlpzx mentioned this pull request Dec 9, 2024
Merged
dlpzx added a commit that referenced this pull request Jan 15, 2025
@dlpzx @mourya-33
@noah-paige @petrkalos @SofiaSazonova
2.6.2 Security features (#1737)
749ffc3 
### Feature or Bugfix
- Security

### Detail

### 🔐 Security
* Update sanitization technique for terms filtering by @noah-paige in
#1692 and in
#1693
* Move access logging to a separate environment logging bucket by
@noah-paige in #1695
* Add explicit token duration config for both JWTs by @noah-paige in
#1698
* Disable GraphQL introspection if prod sizing by @noah-paige in
#1704
* Add snyk workflow on schedule by @noah-paige in
#1705,
#1708,
#1713,
#1745 and in in
#1746
* Unify Logger Config for Tasks by @noah-paige in
#1709
* Updating overly permissive policies tagged by checkov for environment
role using least privilege principles by @mourya-33 in
#1632

Data.all permission model has been reviewed to ensure all Mutations and
Queries have proper permissions:
* Add MANAGE_SHARES permissions by @dlpzx in
#1702
* Add permission check - is tenant to update SSM parameters API by
@dlpzx in #1714
* Add GET_SHARE_OBJECT permissions to get data filters API by @dlpzx in
#1717
* Add permissions on list datasets for env group + cosmetic S3 Datasets
by @dlpzx in #1718
* Add GET_WORKSHEET permission in RUN_SQL_QUERY by @dlpzx in
#1716
* Add permissions to Quicksight monitoring service layer by @dlpzx in
#1715
* Add LIST_ENVIRONMENT_DATASETS permission for listing shared datasets
and cleanup unused code by @dlpzx in
#1719
* Add is_owner permissions to Glossary mutations + add new integration
tests by @dlpzx in #1721
* Refactor env permissions + modify getTrustAccount by @dlpzx in
#1712
* Add Feed consistent permissions by @dlpzx in
#1722
* Add Votes consistent permissions by @dlpzx in
#1724
* Consistent get_<DATA_ASSET> permissions - Dashboards by @dlpzx in
#1729


### 🧪 Test improvements
Integration tests are in sync with `main` without 2.7 planned features.
In this PR all core modules, optional modules and submodules are tested.
That includes: tenant-permissions, omics, mlstudio, votes, notifications
and backwards compatiblity of s3 shares. by @SofiaSazonova, @noah-paige
, @petrkalos and @dlpzx


In addition, the following PR adds functional tests that ensure the
permission model of data.all is not corrupted.
* ⭐ Add resource permission checks by @petrkalos in
#1711


### Dependencies
* Update FastAPI by @petrkalos in #1577 
* update fastapi dependency by @noah-paige in
#1699
* Upgrade "cross-spawn" to "7.0.5" by @dlpzx in
#1701
* Bump python runtime to bump cdk klayers cryptography version by
@noah-paige in #1707


### Relates
- List above

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: mourya-33 <[email protected]>
Co-authored-by: Mourya Darivemula <[email protected]>
Co-authored-by: Noah Paige <[email protected]>
Co-authored-by: Petros Kalos <[email protected]>
Co-authored-by: Sofia Sazonova <[email protected]>
Co-authored-by: Sofia Sazonova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrkalos petrkalos petrkalos approved these changes

@SofiaSazonova SofiaSazonova SofiaSazonova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dlpzx @SofiaSazonova @petrkalos