Skip to content

Context state keyError Exception message for humans#324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
peppelinux wants to merge 1 commit into from
Closed

Context state keyError Exception message for humans #324

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/satosa/base.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -134,6 +134,12 @@ def _auth_resp_callback_func(self, context, internal_response):
"""

context.request = None
context_state = context.state.get(STATE_KEY)
if not context_state:

@c00kiemon5terc00kiemon5terApr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which are the conditions that make this possible? Why would the state be empty?

@peppelinuxpeppelinuxApr 29, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is a superior knowledge that I would gain when I grow up ;)

That's the footprint I collected:
#326 (comment)

Which method do you suggest to do, should we pick all the things with a python debugger or we should move to a big view that let us to satisfy our users without make too much noise in the code. It's a kind of paternalism that we acquire getting older, the users are tring to tell us something ...

Probably you know better than me how and when the context.state became inconsistent. Let's try to talk about this in a separate thread if you agree

redirect_url = self.config.get("UNKNOW_ERROR_REDIRECT_PAGE")

@c00kiemon5terc00kiemon5terApr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to be removed as @ioparaskev mentioned.

@peppelinuxpeppelinuxApr 29, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ioparaskev@c00kiemon5ter I miss something here, should we really print the things only on the logs and do not redirect users to a informational page?

We could find a transitional solution if you agree, something good for our users and then a following refactor with more design elements in the hands. Let me know, you know my limits, try your best :)

raise SATOSAStateError(('context.state has no {}. Your session '
'is not valid, please start a new '
'Authentication request again.'.format(STATE_KEY)))

@c00kiemon5terc00kiemon5terApr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is good to catch such cases or exceptions, but first we should make sure we understand the error, where it comes from and why it is happening. Can we change the code to make it as if the error did not happen at all?

@c00kiemon5terc00kiemon5terApr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As @ioparaskev said, SATOSA is currently not concerned about user-facing messages. This message is a mix of technical ("context.state has no") and user-facing information ("please start a new authentication"). I think that neither part is good. What we want to provide in an exception is not a message, but error context. Which information is useful to understand why this happened? This is the information that the exception should hold. I lean on making this information structured, a dict with meaningful keys and data as values.

@peppelinuxpeppelinuxApr 29, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think that the PR suggested by @skoranda is the solution for the 60% of this kind of problem, really. I follow the users in their daily abits and many of them use the Disco url to bookmark the SP... That's it.

Scott gave us the remediation here:
#322

Move in that direction, merge that first then come back here with better refactoring. I'm behind you (but with 2 meters of covid-social-distancing!)

@peppelinuxpeppelinuxDec 15, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ping here

internal_response.requester = context.state[STATE_KEY]["requester"]

# If configured construct the user id from attribute values.
Expand Down