NOISSUE - Handle authorization errors other than unauthorize (#264)

api.go

@@ -1,6 +1,6 @@
 package mainflux
 
-// Response contains HTTP response specifig methods.
+// Response contains HTTP response specific methods.
 type Response interface {
 	// Code returns HTTP response code.
 	Code() int

clients/api/grpc/endpoint_test.go

@@ -24,10 +24,8 @@ const (
 )
 
 var (
-	client                = clients.Client{Type: "app", Name: "test_app", Payload: "test_payload"}
-	channel               = clients.Channel{Name: "test"}
-	errMalformedReq       = status.Error(codes.InvalidArgument, "received invalid can access request")
-	errUnauthorizedAccess = status.Error(codes.PermissionDenied, "failed to identify client or client isn't connected to specified channel")
+	client  = clients.Client{Type: "app", Name: "test_app", Payload: "test_payload"}
+	channel = clients.Channel{Name: "test"}
 )
 
 func newService(tokens map[string]string) clients.Service {
@@ -70,16 +68,19 @@ func TestCanAccess(t *testing.T) {
 		clientKey string
 		chanID    string
 		id        string
-		err       error
+		code      codes.Code
 	}{
-		"check if connected client can access existing channel":     {connectedClient.Key, chanID, connectedClientID, nil},
-		"check if unconnected client can access existing channel":   {client.Key, chanID, "", errUnauthorizedAccess},
-		"check if connected client can access non-existent channel": {connectedClient.Key, "1", "", errMalformedReq},
+		"check if connected client can access existing channel":     {connectedClient.Key, chanID, connectedClientID, codes.OK},
+		"check if unconnected client can access existing channel":   {client.Key, chanID, "", codes.PermissionDenied},
+		"check if wrong client can access existing channel":         {mocks.UnauthorizedToken, chanID, "", codes.Unauthenticated},
+		"check if connected client can access non-existent channel": {connectedClient.Key, "1", "", codes.InvalidArgument},
 	}
 
 	for desc, tc := range cases {
-		id, err := cli.CanAccess(ctx, &mainflux.AccessReq{Token: tc.clientKey, ChanID: tc.chanID})
+		id, err := cli.CanAccess(ctx, &mainflux.AccessReq{tc.clientKey, tc.chanID})
+		e, ok := status.FromError(err)
+		assert.True(t, ok, "OK expected to be true")
 		assert.Equal(t, tc.id, id.GetValue(), fmt.Sprintf("%s: expected %s got %s", desc, tc.id, id.GetValue()))
-		assert.Equal(t, tc.err, err, fmt.Sprintf("%s: expected %s got %s", desc, tc.err, err))
+		assert.Equal(t, tc.code, e.Code(), fmt.Sprintf("%s: expected %s got %s", desc, tc.code, e.Code()))
 	}
 }

clients/api/grpc/server.go

@@ -52,7 +52,9 @@ func encodeError(err error) error {
 	case clients.ErrMalformedEntity:
 		return status.Error(codes.InvalidArgument, "received invalid can access request")
 	case clients.ErrUnauthorizedAccess:
-		return status.Error(codes.PermissionDenied, "failed to identify client or client isn't connected to specified channel")
+		return status.Error(codes.Unauthenticated, "failed to identify client")
+	case clients.ErrAccessForbidden:
+		return status.Error(codes.PermissionDenied, "client isn't connected to specified channel")
 	default:
 		return status.Error(codes.Internal, "internal server error")
 	}

clients/mocks/security.go

@@ -1,12 +1,17 @@
 package mocks
 
-import "github.com/mainflux/mainflux/clients"
+import (
+	"github.com/mainflux/mainflux/clients"
+)
 
 var (
 	_ clients.Hasher           = (*hasherMock)(nil)
 	_ clients.IdentityProvider = (*identityProviderMock)(nil)
 )
 
+// UnauthorizedToken is used to mock Identity method failure.
+const UnauthorizedToken = "bd2a557f-27e6-4377-9e40-3c75f3f5211f"
+
 type hasherMock struct{}
 
 func (hm *hasherMock) Hash(pwd string) (string, error) {
@@ -24,7 +29,7 @@ func (hm *hasherMock) Compare(plain, hashed string) error {
 type identityProviderMock struct{}
 
 func (idp *identityProviderMock) TemporaryKey(id string) (string, error) {
-	if id == "" {
+	if id == "" || id == UnauthorizedToken {
 		return "", clients.ErrUnauthorizedAccess
 	}
 

clients/service.go

@@ -21,6 +21,10 @@ var (
 	// when accessing a protected resource.
 	ErrUnauthorizedAccess = errors.New("missing or invalid credentials provided")
 
+	// ErrAccessForbidden indicates valid credentials provided but
+	// accessing a protected resource is not able due to permission issue.
+	ErrAccessForbidden = errors.New("access forbidden")
+
 	// ErrNotFound indicates a non-existent entity request.
 	ErrNotFound = errors.New("non-existent entity")
 )
@@ -258,7 +262,7 @@ func (ms *clientsService) CanAccess(key, channel string) (string, error) {
 	}
 
 	if !ms.channels.HasClient(channel, client) {
-		return "", ErrUnauthorizedAccess
+		return "", ErrAccessForbidden
 	}
 
 	return client, nil

clients/service_test.go

@@ -331,8 +331,8 @@ func TestCanAccess(t *testing.T) {
 		err     error
 	}{
 		"allowed access":              {client.Key, channel.ID, nil},
-		"not-connected cannot access": {wrong, channel.ID, clients.ErrUnauthorizedAccess},
-		"access non-existing channel": {client.Key, wrong, clients.ErrUnauthorizedAccess},
+		"not-connected cannot access": {"", channel.ID, clients.ErrUnauthorizedAccess},
+		"access non-existing channel": {client.Key, wrong, clients.ErrAccessForbidden},
 	}
 
 	for desc, tc := range cases {

coap/README.md

@@ -13,7 +13,7 @@ default values.
 |-----------------------|------------------------|-------------------------|
 | MF_COAP_ADAPTER_PORT  | adapter listening port | `5683`                  |
 | MF_NATS_URL           | NATS instance URL      | `nats://localhost:4222` |
-| MF_MANAGER_URL        | manager service URL    | `http://localhost:8180` |
+| MF_CLIENTS_URL        | clients service URL    | `http://localhost:8181` |
 
 ## Deployment
 
@@ -29,9 +29,9 @@ services:
     ports:
       - [host machine port]:[configured port]
     environment:
-      MF_MANAGER_URL: [Manager service URL]
-      MF_NATS_URL: [NATS instance URL]
       MF_COAP_ADAPTER_PORT: [Service HTTP port]
+      MF_NATS_URL: [NATS instance URL]
+      MF_CLIENTS_URL: [Manager service URL]
 ```
 
 Running this service outside of container requires working instance of the NATS service.

coap/api/server.go

@@ -7,11 +7,14 @@ import (
 	"strings"
 	"time"
 
+	"google.golang.org/grpc/codes"
+
+	"github.com/asaskevich/govalidator"
 	mux "github.com/dereulenspiegel/coap-mux"
+	gocoap "github.com/dustin/go-coap"
 	"github.com/mainflux/mainflux"
 	"github.com/mainflux/mainflux/coap"
-
-	gocoap "github.com/dustin/go-coap"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -32,6 +35,11 @@ func authKey(opt interface{}) (string, error) {
 }
 
 func authorize(msg *gocoap.Message, res *gocoap.Message, cid string) (publisher *mainflux.Identity, err error) {
+	if !govalidator.IsUUID(cid) {
+		res.Code = gocoap.NotFound
+		return
+	}
+
 	// Device Key is passed as Uri-Query parameter, which option ID is 15 (0xf).
 	key, err := authKey(msg.Option(gocoap.URIQuery))
 	if err != nil {
@@ -50,7 +58,19 @@ func authorize(msg *gocoap.Message, res *gocoap.Message, cid string) (publisher
 	publisher, err = auth.CanAccess(ctx, &mainflux.AccessReq{Token: key, ChanID: cid})
 
 	if err != nil {
-		res.Code = gocoap.Unauthorized
+		e, ok := status.FromError(err)
+		if ok {
+			switch e.Code() {
+			case codes.Unauthenticated:
+				res.Code = gocoap.Unauthorized
+			case codes.PermissionDenied:
+				res.Code = gocoap.Forbidden
+			default:
+				res.Code = gocoap.ServiceUnavailable
+			}
+			return
+		}
+		res.Code = gocoap.InternalServerError
 	}
 	return
 }
@@ -78,7 +98,6 @@ func serve(svc coap.Service, conn *net.UDPConn, data []byte, addr *net.UDPAddr,
 		res.Type = gocoap.Acknowledgement
 		publisher, err := authorize(&msg, res, cid)
 		if err != nil {
-			res.Code = gocoap.Unauthorized
 			break
 		}
 		id := fmt.Sprintf("%s-%x", publisher, msg.Token)
@@ -89,7 +108,6 @@ func serve(svc coap.Service, conn *net.UDPConn, data []byte, addr *net.UDPAddr,
 		res.Type = gocoap.Acknowledgement
 		publisher, err := authorize(&msg, res, cid)
 		if err != nil {
-			res.Code = gocoap.Unauthorized
 			break
 		}
 		id := fmt.Sprintf("%s-%x", publisher, msg.Token)

coap/api/transport.go

@@ -79,7 +79,6 @@ func receive(svc coap.Service) handler {
 		cid := mux.Var(msg, "id")
 		publisher, err := authorize(msg, res, cid)
 		if err != nil {
-			res.Code = gocoap.Unauthorized
 			return res
 		}
 
@@ -115,7 +114,6 @@ func observe(svc coap.Service) handler {
 		publisher, err := authorize(msg, res, cid)
 
 		if err != nil {
-			res.Code = gocoap.Unauthorized
 			return res
 		}
 
@@ -157,24 +155,25 @@ func sendMessage(svc coap.Service, id string, conn *net.UDPConn, addr *net.UDPAd
 		if err != nil {
 			return err
 		}
-		return sendConfirmable(conn, addr, msg, ch)
+		go sendConfirmable(conn, addr, msg, ch)
+		return nil
 	}
 	return gocoap.Transmit(conn, addr, *msg)
 }
 
-func sendConfirmable(conn *net.UDPConn, addr *net.UDPAddr, msg *gocoap.Message, ch chan bool) error {
+func sendConfirmable(conn *net.UDPConn, addr *net.UDPAddr, msg *gocoap.Message, ch chan bool) {
 	msg.SetOption(gocoap.MaxRetransmit, coap.MaxRetransmit)
 	// Try to transmit MAX_RETRANSMITION times; every attempt duplicates timeout between transmission.
 	for i := 0; i < coap.MaxRetransmit; i++ {
 		if err := gocoap.Transmit(conn, addr, *msg); err != nil {
-			return err
+			return
 		}
 		state, ok := <-ch
 		if !state || !ok {
-			return nil
+			return
 		}
 	}
-	return nil
+	return
 }
 
 func handleSub(svc coap.Service, id string, conn *net.UDPConn, addr *net.UDPAddr, msg *gocoap.Message, ch nats.Channel) {
@@ -191,26 +190,27 @@ func handleSub(svc coap.Service, id string, conn *net.UDPConn, addr *net.UDPAddr
 	res.SetOption(gocoap.ContentFormat, gocoap.AppJSON)
 	res.SetOption(gocoap.LocationPath, msg.Path())
 
-loop:
 	for {
 		select {
 		case <-ticker.C:
+			ticker.Stop()
 			res.Type = gocoap.Confirmable
 			rand.Seed(time.Now().UnixNano())
 			if err := sendMessage(svc, id, conn, addr, res); err != nil {
-				svc.Unsubscribe(id)
-				break loop
+				ticker.Stop()
+				return
 			}
 		case rawMsg, ok := <-ch.Messages:
 			if !ok {
-				break loop
+				ticker.Stop()
+				return
 			}
 			res.Type = gocoap.NonConfirmable
 			res.Payload = rawMsg.Payload
 			if err := sendMessage(svc, id, conn, addr, res); err != nil {
-				break loop
+				ticker.Stop()
+				return
 			}
 		}
 	}
-	ticker.Stop()
 }

http/api/endpoint_test.go

@@ -75,7 +75,7 @@ func TestPublish(t *testing.T) {
 		status      int
 	}{
 		"publish message":                                  {id, msg, "application/senml+json", token, http.StatusAccepted},
-		"publish message with no authorization token":      {id, msg, "application/senml+json", "", http.StatusForbidden},
+		"publish message with no authorization token":      {id, msg, "application/senml+json", "", http.StatusUnauthorized},
 		"publish message with invalid authorization token": {id, msg, "application/senml+json", invalidToken, http.StatusForbidden},
 		"publish message with no content type":             {id, msg, "", token, http.StatusAccepted},
 		"publish message with invalid channel id":          {"1", msg, "application/senml+json", token, http.StatusNotFound},

http/api/transport.go

@@ -3,11 +3,9 @@ package api
 import (
 	"context"
 	"errors"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
-	"strings"
 	"time"
 
 	"github.com/asaskevich/govalidator"
@@ -16,6 +14,8 @@ import (
 	"github.com/mainflux/mainflux"
 	"github.com/mainflux/mainflux/clients"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const protocol string = "http"
@@ -79,7 +79,7 @@ func authorize(r *http.Request) (string, error) {
 	}
 
 	// extract ID from /channels/:id/messages
-	c := strings.Split(r.URL.Path, "/")[2]
+	c := bone.GetValue(r, "id")
 	if !govalidator.IsUUID(c) {
 		return "", errNotFound
 	}
@@ -117,9 +117,23 @@ func encodeError(_ context.Context, err error, w http.ResponseWriter) {
 	case errNotFound:
 		w.WriteHeader(http.StatusNotFound)
 	case clients.ErrUnauthorizedAccess:
+		w.WriteHeader(http.StatusUnauthorized)
+	case clients.ErrAccessForbidden:
 		w.WriteHeader(http.StatusForbidden)
+	case errNotFound:
+		w.WriteHeader(http.StatusNotFound)
 	default:
-		fmt.Println(err)
+		if e, ok := status.FromError(err); ok {
+			switch e.Code() {
+			case codes.Unauthenticated:
+				w.WriteHeader(http.StatusUnauthorized)
+			case codes.PermissionDenied:
+				w.WriteHeader(http.StatusForbidden)
+			default:
+				w.WriteHeader(http.StatusServiceUnavailable)
+			}
+			return
+		}
 		w.WriteHeader(http.StatusInternalServerError)
 	}
 }

http/mocks/clients.go

@@ -27,7 +27,7 @@ func (client clientsClient) CanAccess(ctx context.Context, req *mainflux.AccessR
 
 	id, ok := client.clients[key]
 	if !ok {
-		return nil, clients.ErrUnauthorizedAccess
+		return nil, clients.ErrAccessForbidden
 	}
 
 	return &mainflux.Identity{Value: id}, nil

ws/api/transport.go

@@ -15,6 +15,8 @@ import (
 	log "github.com/mainflux/mainflux/logger"
 	"github.com/mainflux/mainflux/ws"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const protocol = "ws"
@@ -55,9 +57,28 @@ func handshake(svc ws.Service) http.HandlerFunc {
 			return
 		}
 		if err != nil {
-			logger.Warn(fmt.Sprintf("Failed to authorize: %s", err))
-			w.WriteHeader(http.StatusForbidden)
-			return
+			switch err {
+			case errNotFound:
+				logger.Warn(fmt.Sprintf("Invalid channel id: %s", err))
+				w.WriteHeader(http.StatusNotFound)
+				return
+			default:
+				logger.Warn(fmt.Sprintf("Failed to authorize: %s", err))
+				e, ok := status.FromError(err)
+				if ok {
+					switch e.Code() {
+					case codes.Unauthenticated:
+						w.WriteHeader(http.StatusUnauthorized)
+					case codes.PermissionDenied:
+						w.WriteHeader(http.StatusForbidden)
+					default:
+						w.WriteHeader(http.StatusServiceUnavailable)
+					}
+					return
+				}
+				w.WriteHeader(http.StatusForbidden)
+				return
+			}
 		}
 
 		// Create new ws connection.
@@ -104,7 +125,7 @@ func authorize(r *http.Request) (subscription, error) {
 
 	id, err := auth.CanAccess(ctx, &mainflux.AccessReq{Token: authKey, ChanID: chanID})
 	if err != nil {
-		return subscription{}, clients.ErrUnauthorizedAccess
+		return subscription{}, err
 	}
 
 	sub := subscription{