Repository: mainflux/mainflux (go)

Issue:
Invalid handling of auth errors in Things service
**BUG REPORT**

1. What were you trying to achieve?
I wanted to make sure the policies mechanism added on `mainflux/0.13.0` didn't break anything on our project. So while checking the `Things Api` I could check some strange behavior when trying to fetch `Things` that doesn't exist or the `User` doesn't have access to them. My test was quite simple, I just created a random `User` and tried to fetch a `Thing ID` that doesn't exist.

3. What are the expected results?
`404 Not Found` same as `mainflux/0.12.1`

4. What are the received results?
`500 Internal Server Error`

5. What are the steps to reproduce the issue?
```sh
curl -s -X POST -H "Content-Type: application/json" http://localhost:8180/users -d '{"email":"hoge@mail.com","password":"12345678"}'
TOKEN=$(curl -s -X POST -H "Content-Type: application/json" http://localhost:8180/tokens -d '{"email":"hoge@mail.com","password":"12345678"}' | jq -r ".token")
curl -s -X GET -H "Content-Type: application/json" -H "Authorization: Bearer $TOKEN" http://localhost:8182/things/123
```

6. In what environment did you encounter the issue?
`mainflux/0.13.0`

7. Additional information you deem important:
Same thing happens when I have a `Thing` but I don't have the `read` policies on `Keto` to access it. It also throws a `500 Internal Server Error` instead of `403 Forbidden` as the [documentation](https://docs.mainflux.io/authorization/) states.


Pull request:
MF-1614 - Invalid handling of auth errors in Things service
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

### What does this do?
This pull request fixes error handling in the Things service in case of errors received during authentication or authorization.

### Which issue(s) does this PR fix/relate to?
This pull request fixes #1614.

### List any changes that modify/break current functionality
There are changes in API that now match documentation.

### Have you included tests for your changes?
Yes, tests are modified to match the service behavior.

### Did you document any new/modified functionality?
Yes.

### Notes
N/A

Patch:
diff --git a/things/api/things/http/transport.go b/things/api/things/http/transport.go
index fdc911a0..e359e2fe 100644
--- a/things/api/things/http/transport.go
+++ b/things/api/things/http/transport.go
@@ -501,6 +501,9 @@ func encodeResponse(_ context.Context, w http.ResponseWriter, response interface
 
 func encodeError(_ context.Context, err error, w http.ResponseWriter) {
 	switch {
+	// ErrNotFound can be masked by ErrAuthentication, but it has priority.
+	case errors.Contains(err, errors.ErrNotFound):
+		w.WriteHeader(http.StatusNotFound)
 	case errors.Contains(err, errors.ErrAuthentication),
 		err == apiutil.ErrBearerToken:
 		w.WriteHeader(http.StatusUnauthorized)
@@ -520,8 +523,6 @@ func encodeError(_ context.Context, err error, w http.ResponseWriter) {
 		err == apiutil.ErrInvalidDirection,
 		err == apiutil.ErrInvalidIDFormat:
 		w.WriteHeader(http.StatusBadRequest)
-	case errors.Contains(err, errors.ErrNotFound):
-		w.WriteHeader(http.StatusNotFound)
 	case errors.Contains(err, errors.ErrConflict):
 		w.WriteHeader(http.StatusConflict)
 	case errors.Contains(err, errors.ErrScanMetadata):
diff --git a/things/service.go b/things/service.go
index dc4cb65b..07082620 100644
--- a/things/service.go
+++ b/things/service.go
@@ -171,7 +171,6 @@ func (ts *thingsService) CreateThings(ctx context.Context, token string, things
 
 // createThing saves the Thing and adds identity as an owner(Read, Write, Delete policies) of the Thing.
 func (ts *thingsService) createThing(ctx context.Context, thing *Thing, identity *mainflux.UserIdentity) (Thing, error) {
-
 	thing.Owner = identity.GetEmail()
 
 	if thing.ID == "" {
@@ -258,12 +257,12 @@ func (ts *thingsService) claimOwnership(ctx context.Context, objectID string, ac
 func (ts *thingsService) UpdateKey(ctx context.Context, token, id, key string) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), id, writeRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return err
+			return errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -275,12 +274,12 @@ func (ts *thingsService) UpdateKey(ctx context.Context, token, id, key string) e
 func (ts *thingsService) ViewThing(ctx context.Context, token, id string) (Thing, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return Thing{}, err
+		return Thing{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), id, readRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return Thing{}, err
+			return Thing{}, errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -290,7 +289,7 @@ func (ts *thingsService) ViewThing(ctx context.Context, token, id string) (Thing
 func (ts *thingsService) ListThings(ctx context.Context, token string, pm PageMetadata) (Page, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return Page{}, err
+		return Page{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	subject := res.GetId()
@@ -333,7 +332,7 @@ func (ts *thingsService) ListThings(ctx context.Context, token string, pm PageMe
 func (ts *thingsService) ListThingsByChannel(ctx context.Context, token, chID string, pm PageMetadata) (Page, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return Page{}, err
+		return Page{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	return ts.things.RetrieveByChannel(ctx, res.GetEmail(), chID, pm)
@@ -342,12 +341,13 @@ func (ts *thingsService) ListThingsByChannel(ctx context.Context, token, chID st
 func (ts *thingsService) RemoveThing(ctx context.Context, token, id string) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
+
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), id, deleteRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return err
+			return errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -360,7 +360,7 @@ func (ts *thingsService) RemoveThing(ctx context.Context, token, id string) erro
 func (ts *thingsService) CreateChannels(ctx context.Context, token string, channels ...Channel) ([]Channel, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return []Channel{}, err
+		return []Channel{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	chs := []Channel{}
@@ -402,12 +402,12 @@ func (ts *thingsService) createChannel(ctx context.Context, channel *Channel, id
 func (ts *thingsService) UpdateChannel(ctx context.Context, token string, channel Channel) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), channel.ID, writeRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return err
+			return errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -418,12 +418,12 @@ func (ts *thingsService) UpdateChannel(ctx context.Context, token string, channe
 func (ts *thingsService) ViewChannel(ctx context.Context, token, id string) (Channel, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return Channel{}, err
+		return Channel{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), id, readRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return Channel{}, err
+			return Channel{}, errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -433,7 +433,7 @@ func (ts *thingsService) ViewChannel(ctx context.Context, token, id string) (Cha
 func (ts *thingsService) ListChannels(ctx context.Context, token string, pm PageMetadata) (ChannelsPage, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return ChannelsPage{}, err
+		return ChannelsPage{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	// If the user is admin, fetch all channels from the database.
@@ -453,7 +453,7 @@ func (ts *thingsService) ListChannels(ctx context.Context, token string, pm Page
 func (ts *thingsService) ListChannelsByThing(ctx context.Context, token, thID string, pm PageMetadata) (ChannelsPage, error) {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return ChannelsPage{}, err
+		return ChannelsPage{}, errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	return ts.channels.RetrieveByThing(ctx, res.GetEmail(), thID, pm)
@@ -462,12 +462,12 @@ func (ts *thingsService) ListChannelsByThing(ctx context.Context, token, thID st
 func (ts *thingsService) RemoveChannel(ctx context.Context, token, id string) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	if err := ts.authorize(ctx, res.GetId(), id, deleteRelationKey); err != nil {
 		if err := ts.authorize(ctx, res.GetId(), authoritiesObject, memberRelationKey); err != nil {
-			return err
+			return errors.Wrap(errors.ErrNotFound, err)
 		}
 	}
 
@@ -481,7 +481,7 @@ func (ts *thingsService) RemoveChannel(ctx context.Context, token, id string) er
 func (ts *thingsService) Connect(ctx context.Context, token string, chIDs, thIDs []string) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	return ts.channels.Connect(ctx, res.GetEmail(), chIDs, thIDs)
@@ -490,7 +490,7 @@ func (ts *thingsService) Connect(ctx context.Context, token string, chIDs, thIDs
 func (ts *thingsService) Disconnect(ctx context.Context, token string, chIDs, thIDs []string) error {
 	res, err := ts.auth.Identify(ctx, &mainflux.Token{Value: token})
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthentication, err)
 	}
 
 	for _, chID := range chIDs {
@@ -612,10 +612,10 @@ func (ts *thingsService) authorize(ctx context.Context, subject, object string,
 	}
 	res, err := ts.auth.Authorize(ctx, req)
 	if err != nil {
-		return err
+		return errors.Wrap(errors.ErrAuthorization, err)
 	}
 	if !res.GetAuthorized() {
-		return err
+		return errors.ErrAuthorization
 	}
 	return nil
 }


Test patch:
diff --git a/pkg/sdk/go/things_test.go b/pkg/sdk/go/things_test.go
index d4c955d2..dc93a0c7 100644
--- a/pkg/sdk/go/things_test.go
+++ b/pkg/sdk/go/things_test.go
@@ -238,7 +238,7 @@ func TestThing(t *testing.T) {
 			desc:     "get non-existent thing",
 			thID:     "43",
 			token:    token,
-			err:      createError(sdk.ErrFailedFetch, http.StatusForbidden),
+			err:      createError(sdk.ErrFailedFetch, http.StatusNotFound),
 			response: sdk.Thing{},
 		},
 		{
@@ -252,7 +252,6 @@ func TestThing(t *testing.T) {
 
 	for _, tc := range cases {
 		respTh, err := mainfluxSDK.Thing(tc.thID, tc.token)
-
 		assert.Equal(t, tc.err, err, fmt.Sprintf("%s: expected error %s, got %s", tc.desc, tc.err, err))
 		assert.Equal(t, tc.response, respTh, fmt.Sprintf("%s: expected response thing %s, got %s", tc.desc, tc.response, respTh))
 	}
@@ -592,7 +591,7 @@ func TestDeleteThing(t *testing.T) {
 			desc:    "delete non-existing thing",
 			thingID: "2",
 			token:   token,
-			err:     createError(sdk.ErrFailedRemoval, http.StatusForbidden),
+			err:     createError(sdk.ErrFailedRemoval, http.StatusNotFound),
 		},
 		{
 			desc:    "delete thing with invalid id",
diff --git a/things/api/things/http/endpoint_test.go b/things/api/things/http/endpoint_test.go
index 686bc27c..5e0f05b9 100644
--- a/things/api/things/http/endpoint_test.go
+++ b/things/api/things/http/endpoint_test.go
@@ -53,7 +53,6 @@ var (
 	}
 	invalidName    = strings.Repeat("m", maxNameSize+1)
 	notFoundRes    = toJSON(apiutil.ErrorRes{Err: errors.ErrNotFound.Error()})
-	unauthzRes     = toJSON(apiutil.ErrorRes{Err: errors.ErrAuthorization.Error()})
 	unauthRes      = toJSON(apiutil.ErrorRes{Err: errors.ErrAuthentication.Error()})
 	missingTokRes  = toJSON(apiutil.ErrorRes{Err: apiutil.ErrBearerToken.Error()})
 	searchThingReq = things.PageMetadata{
@@ -621,7 +620,7 @@ func TestUpdateKey(t *testing.T) {
 			id:          strconv.FormatUint(wrongID, 10),
 			contentType: contentType,
 			auth:        token,
-			status:      http.StatusForbidden,
+			status:      http.StatusNotFound,
 		},
 		{
 			desc:        "update thing with invalid id",
@@ -629,7 +628,7 @@ func TestUpdateKey(t *testing.T) {
 			id:          "invalid",
 			contentType: contentType,
 			auth:        token,
-			status:      http.StatusForbidden,
+			status:      http.StatusNotFound,
 		},
 		{
 			desc:        "update thing with invalid user token",
@@ -722,8 +721,8 @@ func TestViewThing(t *testing.T) {
 			desc:   "view non-existent thing",
 			id:     strconv.FormatUint(wrongID, 10),
 			auth:   token,
-			status: http.StatusForbidden,
-			res:    unauthzRes,
+			status: http.StatusNotFound,
+			res:    notFoundRes,
 		},
 		{
 			desc:   "view thing by passing invalid token",
@@ -743,8 +742,8 @@ func TestViewThing(t *testing.T) {
 			desc:   "view thing by passing invalid id",
 			id:     "invalid",
 			auth:   token,
-			status: http.StatusForbidden,
-			res:    unauthzRes,
+			status: http.StatusNotFound,
+			res:    notFoundRes,
 		},
 	}
 
@@ -1377,7 +1376,7 @@ func TestRemoveThing(t *testing.T) {
 			desc:   "delete non-existent thing",
 			id:     strconv.FormatUint(wrongID, 10),
 			auth:   token,
-			status: http.StatusForbidden,
+			status: http.StatusNotFound,
 		},
 		{
 			desc:   "delete thing with invalid token",
diff --git a/things/service_test.go b/things/service_test.go
index 0c5993ab..94cdbb24 100644
--- a/things/service_test.go
+++ b/things/service_test.go
@@ -263,7 +263,7 @@ func TestViewThing(t *testing.T) {
 		"view non-existing thing": {
 			id:    wrongID,
 			token: token,
-			err:   errors.ErrAuthorization,
+			err:   errors.ErrNotFound,
 		},
 	}