Context state keyError Exception message for humans #324
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
| redirect_url = self.config.get("UNKNOW_ERROR_REDIRECT_PAGE") | ||
|
There was a problem hiding this comment. This needs to be removed as @ioparaskev mentioned. There was a problem hiding this comment. @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))) | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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: 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!) |
||
| internal_response.requester = context.state[STATE_KEY]["requester"] | ||
|
|
||
| # If configured construct the user id from attribute values. | ||
|
|
||
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.
Which are the conditions that make this possible? Why would the state be empty?
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 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