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

Support For External IDP and External User Pool Provider In data.all #897

Merged
merged 21 commits into from
Dec 12, 2023
Merged

Support For External IDP and External User Pool Provider In data.all #897

merged 21 commits into from
Dec 12, 2023

Conversation

TejasRGitHub
Copy link
Contributor

@TejasRGitHub TejasRGitHub commented Dec 1, 2023 •
edited
Loading

Feature or Bugfix

  • Feature

Detail

Currently data.all uses Cognito for user login and also maintains the user pool for teams/groups in data.all.

This feature adds support to add any external OIDC based IdP. Apart from that , any external service provider can be used for maintaining teams/groups information.

This PR contains both backend and frontend changes for External IDP.

Note - When deploying with External IDP the user guide won't be setup as a part of deployment. Created following github issue to track this incremental change - #898

Testing

  • Local Development environment with Local Auth Context
  • make test - Unit Tests
  • Deployment to AWS With Cognito
  • Deployment to AWS with External IDP

Relates

Github Issue - #872

Credits - This code change was build on top of work done by @blitzmohit

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)? Yes
    • Is the input sanitized? Yes
    • What precautions are you taking before deserializing the data you consume? N/A
    • Is injection prevented by parametrizing queries? N/A
    • Have you ensured no eval or similar functions are used? N/A
  • Does this PR introduce any functionality or component that requires authorization? Yes
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms? Yes
    • Are you logging failed auth attempts? Yes ( At External IDP )
  • Are you using or adding any cryptographic features? No
    • 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? Yes
    • Have you used the least-privilege principle? How? Yes

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

@TejasRGitHub TejasRGitHub marked this pull request as ready for review December 1, 2023 18:43
@TejasRGitHub TejasRGitHub mentioned this pull request Dec 1, 2023
Closed
@TejasRGitHub
Copy link
Contributor Author

TejasRGitHub commented Dec 1, 2023 •
edited
Loading

Todo -

@TejasRGitHub TejasRGitHub marked this pull request as draft December 1, 2023 21:50
@TejasRGitHub TejasRGitHub reopened this Dec 1, 2023
@TejasRGitHub TejasRGitHub marked this pull request as ready for review December 1, 2023 22:11
dlpzx
dlpzx reviewed Dec 5, 2023
View reviewed changes
deploy/stacks/pipeline.py Outdated Show resolved Hide resolved
dlpzx
dlpzx reviewed Dec 5, 2023
View reviewed changes
deploy/stacks/cloudfront.py Outdated Show resolved Hide resolved
dlpzx
dlpzx reviewed Dec 5, 2023
View reviewed changes
deploy/stacks/cloudfront.py Outdated Show resolved Hide resolved
dlpzx
dlpzx reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

I am reviewing in parts. At the moment I am reviewing the deployment part

dlpzx
dlpzx reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

Some more comments. I still need to review the custom resources in the deployment

deploy/stacks/cloudfront.py Show resolved Hide resolved
backend/api_handler.py Show resolved Hide resolved
backend/api_handler.py Show resolved Hide resolved
deploy/stacks/lambda_api.py Show resolved Hide resolved
deploy/stacks/lambda_api.py Outdated Show resolved Hide resolved
@dlpzx
Copy link
Contributor

dlpzx commented Dec 8, 2023 •
edited
Loading

I am testing in AWS in a deployment with default parameters. (Cognito Auth)

  • There is a conflict to be resolved but is a very straight forward one @TejasRGitHub
    Tests:
  • CICD deploys successfully
  • login window rendered
  • open UI, first API call to api handler listEnvironments successfully returns the environments of my user
  • open catalog, API call to search handler successfully returns all data items
  • UI shows user guide
  • UI shows user name in the top right corner
  • invite team modal in environment lists all Cognito groups
  • create Dataset, list groups successfully and passes user as owner

noah-paige
noah-paige reviewed Dec 8, 2023
View reviewed changes
backend/dataall/core/groups/api/queries.py
from dataall.base.api import gql
from dataall.core.groups.api.resolvers import get_group, list_groups, get_groups_for_user

getGroup = gql.QueryField(
Copy link
Contributor

Choose a reason for hiding this comment

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

is getGroup used anywhere other than tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, atleast from what I searched . It just seems to have been used in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noah-paige is there any clean-up issue that is already filed ? This can be added into it

@TejasRGitHub TejasRGitHub reopened this Dec 8, 2023
@noah-paige
Copy link
Contributor

Testing in AWS in a deployment with VPC Facing Cognito Auth (with ReAuth enforced on CreateDataset and ListOrg APIs):

  • CICD deploys successfully
  • login window rendered
  • open UI, first API call to api handler listEnvironments successfully returns the environments of my user
  • open catalog, API call to search handler successfully returns all data items
  • UI shows user guide
  • UI shows user name in the top right corner
  • invite team modal in environment lists all Cognito groups
  • create Dataset, list groups successfully and passes user as owner
  • ReAuth on createDataset successful and successfully creates dataset after providing credentials again
  • ReAuth on ListOrg successful and successfully lists orgs and re-directs to page after providing credentials again
  • User Log Out from UI successfully

@noah-paige
Copy link
Contributor

noah-paige commented Dec 12, 2023 •
edited
Loading

Testing Custom Auth + VPC Facing (updating as I test):

  • CICD Pipeline Deploys --> Only After fixing ...-cognito-config-role issue
    • No User Guide Resources Created
    • Custom Authorizer Created
  • Log In via External IdP
  • Open UI, first API call to api handler listEnvironments successfully returns the environments of my user
  • open catalog, API call to search handler successfully returns all data items
  • UI shows user name in the top right corner
  • ReAuth on ListOrg successful and successfully lists orgs and re-directs to page after providing credentials again
  • User Log Out from UI successfully

@dlpzx
Copy link
Contributor

dlpzx commented Dec 12, 2023 •
edited
Loading

Testing in AWS internet-facing no custom-auth:

  • CICD Pipeline Deploys
  • User Guide Created
  • Log In via Cognito
  • Open UI, first API call to api handler listEnvironments successfully returns the environments of my user
  • open catalog, API call to search handler successfully returns all data items
  • UI shows user name in the top right corner
  • User Log Out from UI successfully
  • pen-test listGroups API call
    • calling the API directly and introducing a random user_id results in User Id doesn't match user id from context
    • calling the API directly and introducing the current user usesr_id returns the groups (for cognito this is an empty list)

dlpzx
dlpzx approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

Good to merge!

@dlpzx dlpzx merged commit 36b66c9 into data-dot-all:main Dec 12, 2023
9 checks passed
@noah-paige noah-paige mentioned this pull request Dec 12, 2023
Merged
dlpzx pushed a commit that referenced this pull request Dec 13, 2023
@noah-paige
Create frontend config role regardless of custom auth or not in backe…
2652202 
…nd (#913)

…nd stack

### Feature or Bugfix
<!-- please choose -->
- Bugfix

### Detail
- Config Role used to read SSM parameters in deployment account assumed
by tooling account needs to be created in the BackendStack so it can be
used in the pre CodeBuild commands of the ALBFront Stack
- Applies for VPC Facing Deployments

### Relates
- #897

### 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 Jan 10, 2024
Closed
@noah-paige noah-paige mentioned this pull request Jul 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbalintx dbalintx dbalintx left review comments

@noah-paige noah-paige noah-paige left review comments

@dlpzx dlpzx dlpzx 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.
4 participants
@TejasRGitHub @dlpzx @noah-paige @dbalintx