Skip to content

Cherry pick certificate authorization checks to Ironwood - #182

Closed
kaizoku wants to merge 3 commits into
opencraft-release/ironwood.2-asu-ytpfrom
josh/BB-1388
Closed

Cherry pick certificate authorization checks to Ironwood#182
kaizoku wants to merge 3 commits into
opencraft-release/ironwood.2-asu-ytpfrom
josh/BB-1388

Conversation

@kaizoku

@kaizoku kaizoku commented Jul 26, 2019

Copy link
Copy Markdown

This cherry picks an upstreamed WaffleFlag to require logins for certificate views.

This also adds the certificate status to the GeneratedCertificates admin page view.

JIRA tickets: Implements BB-1388.

Dependencies: None

Sandbox URL: https://pr20957.sandbox.opencraft.hosting/

Testing instructions:

  1. Generate a certificate to test with.
    /edx/bin/{python,manage}.edxapp lms --settings=devstack_docker create_fake_cert verified course-v1:edX+DemoX+Demo_Course
    
  2. Check the certificate renders normally with no login required in an logged-out browser: http://localhost:18000/certificates/user/8/course/course-v1:edX+DemoX+Demo_Course
  3. Enable the waffle switch
    /edx/bin/{python,manage}.edxapp lms --settings=devstack_docker waffle_switch certificates.require_login on
    
  4. Check the certificate page again and ensure it requires login now.

Author notes and concerns:

  1. Should we create a waffle configuration in a new file for the certificates application?

Reviewers

@giovannicimolin

Copy link
Copy Markdown

@kaizoku Your PR is failing because master has a feature to handle WaffleFlagNamespace that our ironwood branch doesn't have.
Can you check if it's possible to cleanly cherry pick these two lines to fix the issue?

@giovannicimolin giovannicimolin 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.

👍

  • I tested this:
  1. Checked out this branch.
  2. Generated fake certificate for user.
  3. Checked that the certificate was publicly accessible ✔️
  4. Switched certificates.require_login on
  5. Checked that the certificate page required login to be accessible, and the redirect pointed back to the correct page. ✔️
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation ⚠️

@kaizoku Thanks for the work here! Can you make these changes on the upstream PR too?

@lgp171188

Copy link
Copy Markdown

@josh, @giovannicimolin, is this still required? Asking because I found a sandbox for this PR on Ocim stage for this PR and was wondering if that can be archived.

mavidser pushed a commit that referenced this pull request Jul 2, 2020
Base automatically changed from opencraft-release/ironwood.2 to opencraft-release/ironwood.2-asu-ytp December 22, 2020 14:30
@Agrendalath

Copy link
Copy Markdown
Member

We're not using this branch anymore, so I'm closing the PR.

@Agrendalath Agrendalath closed this Nov 4, 2022
@Agrendalath
Agrendalath deleted the josh/BB-1388 branch November 4, 2022 12:20
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.

4 participants