Skip to content

Fix errors with fetching Shibboleth metadata - #8733

Merged
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:fix-shibboleth-fetch
Jul 1, 2015
Merged

Fix errors with fetching Shibboleth metadata#8733
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:fix-shibboleth-fetch

Conversation

@bradenmacdonald

Copy link
Copy Markdown
Contributor

Description:
I found a couple of bugs with the Shibboleth metadata fetching (See #8155, #8140).

First, due to an indentation error, the fetch code was returning prematurely and wouldn't fetch all the providers.

Second, if a provider's metadata specifies an encoding, e.g. <?xml version="1.0" encoding="UTF-8"?>, then the XML parser would reject it:

ValueError: Unicode strings with encoding declaration are not supported. Please use bytes input or XML fragments without declaration.

The solution is to pass the XML data as a bytestring rather than a unicode string. I included an updated test for this.

Merge target: Should be included in Cypress as it is pretty key to the SAML functionality.

JIRA: None

Dependencies: None

Sandbox: http://sandbox5.opencraft.com

Test Instructions:
To test on the sandbox, SSH in and as edxapp run the command ./manage.py lms saml pull --settings=aws.

To test on a devstack, configure at least two SAML providers and run the command ./manage.py lms saml pull --settings=devstack. For the metadata URL setting, one can be any a nonexistent URL and the other should be http://calnet-tin.ist.berkeley.edu/shib/shib-test.b.e-metadata.xml which is the metadata file that made me aware of the encoding issue.

Reviewers: @e-kolpakov and Cale

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8733

@e-kolpakov

Copy link
Copy Markdown
Contributor

@bradenmacdonald 👍

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@cpennington Do you have a minute to review this small bugfix PR for the Shibboleth fetch task? I'd like to ensure it gets into Cypress. Sorry for the short notice.

@cpennington

Copy link
Copy Markdown
Contributor

+1
On Jul 1, 2015 4:12 AM, "Braden MacDonald" notifications@github.com wrote:

@cpennington https://github.com/cpennington Do you have a minute to
review this small bugfix PR for the Shibboleth fetch task? I'd like to
ensure it gets into Cypress. Sorry for the short notice.


Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/8733#issuecomment-117524858.

bradenmacdonald added a commit that referenced this pull request Jul 1, 2015
Fix errors with fetching Shibboleth metadata
@bradenmacdonald
bradenmacdonald merged commit cd3abf3 into openedx:master Jul 1, 2015
@bradenmacdonald
bradenmacdonald deleted the fix-shibboleth-fetch branch July 1, 2015 16:17
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! I've created OSPR-1387 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants