From 8dbad84a581e1672f01ca6be246be42f86094f46 Mon Sep 17 00:00:00 2001 From: Alex <33497058+bexsoft@users.noreply.github.com> Date: Mon, 23 Oct 2023 10:17:44 -0600 Subject: [PATCH] Improved error handling in Object Browser (#3097) Signed-off-by: Benjamin Perez --- .../ListBuckets/Objects/ListObjects/types.tsx | 9 +- .../websockets/objectBrowserWSMiddleware.ts | 37 ++++-- .../tests/permissions-7/errorsVisibleOB.ts | 116 ++++++++++++++++++ .../tests/policies/conditionsPolicy4.json | 44 +++++++ portal-ui/tests/scripts/cleanup-env.sh | 2 + portal-ui/tests/scripts/common.sh | 3 + portal-ui/tests/scripts/permissions.sh | 2 + portal-ui/tests/utils/roles.ts | 11 ++ restapi/admin_objects.go | 2 +- restapi/errors.go | 4 + restapi/ws_objects.go | 30 ++++- 11 files changed, 244 insertions(+), 16 deletions(-) create mode 100644 portal-ui/tests/permissions-7/errorsVisibleOB.ts create mode 100644 portal-ui/tests/policies/conditionsPolicy4.json diff --git a/portal-ui/src/screens/Console/Buckets/ListBuckets/Objects/ListObjects/types.tsx b/portal-ui/src/screens/Console/Buckets/ListBuckets/Objects/ListObjects/types.tsx index 106c19a90..e44792463 100644 --- a/portal-ui/src/screens/Console/Buckets/ListBuckets/Objects/ListObjects/types.tsx +++ b/portal-ui/src/screens/Console/Buckets/ListBuckets/Objects/ListObjects/types.tsx @@ -14,7 +14,7 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . -import { BucketObject } from "api/consoleApi"; +import { ApiError, BucketObject } from "api/consoleApi"; import { IFileInfo } from "../ObjectDetails/types"; export interface BucketObjectItem { @@ -38,13 +38,18 @@ export interface WebsocketRequest { export interface WebsocketResponse { request_id: number; - error?: string; + error?: WebsocketErrorResponse; request_end?: boolean; data?: ObjectResponse[]; prefix?: string; bucketName?: string; } +export interface WebsocketErrorResponse { + Code: number; + APIError: ApiError; +} + export interface ObjectResponse { name: string; last_modified: string; diff --git a/portal-ui/src/websockets/objectBrowserWSMiddleware.ts b/portal-ui/src/websockets/objectBrowserWSMiddleware.ts index 4014a3611..5acdfbea8 100644 --- a/portal-ui/src/websockets/objectBrowserWSMiddleware.ts +++ b/portal-ui/src/websockets/objectBrowserWSMiddleware.ts @@ -85,6 +85,12 @@ export const objectBrowserWSMiddleware = ( }; objectsWS.onmessage = (message) => { + const basicErrorMessage = { + errorMessage: "An error occurred", + detailedMessage: + "An unknown error occurred. Please refer to Console logs to get more information.", + }; + const response: WebsocketResponse = JSON.parse( message.data.toString(), ); @@ -94,13 +100,10 @@ export const objectBrowserWSMiddleware = ( return; } - if ( - response.error === - "The Access Key Id you provided does not exist in our records." - ) { - // Session expired. + if (response.error?.Code === 401) { + // Session expired. We reload this page window.location.reload(); - } else if (response.error === "Access Denied.") { + } else if (response.error?.Code === 403) { const internalPathsPrefix = response.prefix; let pathPrefix = ""; @@ -119,10 +122,15 @@ export const objectBrowserWSMiddleware = ( ); if (!permitItems || permitItems.length === 0) { + const errorMsg = response.error.APIError; + dispatch( setErrorSnackMessage({ - errorMessage: response.error, - detailedError: response.error, + errorMessage: + errorMsg.message || basicErrorMessage.errorMessage, + detailedError: + errorMsg.detailedMessage || + basicErrorMessage.detailedMessage, }), ); } else { @@ -131,6 +139,19 @@ export const objectBrowserWSMiddleware = ( } return; + } else if (response.error) { + const errorMsg = response.error.APIError; + + dispatch(setRequestInProgress(false)); + dispatch( + setErrorSnackMessage({ + errorMessage: + errorMsg.message || basicErrorMessage.errorMessage, + detailedError: + errorMsg.detailedMessage || + basicErrorMessage.detailedMessage, + }), + ); } // This indicates final messages is received. diff --git a/portal-ui/tests/permissions-7/errorsVisibleOB.ts b/portal-ui/tests/permissions-7/errorsVisibleOB.ts new file mode 100644 index 000000000..1deb6e75a --- /dev/null +++ b/portal-ui/tests/permissions-7/errorsVisibleOB.ts @@ -0,0 +1,116 @@ +// This file is part of MinIO Console Server +// Copyright (c) 2023 MinIO, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +import * as roles from "../utils/roles"; +import { Selector } from "testcafe"; +import * as functions from "../utils/functions"; +import { namedTestBucketBrowseButtonFor } from "../utils/functions"; + +fixture("Test error visibility in Object Browser Navigation").page( + "http://localhost:9090/", +); + +const bucketName = "my-company"; +const bucketName2 = "my-company2"; +const bucketBrowseButton = namedTestBucketBrowseButtonFor(bucketName); +const bucketBrowseButton2 = namedTestBucketBrowseButtonFor(bucketName2); +export const file = Selector(".ReactVirtualized__Table__rowColumn").withText( + "test.txt", +); +export const deniedError = Selector(".message-text").withText("Access Denied."); + +test + .before(async (t) => { + await functions.setUpNamedBucket(t, bucketName); + await functions.uploadNamedObjectToBucket( + t, + bucketName, + "test.txt", + "portal-ui/tests/uploads/test.txt", + ); + await functions.uploadNamedObjectToBucket( + t, + bucketName, + "home/UserY/test.txt", + "portal-ui/tests/uploads/test.txt", + ); + await functions.uploadNamedObjectToBucket( + t, + bucketName, + "home/UserX/test.txt", + "portal-ui/tests/uploads/test.txt", + ); + })( + "Error Notification is shown in Object Browser when no privileges are set", + async (t) => { + await t + .useRole(roles.conditions3) + .navigateTo(`http://localhost:9090/browser`) + .click(bucketBrowseButton) + .click(Selector(".ReactVirtualized__Table__rowColumn").withText("home")) + .click( + Selector(".ReactVirtualized__Table__rowColumn").withText("UserX"), + ) + .expect(deniedError.exists) + .ok(); + }, + ) + .after(async (t) => { + await functions.cleanUpNamedBucketAndUploads(t, bucketName); + }); + +test + .before(async (t) => { + await functions.setUpNamedBucket(t, bucketName2); + await functions.setVersionedBucket(t, bucketName2); + await functions.uploadNamedObjectToBucket( + t, + bucketName2, + "test.txt", + "portal-ui/tests/uploads/test.txt", + ); + await functions.uploadNamedObjectToBucket( + t, + bucketName2, + "home/UserY/test.txt", + "portal-ui/tests/uploads/test.txt", + ); + await functions.uploadNamedObjectToBucket( + t, + bucketName2, + "home/UserX/test.txt", + "portal-ui/tests/uploads/test.txt", + ); + })( + "Error Notification is shown in Object Browser with Rewind request set", + async (t) => { + await t + .useRole(roles.conditions4) + .navigateTo(`http://localhost:9090/browser`) + .click(bucketBrowseButton2) + .click(Selector("label").withText("Show deleted objects")) + .wait(1500) + .click(Selector(".ReactVirtualized__Table__rowColumn").withText("home")) + .click( + Selector(".ReactVirtualized__Table__rowColumn").withText("UserX"), + ) + .expect(deniedError.exists) + .ok(); + }, + ) + .after(async (t) => { + await functions.cleanUpNamedBucketAndUploads(t, bucketName2); + }); diff --git a/portal-ui/tests/policies/conditionsPolicy4.json b/portal-ui/tests/policies/conditionsPolicy4.json new file mode 100644 index 000000000..9001c4fa5 --- /dev/null +++ b/portal-ui/tests/policies/conditionsPolicy4.json @@ -0,0 +1,44 @@ +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "AllowUserToSeeBucketListInTheConsole", + "Action": [ + "s3:ListAllMyBuckets", + "s3:GetBucketLocation", + "s3:GetBucketVersioning" + ], + "Effect": "Allow", + "Resource": ["arn:aws:s3:::*"] + }, + { + "Sid": "AllowRootAndHomeListingOfCompanyBucket", + "Action": ["s3:ListBucket", "s3:List*"], + "Effect": "Allow", + "Resource": ["arn:aws:s3:::my-company2"], + "Condition": { + "StringEquals": { + "s3:prefix": ["", "home/", "home/User"], + "s3:delimiter": ["/"] + } + } + }, + { + "Sid": "AllowListingOfUserFolder", + "Action": ["s3:ListBucket", "s3:List*"], + "Effect": "Allow", + "Resource": ["arn:aws:s3:::my-company2"], + "Condition": { + "StringLike": { + "s3:prefix": ["home/User/*"] + } + } + }, + { + "Sid": "AllowAllS3ActionsInUserFolder", + "Effect": "Allow", + "Action": ["s3:*"], + "Resource": ["arn:aws:s3:::my-company2/home/User/*"] + } + ] +} diff --git a/portal-ui/tests/scripts/cleanup-env.sh b/portal-ui/tests/scripts/cleanup-env.sh index 6a4421465..6bc1f2121 100644 --- a/portal-ui/tests/scripts/cleanup-env.sh +++ b/portal-ui/tests/scripts/cleanup-env.sh @@ -31,6 +31,7 @@ remove_users() { mc admin user remove minio conditions-$TIMESTAMP mc admin user remove minio conditions-2-$TIMESTAMP mc admin user remove minio conditions-3-$TIMESTAMP + mc admin user remove minio conditions-4-$TIMESTAMP } remove_policies() { @@ -56,6 +57,7 @@ remove_policies() { mc admin policy remove minio conditions-policy-$TIMESTAMP mc admin policy remove minio conditions-policy-2-$TIMESTAMP mc admin policy remove minio conditions-policy-3-$TIMESTAMP + mc admin policy remove minio conditions-policy-4-$TIMESTAMP } __init__() { diff --git a/portal-ui/tests/scripts/common.sh b/portal-ui/tests/scripts/common.sh index d99c9d220..9604a586e 100755 --- a/portal-ui/tests/scripts/common.sh +++ b/portal-ui/tests/scripts/common.sh @@ -49,6 +49,7 @@ create_policies() { mc admin policy create minio conditions-policy-$TIMESTAMP portal-ui/tests/policies/conditionsPolicy.json mc admin policy create minio conditions-policy-2-$TIMESTAMP portal-ui/tests/policies/conditionsPolicy2.json mc admin policy create minio conditions-policy-3-$TIMESTAMP portal-ui/tests/policies/conditionsPolicy3.json + mc admin policy create minio conditions-policy-4-$TIMESTAMP portal-ui/tests/policies/conditionsPolicy4.json } create_users() { @@ -79,6 +80,7 @@ create_users() { mc admin user add minio conditions-$TIMESTAMP conditions1234 mc admin user add minio conditions-2-$TIMESTAMP conditions1234 mc admin user add minio conditions-3-$TIMESTAMP conditions1234 + mc admin user add minio conditions-4-$TIMESTAMP conditions1234 } create_buckets() { @@ -114,4 +116,5 @@ assign_policies() { mc admin policy attach minio conditions-policy-$TIMESTAMP --user conditions-$TIMESTAMP mc admin policy attach minio conditions-policy-2-$TIMESTAMP --user conditions-2-$TIMESTAMP mc admin policy attach minio conditions-policy-3-$TIMESTAMP --user conditions-3-$TIMESTAMP + mc admin policy attach minio conditions-policy-4-$TIMESTAMP --user conditions-4-$TIMESTAMP } \ No newline at end of file diff --git a/portal-ui/tests/scripts/permissions.sh b/portal-ui/tests/scripts/permissions.sh index 47655fdeb..5201d0304 100755 --- a/portal-ui/tests/scripts/permissions.sh +++ b/portal-ui/tests/scripts/permissions.sh @@ -39,6 +39,7 @@ remove_users() { mc admin user remove minio conditions-"$TIMESTAMP" mc admin user remove minio conditions-2-"$TIMESTAMP" mc admin user remove minio conditions-3-"$TIMESTAMP" + mc admin user remove minio conditions-4-"$TIMESTAMP" } remove_policies() { @@ -65,6 +66,7 @@ remove_policies() { mc admin policy remove conditions-policy-"$TIMESTAMP" mc admin policy remove conditions-policy-2-"$TIMESTAMP" mc admin policy remove conditions-policy-3-"$TIMESTAMP" + mc admin policy remove conditions-policy-4-"$TIMESTAMP" } remove_buckets() { diff --git a/portal-ui/tests/utils/roles.ts b/portal-ui/tests/utils/roles.ts index 42a56bcfd..20f7be472 100644 --- a/portal-ui/tests/utils/roles.ts +++ b/portal-ui/tests/utils/roles.ts @@ -283,3 +283,14 @@ export const conditions3 = Role( }, { preserveUrl: true }, ); + +export const conditions4 = Role( + loginUrl, + async (t) => { + await t + .typeText("#accessKey", "conditions-4-" + unixTimestamp) + .typeText("#secretKey", "conditions1234") + .click(submitButton); + }, + { preserveUrl: true }, +); diff --git a/restapi/admin_objects.go b/restapi/admin_objects.go index 059643f5c..7a20b847a 100644 --- a/restapi/admin_objects.go +++ b/restapi/admin_objects.go @@ -41,7 +41,7 @@ type ObjectsRequest struct { type WSResponse struct { RequestID int64 `json:"request_id,omitempty"` - Error string `json:"error,omitempty"` + Error *CodedAPIError `json:"error,omitempty"` RequestEnd bool `json:"request_end,omitempty"` Prefix string `json:"prefix,omitempty"` BucketName string `json:"bucketName,omitempty"` diff --git a/restapi/errors.go b/restapi/errors.go index 3f3c371b9..06f0a4c1d 100644 --- a/restapi/errors.go +++ b/restapi/errors.go @@ -108,6 +108,10 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { errorCode = 401 errorMessage = ErrInvalidLogin.Error() } + if strings.Contains(strings.ToLower(err1.Error()), ErrAccessDenied.Error()) { + errorCode = 403 + errorMessage = err1.Error() + } // If the last error is ErrInvalidLogin, this is a login failure if errors.Is(lastError, ErrInvalidLogin) { errorCode = 401 diff --git a/restapi/ws_objects.go b/restapi/ws_objects.go index e3d9629bc..ebe455c82 100644 --- a/restapi/ws_objects.go +++ b/restapi/ws_objects.go @@ -60,6 +60,7 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { err := json.Unmarshal(message, &messageRequest) if err != nil { LogInfo("Error on message request unmarshal") + close(done) return } @@ -95,6 +96,14 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { objectRqConfigs, err := getObjectsOptionsFromReq(messageRequest) if err != nil { LogInfo(fmt.Sprintf("Error during Objects OptionsParse %s", err.Error())) + + writeChannel <- WSResponse{ + RequestID: messageRequest.RequestID, + Error: ErrorWithContext(ctx, err), + Prefix: messageRequest.Prefix, + BucketName: messageRequest.BucketName, + } + return } var buffer []ObjectResponse @@ -105,7 +114,7 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { if lsObj.Err != nil { writeChannel <- WSResponse{ RequestID: messageRequest.RequestID, - Error: lsObj.Err.Error(), + Error: ErrorWithContext(ctx, lsObj.Err), Prefix: messageRequest.Prefix, BucketName: messageRequest.BucketName, } @@ -159,7 +168,13 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { objectRqConfigs, err := getObjectsOptionsFromReq(messageRequest) if err != nil { LogInfo(fmt.Sprintf("Error during Objects OptionsParse %s", err.Error())) - cancel() + writeChannel <- WSResponse{ + RequestID: messageRequest.RequestID, + Error: ErrorWithContext(ctx, err), + Prefix: messageRequest.Prefix, + BucketName: messageRequest.BucketName, + } + return } @@ -167,8 +182,13 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { s3Client, err := newS3BucketClient(session, objectRqConfigs.BucketName, objectRqConfigs.Prefix, clientIP) if err != nil { - LogError("error creating S3Client:", err) - close(done) + writeChannel <- WSResponse{ + RequestID: messageRequest.RequestID, + Error: ErrorWithContext(ctx, err), + Prefix: messageRequest.Prefix, + BucketName: messageRequest.BucketName, + } + cancel() return } @@ -181,7 +201,7 @@ func (wsc *wsMinioClient) objectManager(session *models.Principal) { if lsObj.Err != nil { writeChannel <- WSResponse{ RequestID: messageRequest.RequestID, - Error: lsObj.Err.String(), + Error: ErrorWithContext(ctx, lsObj.Err.ToGoError()), Prefix: messageRequest.Prefix, BucketName: messageRequest.BucketName, }