Skip to content

fix: use quote instead of quote_plus - #324

Open
vladimir-mencl-eresearch wants to merge 3 commits into
IdentityPython:masterfrom
REANNZ:use_quote
Open

vladimir-mencl-eresearch wants to merge 3 commits into
IdentityPython:masterfrom
REANNZ:use_quote

Conversation

@vladimir-mencl-eresearch

Copy link
Copy Markdown
Contributor

MDQ spec explicitly asks for spaces to be encoded as %20, not +

This is achieved by urllib.parse.quote - the only difference quote_plus brings is it would be encoding spaces as +:
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.quote_plus

While entityIDs cannot really contain spaces (not permitted in an URI), this is all just a technicality, but quote is the correct function to use.

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • [N/A] Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@vladimir-mencl-eresearch

Copy link
Copy Markdown
Contributor Author

Rebased on current master

@vladimir-mencl-eresearch

Copy link
Copy Markdown
Contributor Author

@theseal rebased with signed commits

MDQ spec explicitly asks for spaces to be encoded as %20, not +

This is achieved by urllib.parse.quote - the only difference quote_plus
brings is it would be encoding spaces as '+':
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.quote_plus

While entityIDs cannot really contain spaces (not permitted in an URI),
this is all just a technicality, but quote is the correct function to use.
Got copy-pasted from set_reginfo below.

Here (in set_pubinfo), the element to be inserted is PublicationInfo.
There is a difference between quote and quote_plus in default value for safe:
* quote defaults to safe='/' (and so would pass '/' without encoding)
* quote_plus defaults to safe='' (and so encodes '/')

Invoke quote with safe='' to make sure '/' gets encoded.
@vladimir-mencl-eresearch

Copy link
Copy Markdown
Contributor Author

Hi @theseal ,

I thought this one would have been just a trivial fix (use quote instead of quote_plus which sounds off-place, no need to quote spaces with + in this context).

I just found the two functions also differ in default value for safe, so I've added a fix for that.

I also through in an unrelated fix of a typo in an error message (text was copied from another function but not adjusted for the text where it was used).

And I have one more change I'm considering adding: the set_pubinfo function where I fixed up the error message is only allowing PublicationInfo in the context of an EntitiesDescriptor, but the spec says it can appear either in EntitiesDescriptor or EntityDescriptor.

Allowing it for EntityDescriptor would make it possible to label MDQ data with PublicationInfo.

I already have a diff ready:

diff --git a/src/pyff/samlmd.py b/src/pyff/samlmd.py
index 75bfeb0..27a39a3 100644
--- a/src/pyff/samlmd.py
+++ b/src/pyff/samlmd.py
@@ -1400,8 +1400,8 @@ def set_entity_attributes(e, d, nf=NF_URI):


 def set_pubinfo(e, publisher=None, creation_instant=None):
-    if e.tag != "{{{}}}EntitiesDescriptor".format(NS['md']):
-        raise MetadataException("I can only set PublicationInfo to EntitiesDescriptor elements")
+    if not e.tag in ["{{{}}}EntityDescriptor".format(NS['md']), "{{{}}}EntitiesDescriptor".format(NS['md'])]:
+        raise MetadataException("I can only set PublicationInfo to EntityDescriptor or EntitiesDescriptor elements")
     if publisher is None:
         raise MetadataException("At least publisher must be provided")

What do you think - about this change and about what's already in the PR?

Should I add this change as well?

Please let me know.

Cheers,
Vlad

@c00kiemon5ter

Copy link
Copy Markdown
Member

I think it is a good addition.
Also, using if e.tag not in [... is more expressive.

This branch has not been deployed

No deployments
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.

2 participants