GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Do not crash when entity-selection-profile attribute has invalid JSON by enriquepablo · Pull Request #286 · IdentityPython/pyFF

Conversation

enriquepablo commented Nov 27, 2024

Copy link

Copy Markdown

Contributor

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?
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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Do not crash when entity-selection-profile attribute has invalid JSON

f733b10

btmattsson commented Nov 28, 2024

Copy link

Copy Markdown

We also need to check that the json have a key called profiles.
If not sp['profiles'].update(trustinfo['profiles']) will break ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

enriquepablo commented Nov 28, 2024

Copy link

Copy Markdown

Contributor Author

That would result in a warning in the logs, notice that that line is still in the try block

btmattsson reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

enriquepablo force-pushed the master branch from 4689e38 to f733b10 Compare

January 8, 2025 15:34

c00kiemon5ter reviewed

Jan 11, 2025

View reviewed changes

Comment thread

src/pyff/samlmd.py Outdated


		entityID = e.get('entityID', None)
		if entityID is None:
		return None

c00kiemon5ter Jan 11, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why would an entry have no entityID?
Isn't this check part of a validation step already, while ingesting input?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

enriquepablo Jan 13, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yeah you are right, I'll remove that

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

c00kiemon5ter reviewed

Jan 11, 2025

View reviewed changes

Comment thread

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'])

c00kiemon5ter Jan 11, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This means that a profile entry can overwrite a previous entry.
Should we have a warning when that happens?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

enriquepablo Jan 13, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

c00kiemon5ter reacted with thumbs up emoji

All reactions

👍 1 reaction

c00kiemon5ter Jan 13, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

enriquepablo Jan 13, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

c00kiemon5ter Jan 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

enriquepablo Jan 13, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

alexstuart Jan 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

c00kiemon5ter reacted with heart emoji

All reactions

❤️ 1 reaction

c00kiemon5ter mentioned this pull request

Jan 11, 2025

Scoped display names #288

Open

6 tasks done

enriquepablo added 2 commits

January 13, 2025 11:51


          warn when trust info has profiles with same name. Also allow extra me…

b392203

…tadata in trust info in entity attribute.


          select with duplicates from different sources

0fb326d

This branch has not been deployed

No deployments

Reviewers

c00kiemon5ter

c00kiemon5ter left review comments

+1 more reviewer

alexstuart

alexstuart left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

enriquepablo commented Nov 27, 2024

All Submissions:

Uh oh!

btmattsson commented Nov 28, 2024

Uh oh!

enriquepablo commented Nov 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c00kiemon5ter Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexstuart Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

c00kiemon5ter Jan 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

alexstuart Jan 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.