-
Notifications
You must be signed in to change notification settings - Fork 81
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
add resource permission checks #1711
Conversation
891b681
to
02a7fa9
Compare
backend/dataall/modules/s3_datasets/api/storage_location/resolvers.py
Outdated
Show resolved
Hide resolved
backend/dataall/core/organizations/services/organization_service.py
Outdated
Show resolved
Hide resolved
3d342e2
to
e65674c
Compare
44353cd
to
6170b9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments on updates to ignore cases
Couple of high level Qs:
-
(1) Is there any way to assess that the FE client properly handles all partial data returns appropriately?
-
(2 - Not necessarily in this PR but …) should we change everywhere on our FE client side to check if there is no data rather than if no errors (i.e.
if (!response.errors)
) ++ dispatch all error if exist rather than just the first (i.e.dispatch({ type: SET_ERROR, error: response.errors[0].message });
)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final comment on get_environment_networks
changes
resource_perm=GET_DATASET_TABLE, tenant_ignore=IgnoreReason.NOTREQUIRED | ||
), | ||
field_id('DatasetTable', 'columns'): TestData( | ||
resource_ignore=IgnoreReason.CUSTOM, tenant_ignore=IgnoreReason.USERLIMITED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CUSTOM
as I understand it is that permissions are checked, but they are checked through their own mechanisms. Are there any checks happening in the columns resolver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I changed this after @noah-paige suggestion because it is checking the Confidentiality Classification...
@staticmethod
def paginate_active_columns_for_table(uri: str, filter=None):
context = get_context()
with context.db_engine.scoped_session() as session:
table: DatasetTable = DatasetTableRepository.get_dataset_table_by_uri(session, uri)
dataset = DatasetRepository.get_dataset_by_uri(session, table.datasetUri)
if (
ConfidentialityClassification.get_confidentiality_level(dataset.confidentiality)
!= ConfidentialityClassification.Unclassified.value
) and (dataset.SamlAdminGroupName not in context.groups and dataset.stewards not in context.groups):
raise exceptions.UnauthorizedOperation(
action='LIST_DATASET_TABLE_COLUMNS',
message='User is not authorized to view Columns for Confidential datasets',
)
return DatasetColumnRepository.paginate_active_columns_for_table(session, uri, filter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some nit changes but overall I think it is good to go. I was not strict with Redshift or Metadata Forms
2c1e9a4
to
ce71471
Compare
Feature * introducing a test that is going through all the nested SQL queries and assert that `ResourcePolicyService.check_user_resource_permission` have been called with the expected permission name OR explicitly ignore the test. * New subqueries will be tested automatically and fail if the expected permission is missing * Removed queries will make the test suite fail to avoid keeping stale permissions * UI: Make handling responses (i.e ListDatasets, GetDataset) more tolerant to missing information (i.e missing Stack) by doing conditional rendering. Example usecase: A dataset is being shared by a user but only owners have permissions to see stack and environment info. * Override config.json and enable all modules when running the tests. As a result checkov now synths the pipeline module that throws some errors (added in the baseline). @noah-paige * Make TestClient more tolerant to GQLErrors previously it would always throw if errors, now it will throw if there are only erros (and no data) allowing for partial information to be returned to the caller 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.
Feature or Bugfix
Feature
Detail
ResourcePolicyService.check_user_resource_permission
have been called with the expected permission name OR explicitly ignore the test.Example usecase: A dataset is being shared by a user but only owners have permissions to see stack and environment info.
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.