Context state keyError Exception message for humans #324
Conversation
|
While I'm in favor of a more helpful error message in the logs (or in a FAQ maybe), the current log entry is like it is addressed to the user and not the admin that will look at the error. p.s: in the current commit while there's a |
|
Thank you John, you are proof of the existence of life here :') What we would do to get this job done? Would we instead use a redirect page? space for ideas, the integrations are often trivial |
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. We should not be masking errors that we haven't analysed and made sure that the handling of the error is in the right place.
In this specific case, the state has been created/parsed in a previous step. At which point in the flow do we have enough information to know that the state shouldn't have been empty?
Is this the right place? or is this just the place where the state was accessed and things broke?
| @@ -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: | |||
There was a problem hiding this comment.
Which are the conditions that make this possible? Why would the state be empty?
There was a problem hiding this comment.
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
| @@ -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 :)
| redirect_url = self.config.get("UNKNOW_ERROR_REDIRECT_PAGE") | ||
| 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:
#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!)
|
closed by d986464 |
|
See my comment here |
This PR introduces a human readable message to Users when they gets SATOSA_BASE KeyError.
Found it usefull to avoid weird Users open assistane tickets when they gets that "ermetic" error on the screen.