Do not crash when entity-selection-profile attribute has invalid JSON #286
Conversation
|
We also need to check that the json have a key called profiles. |
|
That would result in a warning in the logs, notice that that line is still in the |
src/pyff/samlmd.py
Outdated
| @@ -1047,14 +1047,25 @@ def discojson_sp_attr(e): | |||
| if b64_trustinfos is None: | |||
| return None | |||
|
|
|||
| entityID = e.get('entityID', None) | |||
| if entityID is None: | |||
| return None | |||
There was a problem hiding this comment.
Why would an entry have no entityID?
Isn't this check part of a validation step already, while ingesting input?
There was a problem hiding this comment.
Yeah you are right, I'll remove that
src/pyff/samlmd.py
Outdated
| try: | ||
| str_trustinfo = b64decode(b64_trustinfo.encode('ascii')) | ||
| trustinfo = json.loads(str_trustinfo.decode('utf8')) | ||
| sp['profiles'].update(trustinfo['profiles']) |
There was a problem hiding this comment.
This means that a profile entry can overwrite a previous entry.
Should we have a warning when that happens?
There was a problem hiding this comment.
Overwriting a previous entry would be an error in the side of the SP (publishing 2 profiles with the same name), and this warning would be seen by the aggregator, so this would seem to put some responsibility on the aggregator over the correctness of the SP metadata...
I'll add the warning anyway, it can always be ignored.
There was a problem hiding this comment.
to put some responsibility on the aggregator over the correctness of the SP metadata
I understand that, but at least the aggregator-operators can be aware and maybe contact the SP-operators to resolve this, instead of hiding the issue.
There was a problem hiding this comment.
There is also the possibility to have trust info in both JSON in an entity attribute and as XML in a TrustInfo element. At this point pyFF will produce discojson_sp with repeated entities, that will be merged by thiss-mdq. So not all equally named trust profiles will be caught here.
There was a problem hiding this comment.
I think these are issues and questions that should go back to the writers of the specification. They should guide us on the behaviour that is intended when such conflicts occur.
There was a problem hiding this comment.
Yes, at one point there was talk of removing the possibility of having trust info in XML, and allowing it only JSON in the entity attribute.
I have addressed both concerns above. I have also added the possibility to have extra_md in JSON trust info.
There was a problem hiding this comment.
Hi Ivan, there should be a meeting of the REFEDS spec working group on Thursday, and I'll bring your point up. We have explicitly said in the current draft (version 6):
- There is only one instance of an Entity Selection Profile entity attribute in an entity. Behaviour when more than one is present is not defined by this profile.
- Composition with other sources of filtering (such as SeamlessAccess button) is not defined by this profile
so will obviously need to revist those.
…tadata in trust info in entity attribute.
All Submissions: