openedx / wg-data

Tracking work and progress of the Open edX Data Working Group
1 stars 2 forks source link

Spike: Investigate Apache Superset Permissions #21

Closed bmtcril closed 1 year ago

bmtcril commented 1 year ago

Superset is currently a front-runner as an open source reporting and visualization tool to replace the Insights front end. One point of concern is that it may not adequately handle the permissions needed for multi-tenancy on a single Open edX install. We would like to get clarity on this issue and any possible related configuration challenges early on in our investigations.

References: https://superset.apache.org/docs/security/ https://superset.apache.org/docs/installation/configuring-superset/#custom-oauth2-configuration

Acceptance Criteria:

Document each of the following steps as you go

pomegranited commented 1 year ago

assign me

pomegranited commented 1 year ago

WIP document: Investigate Apache Superset Permissions

pomegranited commented 1 year ago

@bmtcril I've finished the document Investigate Apache Superset Permissions, and moved my code customizations to https://github.com/open-craft/superset/pull/1 to make them easier to use and discuss.

However, I hit a caching issue -- we'd need to customize the caching system to handle course-based data too, which will affect performance.

You cannot SSO into Superset as a learner with no instructor roles

I neglected to implement this part -- but I did make it so that users with no instructor roles can't see any enrollment data.

What do you think? Is this enough work for this investigation, or should I continue trying to sort out these remaining issues?

pomegranited commented 1 year ago

@bmtcril From our meeting today, I'm increasing the scope here to include:

And I'll include "You cannot SSO into Superset as a learner with no instructor roles" in this work as well.

pomegranited commented 1 year ago

@bmtcril

  • add custom user-based caching

Good news! We don't have to write custom user-based caching: we just have to use the current_username() macro in our query, which by default, includes the current username in the cache key! ref https://github.com/apache/superset/issues/6092#issuecomment-559828355

See https://github.com/open-craft/superset/pull/1/commits/ecfa2498302248d53a7a73d4d4e3c1b90b03e380 for the fix and doc change.

  • You cannot SSO into Superset as a learner with no instructor roles

Supported this requirement with https://github.com/open-craft/superset/pull/1/commits/e8b13471052e628baa0f96b1b45fa2a483fbd56e

So all I need to do now is package this for Tutor :)

bmtcril commented 1 year ago

Fantastic news! Thanks for digging into this. I'm not sure how much Tutor plugin work you've done, but I found the docs at the Cookiecutter to be helpful. It may help to look at my Clickhouse plugin for reference as well.

pomegranited commented 1 year ago

This is my first Tutor plugin, so those references are very useful, thank you @bmtcril !

I've started this, but haven't finished it yet: https://github.com/open-craft/tutor-contrib-superset/pull/1

pomegranited commented 1 year ago

Hey @bmtcril , I've got a working Tutor plugin for Superset!

Wanna try it out/review my PR? https://github.com/open-craft/tutor-contrib-superset/pull/1

pomegranited commented 1 year ago

Merged https://github.com/open-craft/tutor-contrib-superset/pull/1, so we're done here :) FYI @bmtcril