Skip to content

Access Control Backends - #491

Merged
OmarIthawi merged 2 commits into
appsembler/tahoe/developfrom
omar/acl-backends
Dec 6, 2019
Merged

Access Control Backends#491
OmarIthawi merged 2 commits into
appsembler/tahoe/developfrom
omar/acl-backends

Conversation

@OmarIthawi

@OmarIthawi OmarIthawi commented Nov 26, 2019

Copy link
Copy Markdown

This is the base access control plugin system. Although it's intended to be used with the Course Access Group app it can be used for other purposes as well.

TODO

  • Create the base plugin system
  • Manually test the plugin system
  • Write tests for the plugin system
  • Write some code docs
  • Get a quick review
  • Get a detailed review

Background

Please do read the full proposal at the edX Discuss. The full design and discussion can be found there.

How to Review?

How to do manual testing on devstack?

  • Create a Red.yourlocaldomain microsite
  • Create a course with the id course-v1:Red+Red+Red2020
  • Create a user in the @example.com domain
  • Login with the @example.com user
  • The course should disappear from the http://red.yourlocaldomain/courses page
  • Logout
  • The course should appear again for the anonymous users

Why?

The end goal is to a Course Access Groups backend. This also makes it possible to implement more interesting stuff like soft course deletion.

@OmarIthawi
OmarIthawi force-pushed the omar/acl-backends branch 2 times, most recently from 39559ca to 9bea54e Compare November 28, 2019 10:56
@OmarIthawi OmarIthawi changed the title (WIP) Access Control Backends Access Control Backends Nov 28, 2019
@johnbaldwin

Copy link
Copy Markdown

@OmarIthawi For review criteria, you request downloading and running tests and trying out the edx-configs settings. Which environment should we use for testing? the new cloud devstack? And how do you suggest incorporating the new edx-configs changes into cloud devstack if that is what we should use for review?

@OmarIthawi

Copy link
Copy Markdown
Author

@OmarIthawi For review criteria, you request downloading and running tests and trying out the edx-configs settings. Which environment should we use for testing? the new cloud devstack? And how do you suggest incorporating the new edx-configs changes into cloud devstack if that is what we should use for review?

Great question @johnbaldwin. Running the tests is no longer a concern because it's being ran on TravisCI as of #479 .

Regarding testing this locally, I didn't use the new devstack (Sultan) because the local devstack on Linux works well, so I might not be able to provide an accurate idea of how the plugins should be installed.

So, perhaps we should just go with a detailed on-GitHub review and then merge and test on staging. What do you think?

@johnbaldwin johnbaldwin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@OmarIthawi Overall loos great! I haven't done a deep review, but I don't see tests for AccessControlBackends methods backends and query. Am I missing something?

Awesome work!

@OmarIthawi
OmarIthawi force-pushed the omar/acl-backends branch 3 times, most recently from df9baae to 5fca6ac Compare December 5, 2019 08:51
@OmarIthawi

OmarIthawi commented Dec 5, 2019

Copy link
Copy Markdown
Author

Thanks @johnbaldwin

I don't see tests for AccessControlBackends methods backends and query. Am I missing something?

I think you missed the second tests file. I've now changed the names so it's less confusing:

  1. Tests for the integration with the has_access method: lms/djangoapps/courseware/tests/test_access_control_backends_integration.py

  2. Tests the module in detail: lms/lib/tests/test_access_control_backends.py

@OmarIthawi
OmarIthawi force-pushed the omar/acl-backends branch 2 times, most recently from 439889c to 1376e87 Compare December 5, 2019 09:11

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

'exiting' or 'existing' ?

@johnbaldwin

Copy link
Copy Markdown

@OmarIthawi Was the lms/lib test missing? I now see it. Don't recall seeing it before. Anyway, thanks!

Only thing I saw right now looks like a typo test_query_exiting_backend. Should this be test_query_existing_backend? I commented inline

Otherwise, I don't see any reason not to merge this in and start working with it / proving it

Thanks!

@OmarIthawi

Copy link
Copy Markdown
Author

Thanks @johnbaldwin!

Was the lms/lib test missing? I now see it. Don't recall seeing it before. Anyway, thanks!

It was there 🤷‍♂️

Only thing I saw right now looks like a typo test_query_exiting_backend. Should this be test_query_existing_backend? I commented inline

Good catch. I've corrected it.

Otherwise, I don't see any reason not to merge this in and start working with it / proving it

Will do! Thanks!

cc: @melvinsoft

@OmarIthawi
OmarIthawi merged commit 54b0c3d into appsembler/tahoe/develop Dec 6, 2019
@OmarIthawi
OmarIthawi deleted the omar/acl-backends branch December 6, 2019 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants