Skip to content
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.

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

add missing response body to blocked requests #6277

Merged
merged 1 commit into from
May 12, 2023
Merged

add missing response body to blocked requests #6277

merged 1 commit into from
May 12, 2023

Conversation

micbar
Copy link
Contributor

@micbar micbar commented May 10, 2023

Description

Adds the missing response body to blocked requests.

Examples

{
    "error":{
        "code":"deniedByPolicy",
        "message":"Operation denied due to security policies",
        "innererror":{
            "date":"2023-05-10T19:52:19Z",
            "filename":"policies.yaml",
            "method":"POST",
            "path":"/remote.php/dav/spaces/storage-users-1$some-admin-user-id-0000-000000000000",
            "request-id":"5e55a8ad-cd5f-40c9-a002-db9e30f24585"
        }
    }
}
{
    "error":{
        "code":"deniedByPolicy",
        "message":"Operation denied due to security policies",
        "innererror":{
            "date":"2023-05-10T20:05:00Z",
            "filename":"Neue Datei.md",
            "method":"PUT",
            "path":"/remote.php/dav/spaces/storage-users-1$some-admin-user-id-0000-000000000000/Neue Datei.md",
            "request-id":"9302a92e-c7eb-4c41-9a98-62eb37cf5db4"
        }
    }
}

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@AlexAndBear
Copy link
Contributor

AlexAndBear commented May 10, 2023

I don't think we want to have request denied at top level. This makes it hard to have a generic error handler.

Otherwise I like it

@micbar
Copy link
Contributor Author

micbar commented May 10, 2023

@michaelstingl This is a proposal to have a unified response body for all requests which are blocked by the policy engine to enable all clients to distinguish them. In a second step and in the scope of a cross platform initiative we should introduce server translated error messages.

@micbar
Copy link
Contributor Author

micbar commented May 10, 2023

I don't think we want to have request denied at top level. This makes it hard to have a generic error handler.

Otherwise I like it

use error ?

@AlexAndBear
Copy link
Contributor

I wouldn't nest it personally, because the http status code indicates the error already, but that's just personal preference. Error as top level is fine for me as well.

@micbar
Copy link
Contributor Author

micbar commented May 10, 2023

Error as top level is fine for me as well.

It is the same like libregraph. So keeping error at the top level makes it consistent.

@AlexAndBear
Copy link
Contributor

AlexAndBear commented May 10, 2023

💪

That's fine, maybe it's pretty neat to have it. Because in the future we might have error and errors and trough that we could have implementations for different interfaces.

Anyways: I would start the message with capital letter. In the end this is what the user reads (en).

(The operation ...)

@micbar
Copy link
Contributor Author

micbar commented May 10, 2023

Anyways: I would start the message with capital letter. In the end this is what the user reads (en).

Ok. Maybe a misunderstanding on my side. I thought we agree on a fixed string for now which will be translated in web.

@AlexAndBear
Copy link
Contributor

AlexAndBear commented May 10, 2023

Sure sure sure. But let's start this thing off properly and give it a handy message from the beginning, so that an English user gets a neat message.

We will translate it in web but when we change that in the future, you don't need to replace the English strings later ;)

@micbar
Copy link
Contributor Author

micbar commented May 10, 2023

Ok, tried to give it a meaningful and short message.

Operation denied due to security policies

@tbsbdr @kulmann

@ownclouders
Copy link
Contributor

ownclouders commented May 11, 2023

💥 Acceptance test localApiTests-apiGraph-ocis failed. Further test are cancelled...

@ownclouders
Copy link
Contributor

💥 Acceptance test localApiTests-apiCors-ocis failed. Further test are cancelled...

@sonarcloud
Copy link

sonarcloud bot commented May 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@micbar micbar merged commit 81e2365 into master May 12, 2023
@delete-merged-branch delete-merged-branch bot deleted the return-body branch May 12, 2023 06:38
ownclouders pushed a commit that referenced this pull request May 12, 2023
add missing response body to blocked requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants