NOISSUE - Merge authz and authn into new service auth #1313

mteodor · 2020-12-22T15:41:58Z

What does this do?

New auth service will perform functions of authn service

issue token
identify
Additionally it will support groups features
assign member to a group ( member can be user or thing )
get group members

drasko · 2020-12-23T23:19:20Z

auth/README.md

+default values.
+
+| Variable                  | Description                                                              | Default       |
+|---------------------------|--------------------------------------------------------------------------|---------------|


drasko · 2020-12-23T23:19:29Z

auth/README.md

+| MF_AUTH_SERVER_CERT      | Path to server certificate in pem format                                |                |
+| MF_AUTH_SERVER_KEY       | Path to server key in pem format                                        |                |
+| MF_AUTH_SECRET           | String used for signing tokens                                          | auth           |
+| MF_JAEGER_URL             | Jaeger server URL                                                       | localhost:6831 |


dborovcanin · 2020-12-24T14:34:02Z

auth/README.md

@@ -0,0 +1,118 @@
+# Authentication service


This should be updated (it's Auth service, not Authentication service) with the authorization section (at least mention it before we implement all the features).

dborovcanin · 2020-12-24T14:49:32Z

auth/api/grpc/requests.go

+type AssignmentReq struct {
+	GroupID  string
+	MemberID string
+}
+
+func (req AssignmentReq) validate() error {
+	if req.GroupID == "" {
+		return auth.ErrMalformedEntity
+	}
+
+	if req.MemberID == "" {
+		return auth.ErrMalformedEntity
+	}
+
+	return nil
+}


Looks like this is not used. Please remove.

dborovcanin · 2020-12-24T14:51:04Z

auth/api/grpc/requests.go

+	Act string
+}
+
+func (req AuthZReq) validate() error {


This should not be exported. Request and response structs are internal to the API package.

dborovcanin · 2020-12-24T14:58:44Z

auth/api/grpc/server.go

+}
+
+func (s *grpcServer) Assign(ctx context.Context, token *mainflux.Assignment) (*empty.Empty, error) {
+	_, res, err := s.identify.ServeGRPC(ctx, token)


s.assign.ServeGRPC

dborovcanin · 2020-12-24T14:59:00Z

auth/api/grpc/server.go

+}
+
+func (s *grpcServer) Authorize(ctx context.Context, token *mainflux.AuthorizeReq) (*mainflux.AuthorizeRes, error) {
+	_, res, err := s.identify.ServeGRPC(ctx, token)


s.authorize.ServeGRPC

dborovcanin · 2020-12-24T14:59:27Z

auth/api/grpc/server.go

+}
+
+func (s *grpcServer) Members(ctx context.Context, token *mainflux.MembersReq) (*mainflux.MembersRes, error) {
+	_, res, err := s.identify.ServeGRPC(ctx, token)


s.members.ServeGRPC

dborovcanin · 2020-12-24T15:03:59Z

auth/api/http/transport.go

@@ -46,6 +47,83 @@ func MakeHandler(svc authn.Service, tracer opentracing.Tracer) http.Handler {
 		opts...,
 	))

+	mux.Get("/members/:memberID/groups", kithttp.NewServer(


Please reorder methods in CRUD order (where read = get one, list all, list members, list member of...).

auth/postgres/init.go

dborovcanin · 2020-12-24T15:16:17Z

auth/postgres/tracing.go

@@ -21,6 +21,12 @@ type database struct {
 type Database interface {
 	NamedExecContext(context.Context, string, interface{}) (sql.Result, error)
 	QueryRowxContext(context.Context, string, ...interface{}) *sqlx.Row
+	QueryxContext(context.Context, string, ...interface{}) (*sqlx.Rows, error)
+	// NamedExecContext(context.Context, string, interface{}) (sql.Result, error)


Please remove unused methods, both from the interface and from the implementation.

dborovcanin · 2020-12-24T15:17:23Z

authn.proto

@@ -13,9 +13,12 @@ service ThingsService {
    rpc Identify(Token) returns (ThingID) {}


This should be renamed to auth.proto.

drasko · 2020-12-24T14:36:44Z

auth/README.md

@@ -0,0 +1,118 @@
+# Authentication service


I would call this Authentication and Authorization Service.

Also - capitalize words in titles.

drasko · 2020-12-24T14:37:26Z

auth/README.md

@@ -0,0 +1,118 @@
+# Authentication service
+
+Authentication service provides an API for managing authentication keys. User service is using AuthN service gRPC API to obtain login token or password reset token. Authentication key consists of the following fields:


Use just Auth, to avoid using the word Authentication, because this service is both AuthN and AuthZ.

drasko · 2020-12-24T14:39:13Z

auth/README.md

+make install
+
+# set the environment variables and run the service
+MF_AUTH_LOG_LEVEL=[Service log level] MF_AUTH_DB_HOST=[Database host address] MF_AUTH_DB_PORT=[Database host port] MF_AUTH_DB_USER=[Database user] MF_AUTH_DB_PASS=[Database password] MF_AUTH_DB=[Name of the database used by the service] MF_AUTH_DB_SSL_MODE=[SSL mode to connect to the database with] MF_AUTH_DB_SSL_CERT=[Path to the PEM encoded certificate file] MF_AUTH_DB_SSL_KEY=[Path to the PEM encoded key file] MF_AUTH_DB_SSL_ROOT_CERT=[Path to the PEM encoded root certificate file] MF_AUTH_HTTP_PORT=[Service HTTP port] MF_AUTH_GRPC_PORT=[Service gRPC port] MF_AUTH_SECRET=[String used for signing tokens] MF_AUTH_SERVER_CERT=[Path to server certificate] MF_AUTH_SERVER_KEY=[Path to server key] MF_JAEGER_URL=[Jaeger server URL] $GOBIN/mainflux-authn


mainflux-auth, not mainflux-authn

drasko · 2020-12-24T19:44:52Z

auth/README.md

+locally:
+
+```yaml
+version: "2"


We use version 3.8 or something AFAIK. Please verify.

drasko · 2020-12-24T19:47:06Z

auth/api/grpc/client.go

+
+	res, err := client.authorize(ctx, AuthZReq{Act: req.Act, Obj: req.Obj, Sub: req.Sub})
+	if err != nil {
+		return &mainflux.AuthorizeRes{Authorized: false, Err: err.Error()}, err


Please align naming of AuthZReq{} and AuthorizeRes{}

drasko · 2020-12-24T19:49:05Z

auth/api/grpc/requests.go

+// 1. subject - an action invoker
+// 2. object - an entity over which action will be executed
+// 3. action - type of action that will be executed (read/write)
+type AuthZReq struct {


Consider aligning this name (you are were using something like AuthorizeRequest / Response). I am not against AuthZReq also, but then align other entities.

drasko · 2020-12-24T19:53:19Z

auth/service.go

+			Total:  p.Total,
+			Offset: p.Offset,
+			Limit:  p.Limit,
+			// Name:   things,


Remove dead code

Please remove dead code.

auth.proto

auth/README.md

dborovcanin · 2020-12-25T13:28:38Z

auth/api/grpc/responses.go

+	authorized bool
+	err        string
+}
+type membersRes struct {


Should this contain an err field, as well? Please put an empty line between type definitions (membersRes and emptyRes).

auth.proto

auth/service.go

dborovcanin · 2020-12-25T13:39:05Z

auth/service.go


 import (
 	"context"
 	"time"

 	"github.com/mainflux/mainflux"
+	"github.com/mainflux/mainflux/internal/groups"


Since Groups are now in the Auth service (we'll have generic groups), there is no need to put this in internal, since it won't be shared with other services. We can simply move it to auth package.

not sure about that since some endpoints may be in other services

Yes, some endpoints probably will be in other services, but we won't share them; we won't use the groups package as an internally shared package, but as a dependency from the auth service. That is, we won't have multiple implementations of the Groups.

blokovi · 2020-12-25T13:38:58Z

auth.proto

+
+message MembersRes {
+    uint64 total            = 1;
+	uint64 offset           = 2;


check indentation here, looks like it's messed up

blokovi · 2020-12-25T13:46:35Z

twins/README.md

-| MF_AUTHN_GRPC_URL          | AuthN service gRPC URL                                               | localhost:8181        |
-| MF_AUTHN_GRPC_TIMEOUT      | AuthN service gRPC request timeout in seconds                        | 1s                    |
+| MF_AUTH_GRPC_URL          | AuthN service gRPC URL                                               | localhost:8181        |
+| MF_AUTH_GRPC_TIMEOUT      | AuthN service gRPC request timeout in seconds                        | 1s                    |


Alignment of table is broken with new envars, just add one space

blokovi · 2020-12-25T13:48:58Z

things/README.md

@@ -44,8 +44,8 @@ default values.
 | MF_THINGS_SINGLE_USER_EMAIL | User email for single user mode (no gRPC communication with users)     |                |
 | MF_THINGS_SINGLE_USER_TOKEN | User token for single user mode that should be passed in auth header   |                |
 | MF_JAEGER_URL               | Jaeger server URL                                                      | localhost:6831 |
-| MF_AUTHN_GRPC_URL           | AuthN service gRPC URL                                                 | localhost:8181 |
-| MF_AUTHN_GRPC_TIMEOUT       | AuthN service gRPC request timeout in seconds                          | 1s              |
+| MF_AUTH_GRPC_URL           | AuthN service gRPC URL                                                 | localhost:8181 |


add one space in renamed envars because of table alingment

drasko · 2020-12-25T18:20:15Z

auth/postgres/init.go

+
+// Connect creates a connection to the PostgreSQL instance and applies any
+// unapplied database migrations. A non-nil error is returned to indicate
+// failure.


Move this failure word to upper line for easier reading.

auth/service.go

nmarcetic · 2020-12-27T19:00:27Z

auth/service.go

+
+	// Revoke removes the Key with the provided id that is
+	// issued by the user identified by the provided key.
+	RevokeKey(ctx context.Context, token, id string) error


Same question, is Key is necessary ?

nmarcetic · 2020-12-27T19:02:05Z

auth/service.go

+}
+
+func (svc service) Authorize(ctx context.Context, token, sub, obj, act string) (bool, error) {
+	return true, nil


Is this is a placeholder for impl ?

nmarcetic · 2020-12-27T19:11:53Z

auth/service.go

+// implementation, and all of its decorators (e.g. logging & metrics).
+// Token is a string value of the actual Key and is used to authenticate
+// an Auth service request.
+type Service interface {


Would embeding interfaces makes more sense ? So i can see the union of multiple interfaces (Groups and Auth) in Auth spec. IMHO it would be much more obvious.

internal/groups/groups.go

nmarcetic · 2020-12-27T19:35:00Z

auth/jwt/token_test.go

@@ -57,7 +57,7 @@ func TestParse(t *testing.T) {
 	require.Nil(t, err, fmt.Sprintf("issuing key expected to succeed: %s", err))

 	apiKey := key()
-	apiKey.Type = authn.APIKey
+	apiKey.Type = auth.APIKey
 	apiKey.ExpiresAt = time.Now().UTC().Add(-1 * time.Minute).Round(time.Second)
 	apiToken, err := tokenizer.Issue(apiKey)


if you will follow naming, then token makes more sense than tokenizer (interface name).

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko · 2020-12-29T11:01:13Z

auth/postgres/tracing.go

@@ -30,14 +32,24 @@ func NewDatabase(db *sqlx.DB) Database {
 	}
 }

-func (d database) NamedExecContext(ctx context.Context, query string, args interface{}) (sql.Result, error) {
+func (dm database) NamedQueryContext(ctx context.Context, query string, args interface{}) (*sqlx.Rows, error) {


Why was this changed to dm, and what m really represents here (a bit confusing)?

drasko · 2020-12-29T11:02:45Z

twins/mocks/auth.go

+	return &authNServiceClient{users}
+}
+
+func (svc authNServiceClient) Identify(ctx context.Context, in *mainflux.Token, opts ...grpc.CallOption) (*mainflux.UserIdentity, error) {


authN or auth?

codecov-io · 2020-12-29T13:56:14Z

Codecov Report

Merging #1313 (477c5d3) into master (b2ccbae) will decrease coverage by 3.94%.
The diff coverage is 38.02%.

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   64.91%   60.97%   -3.95%     
==========================================
  Files         117      110       -7     
  Lines        8029     8581     +552     
==========================================
+ Hits         5212     5232      +20     
- Misses       2361     2904     +543     
+ Partials      456      445      -11

Impacted Files	Coverage Δ
auth/api/http/responses.go	`100.00% <ø> (ø)`
auth/keys.go	`100.00% <ø> (ø)`
auth/postgres/groups.go	`0.00% <0.00%> (ø)`
auth/postgres/tracing.go	`75.00% <0.00%> (ø)`
bootstrap/service.go	`74.34% <ø> (ø)`
things/service.go	`54.54% <ø> (ø)`
things/users/users.go	`81.25% <0.00%> (-18.75%)`	⬇️
twins/service.go	`69.38% <ø> (ø)`
users/api/metrics.go	`0.00% <ø> (ø)`
auth/api/grpc/requests.go	`34.28% <34.28%> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2ccbae...477c5d3. Read the comment docs.

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko · 2020-12-29T18:18:05Z

writers/influxdb/messages_test.go

@@ -84,7 +84,7 @@ func TestSave(t *testing.T) {
 	}

 	for _, tc := range cases {
-		// Clean previously saved messages.
+		// // Clean previously saved messages.


No need for double //. Please remove.

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko · 2020-12-29T20:00:20Z

auth/api/grpc/endpoint.go

+			return issueRes{}, err
+		}
+
+		now := time.Now().UTC()


I do not think that we need this var - just call the fnc directly in the struct declaration below.

drasko · 2020-12-29T20:03:52Z

auth/api/metrics.go

+}
+
+// MetricsMiddleware instruments core service by tracking request count and
+// latency.


Move this word to upper line to simplify comment reading.

drasko · 2020-12-29T20:05:14Z

auth/service.go

+			Total:  p.Total,
+			Offset: p.Offset,
+			Limit:  p.Limit,
+			// Name:   things,


Please remove dead code.

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko

LGTM

mteodor requested a review from a team as a code owner December 22, 2020 15:41

mteodor marked this pull request as draft December 22, 2020 15:42

mteodor self-assigned this Dec 22, 2020

mteodor added the work-in-progress label Dec 22, 2020

mteodor force-pushed the auth branch from d10ac92 to 85e8ab7 Compare December 23, 2020 18:04

drasko requested changes Dec 23, 2020

View reviewed changes

mteodor marked this pull request as ready for review December 24, 2020 14:19

mteodor removed the work-in-progress label Dec 24, 2020

dborovcanin requested changes Dec 24, 2020

View reviewed changes

drasko requested changes Dec 24, 2020

View reviewed changes

dborovcanin requested changes Dec 25, 2020

View reviewed changes

blokovi reviewed Dec 25, 2020

View reviewed changes

drasko requested changes Dec 25, 2020

View reviewed changes

nmarcetic reviewed Dec 27, 2020

View reviewed changes

mteodor added 11 commits December 28, 2020 17:02

remove owner id

d915af4

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

move authz into authn and merge into new service

871da3a

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

add groups

c8945cb

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

add groups

3ccabfd

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

add groups endpoints

50fd0d4

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

add group type

cdff98f

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

adding mocks, some renaming, refactor

d02eca8

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

update proto

3274762

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

adding mocks, some renaming, refactor

fc493ad

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

fix linter err,and comments

74b4e80

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

undo renaming, add interface for authn and authz

06d85d7

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

mteodor force-pushed the auth branch from 534c842 to 06d85d7 Compare December 28, 2020 16:05

drasko requested changes Dec 29, 2020

View reviewed changes

mteodor added 2 commits December 29, 2020 16:36

renam some variables

237785f

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

renaming

781df63

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko requested changes Dec 29, 2020

View reviewed changes

remove extra slashes from comment

83e930d

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

drasko requested changes Dec 29, 2020

View reviewed changes

dborovcanin previously approved these changes Dec 29, 2020

View reviewed changes

resolving small remarks

477c5d3

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

mteodor dismissed dborovcanin’s stale review via 477c5d3 December 29, 2020 21:41

drasko approved these changes Dec 29, 2020

View reviewed changes

drasko changed the title ~~NOISSUE - merge authz and authn into new service auth~~ NOISSUE - Merge authz and authn into new service auth Dec 29, 2020

drasko merged commit 47217cb into absmach:master Dec 29, 2020

jonathandreyer mentioned this pull request Jan 26, 2021

Fix documentation with new Auth service in provision page absmach/magistrala-docs#68

Closed

mteodor deleted the auth branch September 13, 2021 12:28

		@@ -13,9 +13,12 @@ service ThingsService {
		rpc Identify(Token) returns (ThingID) {}

		@@ -0,0 +1,118 @@
		# Authentication service

		Authentication service provides an API for managing authentication keys. User service is using AuthN service gRPC API to obtain login token or password reset token. Authentication key consists of the following fields:

NOISSUE - Merge authz and authn into new service auth #1313

NOISSUE - Merge authz and authn into new service auth #1313

Conversation

mteodor commented Dec 22, 2020

What does this do?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 29, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 29, 2020 •

edited

Loading