IBX-5094: Added try-catch block for UnauthorizedException

[mateuszdebinski]

Jira: https://issues.ibexa.co/browse/IBX-5094

[konradoboza]

I think throwing GraphQL\Error\UserError might be more informative than no data returned. POV ping @bdunogier.

[alongosz]

I think throwing GraphQL\Error\UserError might be more informative than no data returned. POV ping @bdunogier.

return null, will allow you to display the sub-items list but without the section and without the error that is currently going to the logs

Looks like this is a "dangling" class tied to GraphQL only by DI, which makes it difficult to understand what exactly calls it and what is the data flow.

@mateuszdebinski the issue only mentions logs. Does this mean that the output is correct and the issue occurs only in logs?

If we throw GraphQL\Error\UserError instead of our exception, how does that change the output and logs? There's very little information to go on, so it's hard to review this one without spending some time on reverse-engineering.

[konradoboza]

If we throw GraphQL\Error\UserError instead of our exception, how does that change the output and logs? There's very little information to go on, so it's hard to review this one without spending some time on reverse-engineering.

My idea was to inform (UserError will be displayed in GraphiQL client while performing query) the user that they are not authorized to see specific data. However, filtered results (skipping content user doesn't have access to) and properly logged error seems to be the best way to handle this bug in my opinion.

[mateuszdebinski]

@alongosz Yes, this issue did not cause any other issues, apart from the entry in the logs, still, the user could see sub-items.
@konradoboza In other places like trash, we have verification whether the user can see the section and if not, it is not displayed.

If the user does not have access to the selected section, should we put information into the logs if it does not stop the operation and allows you to still display the information?

[alongosz]

My idea was to inform (UserError will be displayed in GraphiQL client while performing query) the user that they are not authorized to see specific data. However, filtered results (skipping content user doesn't have access to) and properly logged error seems to be the best way to handle this bug in my opinion.

(...) the issue only mentions logs. Does this mean that the output is correct and the issue occurs only in logs?

@alongosz Yes, this issue did not cause any other issues, apart from the entry in the logs, still, the user could see sub-items. @konradoboza In other places like trash, we have verification whether the user can see the section and if not, it is not displayed.

If the user does not have access to the selected section, should we put information into the logs if it does not stop the operation and allows you to still display the information?

IMHO it's a good idea to show it in logs and GraphiQL client because that might not be expected result, but permission "misconfiguration". The question is how does UserError look in logs? I'd avoid app.Critical as this seems to cause panic ;-)

[mateuszdebinski]

@alongosz I tested it, and, UserError doesn't throw any error into the log but in graphQL, you get something like this:
"errors": [ { "message": "error", "extensions": { "category": "user" }, "locations": [ { "line": 47, "column": 37 } ], "path": [ "_repository", "location", "children", "edges", 0, "node", "content", "_info", "section" ] } ],

where "message": "error", "extensions": { "category": "user" }, it's configurable

[alongosz]

@alongosz I tested it, and, UserError doesn't throw any error into the log but in graphQL, you get something like this: "errors": [ { "message": "error", "extensions": { "category": "user" }, "locations": [ { "line": 47, "column": 37 } ], "path": [ "_repository", "location", "children", "edges", 0, "node", "content", "_info", "section" ] } ],

where "message": "error", "extensions": { "category": "user" }, it's configurable

It seems that it provides value because someone might expect a section, but not get it and wonder why.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[micszo]

The problem no longer occurs.

QA Approved on Ibexa Commerce 3.3.34-dev
(diff ok on gql v2.3.16)