Skip to content

Conversation

Maleware
Copy link
Member

@Maleware Maleware commented Aug 6, 2024

Description

This adds a custom manager which:

  1. Replaces the login button from superset with a direct link to keycloak ( login with keycloak ) and redirects to the product
  2. Replaces the logout button with a direct link to keycloak which logs out and returns to superset ( Session terminated )
  3. Adds Flask_OIDC and Flask_openID to the Docker image if it's product version 4.0.2 ( needed by fix )

Additionally there are going to be changes for the operator stackabletech/superset-operator#530

This is a little more complicated then expected but works the way outlined in 001-superset_logout.patch.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
- [ ] Changes are OpenShift compatible
- [ ] All added packages (via microdnf or otherwise) have a comment on why they are added
- [ ] Things not downloaded from Red Hat repositories should be mirrored in the Stackable repository and downloaded from there
- [ ] All packages should have (if available) signatures/hashes verified
- [ ] Add an entry to the CHANGELOG.md file
- [ ] Integration tests ran successfully
TIP: Running integration tests with a new product image

The image can be built and uploaded to the kind cluster with the following commands:

bake --product <product> --image-version <stackable-image-version>
kind load docker-image <image-tagged-with-the-major-version> --name=<name-of-your-test-cluster>

See the output of bake to retrieve the image tag for <image-tagged-with-the-major-version>.

@Maleware Maleware self-assigned this Aug 6, 2024
@lfrancke
Copy link
Member

lfrancke commented Aug 6, 2024

Quick question: Is this needed for everyone?
I don't want to fix a problem for one customer just for it to break for everyone else.

@Maleware
Copy link
Member Author

Maleware commented Aug 6, 2024

Yes it's a generic issue mentioned here apache/superset#23443 ( although that's Oauth ) This fixes the exact same problem for OIDC. The CustomSecurityManager will only be used if set in superset_config.py which means this is a opt-in feature and won't break anything for others ( if not willingly set )

@Maleware Maleware marked this pull request as draft August 6, 2024 14:26
@Maleware
Copy link
Member Author

Maleware commented Aug 6, 2024

Converted to draft as we want to discuss a bit more about this. To be picked up at some point.

@sbernauer
Copy link
Member

I think this waits on the decision https://github.com/stackabletech/decisions/issues/21, putting it back to progress

@Maleware
Copy link
Member Author

Further more, this waits on a discussion when @lfrancke is back since it's security relevance.

@Maleware Maleware marked this pull request as ready for review September 11, 2024 09:20
NickLarsenNZ
NickLarsenNZ previously approved these changes Sep 13, 2024
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Left a comment with a suggestion, but marked as approved in case my interpretation is wrong.

@sbernauer sbernauer changed the title Fix/superset OIDC logout feat(superset): Add needed Python libs for OIDC Sep 16, 2024
NickLarsenNZ
NickLarsenNZ previously approved these changes Oct 1, 2024
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Just a minor change required

@NickLarsenNZ NickLarsenNZ self-requested a review October 1, 2024 13:23
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

LGTM

@Maleware Maleware added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit cc80981 Oct 1, 2024
2 checks passed
@Maleware Maleware deleted the fix/superset-oidc-logout branch October 1, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants