NOISSUE - Add user groups #1228

mteodor · 2020-08-21T11:24:47Z

What does this do?

Introducing groups for user, this is related to https://github.com/mainflux/mainflux/issues/649

List any changes that modify/break current functionality

None

Have you included tests for your changes?

Yes

manuio · 2020-09-07T15:30:09Z

@mteodor CI is failing. Please, use this issue as reference: https://github.com/mainflux/mainflux/issues/649

drasko · 2020-09-07T23:02:19Z

cli/groups.go

+
+var cmdGroups = []cobra.Command{
+	cobra.Command{
+		Use:   "create",


What is the full command? group create ...? Would it not be confusing when we add other entities groups (things, channels)?

drasko · 2020-09-07T23:05:06Z

cmd/users/main.go

+	groupName := c.adminGroup
+
+	_, err := userRepo.RetrieveByEmail(context.Background(), user.Email, false)
+	if err == nil {


RetrieveByEmail actually fails when there is no user? I did not know that, I though it returns empty result...

Also, I see below that you are using users.ErrNotFound, so please be consistent.

Also, inline the check

drasko · 2020-09-07T23:06:31Z

pkg/sdk/go/channels_test.go

@@ -28,6 +28,7 @@ func TestCreateChannel(t *testing.T) {
 	sdkConf := sdk.Config{
 		BaseURL:           ts.URL,
 		UsersPrefix:       "",
+		GroupsPrefix:      "",


Group of what? Users? What about Things?

drasko · 2020-09-07T23:07:30Z

users/README.md

@@ -31,6 +31,8 @@ default values.
 | MF_USERS_HTTP_PORT        | Users service HTTP port                                                 | 8180           |
 | MF_USERS_SERVER_CERT      | Path to server certificate in pem format                                |                |
 | MF_USERS_SERVER_KEY       | Path to server key in pem format                                        |                |
+| MF_USERS_ADMIN_EMAIL   | Default user, created on startup                                        |                |


Please align text nicely

manuio · 2020-09-07T23:24:31Z

cmd/users/main.go

@@ -292,10 +309,60 @@ func newService(db *sqlx.DB, tracer opentracing.Tracer, auth mainflux.AuthNServi
 			Help:      "Total duration of requests in microseconds.",
 		}, []string{"method"}),
 	)
-
+	if err := createAdmin(svc, userRepo, groupRepo, c); err != nil {
+		logger.Error("failed to create admin user:" + err.Error())


Maybe you can use Wrap here. Otherwise add a white space after :

nmarcetic · 2020-09-09T09:17:10Z

users/service.go

+	return svc.groups.Save(ctx, group)
+}
+
+func (svc usersService) ListGroups(ctx context.Context, token string, offset, limit uint64, name string, meta Metadata) (GroupPage, error) {


Who can assign user to the group?
Who can remove the user from the group?
Who can delete a group?

I saw you have owner checking only in GET. What's the idea for other operations?

access rights will be introduced with policies

That makes sense. I was confused with the owner checking in RetrieveAll.
Maybe policies should handle this also, I mean if you have read in policies that mean that I can retrieve all groups where I am assigned. Not sure how this should work together, the thing to think about :)

nmarcetic · 2020-09-09T10:04:58Z

users/postgres/users.go

 	if _, err := ur.db.NamedExecContext(ctx, q, dbu); err != nil {
 		return errors.Wrap(errUpdateUserDB, err)
 	}

 	return nil
 }

-func (ur userRepository) RetrieveByEmail(ctx context.Context, email string) (users.User, error) {
+func (ur userRepository) RetrieveByEmail(ctx context.Context, email string, groups bool) (users.User, error) {


Why this group retrieves? Do we need this?

Probably not, that was my initial idea to have a flag whether you want to retrieve all groups that user belongs along
but I made a methods RetrieveAllGroupsForUser and RetrieveAllUsersForGroup

I see. Yes, I also think that we don't need it now, its better to decouple.

nmarcetic · 2020-09-09T10:09:06Z

users/service.go

+	return svc.groups.Save(ctx, group)
+}
+
+func (svc usersService) ListGroups(ctx context.Context, token string, offset, limit uint64, name string, meta Metadata) (GroupPage, error) {


That makes sense. I was confused with the owner checking in RetrieveAll.
Maybe policies should handle this also, I mean if you have read in policies that mean that I can retrieve all groups where I am assigned. Not sure how this should work together, the thing to think about :)

manuio · 2020-09-10T17:51:11Z

cmd/users/main.go

+	groupName := c.adminGroup
+
+	_, err := userRepo.RetrieveByEmail(context.Background(), user.Email, false)
+	if err == nil {


Also, inline the check

codecov-commenter · 2020-09-12T12:41:03Z

Codecov Report

Merging #1228 into master will decrease coverage by 6.32%.
The diff coverage is 30.13%.

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
- Coverage   74.36%   68.03%   -6.33%     
==========================================
  Files         106      108       +2     
  Lines        6159     7136     +977     
==========================================
+ Hits         4580     4855     +275     
- Misses       1194     1850     +656     
- Partials      385      431      +46

Impacted Files	Coverage Δ
pkg/sdk/go/groups.go	`0.00% <0.00%> (ø)`
things/api/auth/http/requests.go	`100.00% <ø> (ø)`
users/api/logging.go	`0.00% <0.00%> (ø)`
users/api/metrics.go	`0.00% <0.00%> (ø)`
users/users.go	`78.94% <ø> (ø)`
pkg/sdk/go/users.go	`17.58% <17.85%> (-3.26%)`	⬇️
users/api/requests.go	`43.13% <18.51%> (-27.70%)`	⬇️
users/api/endpoint.go	`41.36% <26.66%> (-41.18%)`	⬇️
users/service.go	`58.51% <32.20%> (-23.76%)`	⬇️
users/postgres/groups.go	`32.24% <32.24%> (ø)`
... and 10 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 043d1e0...b12dcbb. Read the comment docs.

manuio · 2020-09-14T15:29:25Z

docker/docker-compose.yml

@@ -103,6 +103,8 @@ services:
      POSTGRES_DB: ${MF_USERS_DB}
    networks:
      - mainflux-base-net
+    ports:
+      - "6001:5432"


Can you maybe use ENVARS here? It's hard to understand this values.

this is mistake, will remove this, only for development purposes

manuio · 2020-09-14T18:11:24Z

users/users.go

 	RetrieveByEmail(ctx context.Context, email string) (User, error)

+	// RetrieveByID retrieves user by its unique identifier ID.
+	// If groups = TRUE retrieve groups for given User


do we need this comment?

manuio · 2020-09-14T18:11:42Z

users/users.go


 	// Update updates the user metadata.
 	UpdateUser(ctx context.Context, u User) error

 	// RetrieveByEmail retrieves user by its unique identifier (i.e. email).
+	// If groups = TRUE retrieve groups for given User


do we need this comment?

manuio · 2020-09-14T18:15:42Z

users/service.go

+	}
+
+	return svc.users.RetrieveAllForGroup(ctx, groupID, offset, limit, um)
+


Remove this blank line

manuio · 2020-09-14T18:16:42Z

users/service.go

+	ErrUserAlreadyAssigned = errors.New("user is already assigned to a group")
+
+	// ErrAssignUserToGroup indicates an error in assigning user to a group.
+	ErrAssignUserToGroup = errors.New("error assigning user to a group")


Please, avoid error in an error message. Use Failed to instead.

manuio · 2020-09-14T18:19:19Z

users/postgres/users.go

+	q := fmt.Sprintf(`SELECT u.id, u.email, u.metadata FROM users u, group_relations g
+					  WHERE u.id = g.user_id AND g.group_id = :group 
+		  	          %s ORDER BY id LIMIT :limit OFFSET :offset;`, mq)


This doesn't look properly aligned

users/postgres/users.go

manuio · 2020-09-14T18:21:19Z

users/postgres/init.go

+					    email    VARCHAR(254) PRIMARY KEY,
+						password CHAR(60)     NOT  NULL


This doesn't look properly aligned

manuio · 2020-09-14T18:24:02Z

users/groups.go

+	// AssignUser adds user to group.
+	AssignUser(ctx context.Context, userID, groupID string) error
+
+	// RemoveUser


Complete this comment

manuio · 2020-09-14T18:24:35Z

users/emailer/emailer.go

@@ -24,6 +24,6 @@ func New(url string, c *email.Config) (users.Emailer, error) {

 func (e *emailer) SendPasswordReset(To []string, host string, token string) error {
 	url := fmt.Sprintf("%s%s?token=%s", host, e.resetURL, token)
-	content := fmt.Sprintf("%s", url)
+	content := url


Do you need content anymore?

manuio · 2020-09-14T18:27:12Z

users/api/metrics.go

+	return ms.svc.ListGroups(ctx, token, id, offset, limit, meta)
+}
+
+func (ms *metricsMiddleware) ListUsersInGroup(ctx context.Context, token, id string, offset, limit uint64, meta users.Metadata) (users.UserPage, error) {


This could be ListGroupUsers

@manuio ListUsersGroup doesn't it sound more logical? Following work orders List Users Group e.g List all users group. Detail ;)

@nmarcetic I don't think so :) We want users that are in a group not the group belonging to users.
We can keep this naming to avoid confusions.

Hm @manuio Yea, that makes more sense :)

manuio · 2020-09-15T08:46:59Z

cmd/users/main.go

@@ -99,21 +107,26 @@ const (
 	envAuthnCACerts = "MF_AUTHN_CA_CERTS"
 	envAuthnURL     = "MF_AUTHN_GRPC_URL"
 	envAuthnTimeout = "MF_AUTHN_GRPC_TIMEOUT"
+
+	defaultAdminGroupDescription = "Mainflux default admin group"


Can we move this with other def values?

mteodor · 2020-09-15T09:04:23Z

pkg/sdk/go/groups.go

+	return tp, nil
+}
+
+func (sdk mfSDK) UsersGroups(token string, offset, limit uint64, id string) (UsersGroupsPage, error) {


I'm not sure what would fit better here, this method will list Groups, later we will introduce groups for things so we will have ThingsGroups, if I use User instead of Users may sound like list Groups for certain user

@mteodor I was thinking that you will split user groups to users and thing groups to things. Not making pkg/sdk/go/groups

it was clearer to me have groups separate as a separate entity, similar to the channels.
there is a possibility that we will actually reuse these groups for things as well, maybe just adding type to the group entity.

Its something that we should agree, I know about this idea from ~ 1 year ago :) at least. It's still not documented and you started to work in freestyle mode, not really :) But you get the point.
@mteodor I think we need new issue like Generic groups and there discuss about this and get a clear vision. It will helps a lot if we brainstorm

I agree. I'm not sure about the idea. However, if we decide to create generic groups, it starts to look like a new service, which I am skeptical about.

manuio · 2020-09-15T08:57:56Z

users/api/metrics.go

+	return ms.svc.ListGroups(ctx, token, id, offset, limit, meta)
+}
+
+func (ms *metricsMiddleware) ListUsersInGroup(ctx context.Context, token, id string, offset, limit uint64, meta users.Metadata) (users.UserPage, error) {


@nmarcetic I don't think so :) We want users that are in a group not the group belonging to users.
We can keep this naming to avoid confusions.

mteodor · 2020-09-15T09:04:23Z

pkg/sdk/go/groups.go

+	return tp, nil
+}
+
+func (sdk mfSDK) UsersGroups(token string, offset, limit uint64, id string) (UsersGroupsPage, error) {


I'm not sure what would fit better here, this method will list Groups, later we will introduce groups for things so we will have ThingsGroups, if I use User instead of Users may sound like list Groups for certain user

manuio · 2020-09-15T09:37:09Z

pkg/sdk/go/sdk.go

-	Metadata map[string]interface{} `json:"metadata,omitempty"`
+	ID          string                 `json:"id,omitempty"`
+	Email       string                 `json:"email,omitempty"`
+	UsersGroups []string               `json:"groups,omitempty"`


I would simply use Groups or GroupsIDs

manuio · 2020-09-15T09:40:50Z

pkg/sdk/go/sdk.go

+	AssignUserToGroup(userID, groupID, token string) error
+
+	// RemoveUserFromGroup
+	RemoveUserFromGroup(userID, groupID, token string) error
+
+	// ListUsersForGroup
+	ListUsersForGroup(groupID, token string, offset, limit uint64) (UsersPage, error)


This naming is not consistent with the rest. I would use CreateGroupUser, RemoveGroupUser and GroupUsers

manuio · 2020-09-15T10:19:14Z

pkg/sdk/go/groups.go

+	}
+
+	return nil
+


Remove this blank line please

manuio · 2020-09-15T11:30:11Z

pkg/sdk/go/groups.go

 	endpoint := fmt.Sprintf("%s/%s/users?offset=%d&limit=%d&", groupsEndpoint, groupID, offset, limit)
 	url := createURL(sdk.baseURL, sdk.groupsPrefix, endpoint)
-
+	fmt.Println(url)


Remove this log

dborovcanin · 2020-09-15T21:35:45Z

users/postgres/groups.go

+	} else {
+		q = `INSERT INTO groups (name, description, id, owner_id, parent_id, metadata) VALUES (:name, :description, :id, :owner_id, :parent_id, :metadata) RETURNING id`
+	}


Please avoid using else.

dborovcanin · 2020-09-15T21:36:41Z

users/postgres/groups.go

+	if group.ID == "" || group.Name == "" || !groupRegexp.MatchString(group.Name) {
+		return users.Group{}, users.ErrMalformedEntity
+	}


This can be done on the service layer.

dborovcanin · 2020-09-15T21:42:51Z

users/postgres/groups.go

+}
+
+func (gr groupRepository) Delete(ctx context.Context, groupID string) error {
+	tx, err := gr.db.BeginTxx(context.Background(), nil)


Looks like this check introduces a lot of complexity only to check if a group is empty. Leaving this check to the user and removing it from the code would simplify it. If there is a need, we can add it later.

dborovcanin · 2020-09-15T21:44:45Z

users/postgres/groups.go

+	tx, err := gr.db.BeginTxx(context.Background(), nil)
+	if err != nil {
+		return errors.Wrap(errDeleteGroupDB, err)
+	}
+	defer func() {
+		if err != nil {
+			if txErr := tx.Rollback(); txErr != nil {
+				err = errors.Wrap(err, errors.Wrap(errTransRollback, txErr))
+			}
+		}
+
+		if err = tx.Commit(); err != nil {
+			err = errors.Wrap(errDeleteGroupDB, err)
+		}
+	}()
+
+	q := `SELECT COUNT(*) FROM group_relations WHERE group_id = :group_id AND user_id = :user_id ;`
+	dbr, err := toDBGroupRelation(userID, groupID)
+	if err != nil {
+		return errors.Wrap(users.ErrAssignUserToGroup, err)
+	}
+	tot, err := total(ctx, gr.db, q, dbr)
+	if err != nil {
+		return errors.Wrap(users.ErrAssignUserToGroup, err)
+	}
+	if tot > 0 {
+		return errors.Wrap(users.ErrUserAlreadyAssigned, err)
+	}


The same goes here. We can simply try to insert, and examine an error to decide what went wrong.

dborovcanin · 2020-09-15T21:47:18Z

users/postgres/groups.go

+	Group uuid.NullUUID `db:"group_id"`
+	User  uuid.NullUUID `db:"user_id"`


Why NullID if it can't be null by the schema?

.env

cli/groups.go

users/api/responses.go

dborovcanin

CI is failing, please fix it.

dborovcanin · 2020-09-17T08:30:26Z

cmd/users/main.go

 	return svc
 }

+func createAdmin(svc users.Service, userRepo users.UserRepository, groupRepo users.GroupRepository, c config) error {


I like how you avoid exposing a new method in the service interface, but use existing methods from repo and service layers.

dborovcanin · 2020-09-17T08:35:58Z

pkg/sdk/go/groups.go

+	return nil
+}
+
+func (sdk mfSDK) AddGroupUser(userID, groupID, token string) error {


Please consider renaming this, since it's not clear what this method does. Is it assigning a user to a group?

dborovcanin · 2020-09-17T08:36:07Z

pkg/sdk/go/groups.go

+	return nil
+}
+
+func (sdk mfSDK) RemoveGroupUser(userID, groupID, token string) error {


dborovcanin · 2020-09-17T08:38:08Z

pkg/sdk/go/groups.go

+	return nil
+}
+
+func (sdk mfSDK) GroupUsers(groupID, token string, offset, limit uint64) (UsersPage, error) {


ListUsersByGroup sounds less ambiguous (even though it does not sound well), since this method does not group users, but lists users that belong to the group.

dborovcanin · 2020-09-17T08:41:07Z

pkg/sdk/go/groups.go

+	return tp, nil
+}
+
+func (sdk mfSDK) UsersGroups(token string, offset, limit uint64, id string) (UsersGroupsPage, error) {


I agree. I'm not sure about the idea. However, if we decide to create generic groups, it starts to look like a new service, which I am skeptical about.

dborovcanin · 2020-09-17T08:56:28Z

users/api/metrics.go

+
+func (ms *metricsMiddleware) ListUserGroups(ctx context.Context, token, id string, offset, limit uint64, meta users.Metadata) (users.GroupPage, error) {
+	defer func(begin time.Time) {
+		ms.counter.With("method", "list_groups_for_user").Add(1)


"list_user_groups"

dborovcanin · 2020-09-17T09:10:25Z

users/groups.go

+	AssignUser(ctx context.Context, userID, groupID string) error
+
+	// RemoveUser removes user from group
+	RemoveUser(ctx context.Context, userID, groupID string) error


Consider using unassing as the opposite of assign or add as the opposite of remove. I prefer assign/unassign.

users/mocks/groups.go

dborovcanin · 2020-09-17T09:13:08Z

users/postgres/database.go

 // Database provides a database interface
 type Database interface {
 	NamedExecContext(context.Context, string, interface{}) (sql.Result, error)
 	QueryRowxContext(context.Context, string, ...interface{}) *sqlx.Row
 	NamedQueryContext(context.Context, string, interface{}) (*sqlx.Rows, error)
 	GetContext(context.Context, interface{}, string, ...interface{}) error
+	BeginTxx(ctx context.Context, opts *sql.TxOptions) (*sqlx.Tx, error)


Since you removed the code that uses this method, you can remove it from the interface and implementation, as well.

dborovcanin · 2020-09-17T09:26:27Z

users/users.go

@@ -54,16 +61,22 @@ func (u User) Validate() error {
 type UserRepository interface {
 	// Save persists the user account. A non-nil error is returned to indicate
 	// operation failure.
-	Save(ctx context.Context, u User) error
+	Save(ctx context.Context, u User) (User, error)


Why do we need to return the User? We usually only need an ID of the created entity.

cli/groups.go

drasko · 2020-09-20T21:51:03Z

cmd/cli/main.go

@@ -20,6 +20,7 @@ func main() {
 		CertsURL:          "http://localhost:8204",
 		ReaderPrefix:      "",
 		UsersPrefix:       "",
+		GroupsPrefix: "",


Please align

drasko · 2020-09-20T21:54:06Z

pkg/sdk/go/things_test.go

@@ -65,6 +65,7 @@ func TestCreateThing(t *testing.T) {
 	sdkConf := sdk.Config{
 		BaseURL:           ts.URL,
 		UsersPrefix:       "",
+		GroupsPrefix: "",


Needs to be aligned

drasko · 2020-09-20T21:54:14Z

pkg/sdk/go/things_test.go

@@ -125,6 +126,7 @@ func TestCreateThings(t *testing.T) {
 	sdkConf := sdk.Config{
 		BaseURL:           ts.URL,
 		UsersPrefix:       "",
+		GroupsPrefix: "",


drasko · 2020-09-20T21:55:50Z

pkg/sdk/go/users.go

@@ -147,3 +156,33 @@ func (sdk mfSDK) UpdatePassword(oldPass, newPass, token string) error {

 	return nil
 }
+func (sdk mfSDK) UserGroups(userID, token string, offset, limit uint64) (GroupsPage, error) {


If you are calling everything Groups in SDK, probably you slhould leave Groups() here as well - especially that this is users.go file.

This is for listing groups for user, therefore it is UserGroups

drasko · 2020-09-20T21:59:39Z

users/api/logging.go

+	return lm.svc.UnassignUserFromGroup(ctx, token, userID, groupID)
+}
+
+func (lm *loggingMiddleware) ListUserGroups(ctx context.Context, token, id string, offset, limit uint64, meta users.Metadata) (e users.GroupPage, err error) {


Shouldn't this be ListGroups(), in order to be consistent?

this lists groups that user is assigned to

drasko · 2020-09-20T22:05:15Z

users/tracing/groups.go

+}
+
+// GroupRepositoryMiddleware tracks request and their latency, and adds spans
+// to context.


Move to upper line - ty to keep all comments in one line if it is not longer than code below.

drasko · 2020-09-20T22:05:42Z

users/tracing/groups.go

+	return grm.repo.RetrieveByName(ctx, name)
+}
+
+func (grm groupRepositoryMiddleware) RetrieveAll(ctx context.Context, groupID string, offset, limit uint64, gm users.Metadata) (users.GroupPage, error) {


Please break this line - it's too long

drasko · 2020-09-20T22:05:53Z

users/tracing/groups.go

+	return grm.repo.RetrieveAll(ctx, groupID, offset, limit, gm)
+}
+
+func (grm groupRepositoryMiddleware) RetrieveAllForUser(ctx context.Context, userID string, offset, limit uint64, gm users.Metadata) (users.GroupPage, error) {


Please break this line

Memberships()?

drasko · 2020-09-20T22:06:12Z

users/tracing/users.go

@@ -68,6 +75,14 @@ func (urm userRepositoryMiddleware) UpdatePassword(ctx context.Context, email, p
 	return urm.repo.UpdatePassword(ctx, email, password)
 }

+func (urm userRepositoryMiddleware) RetrieveAllForGroup(ctx context.Context, groupID string, offset, limit uint64, gm users.Metadata) (users.UserPage, error) {


Please break this line

drasko · 2020-09-20T22:09:06Z

users/tracing/groups.go

+	removeUser         = "remove_user_from_group"
+)
+
+var _ users.GroupRepository = (*groupRepositoryMiddleware)(nil)


Is this Middleware suffix aligned to what we used in other services? It makes variable name pretty long. Do we need it at all?

similar to : _ things.ChannelRepository = (*channelRepositoryMiddleware)(nil)

manuio · 2020-09-21T11:02:05Z

cli/README.md

+```bash
+mainflux-cli groups get all <user_auth_token>
+```
+### List children groups for some group


Looks like `shows group with provided group ID`` command is missing

manuio · 2020-09-21T11:07:27Z

cli/groups.go

+		ParentID - ID of a group that is a parent
+		to the creating group


Do we need 2 lines here?

manuio · 2020-09-21T11:09:45Z

cli/README.md

+```
+### List groups user belongs to
+```bash
+mainflux-cli users groups <user_id> <user_auth_token>


This is a users or groups command?

this is users command

Why it's defined with Group commands?

manuio · 2020-09-21T11:11:08Z

cli/groups.go

+	cmd := cobra.Command{
+		Use:   "groups",
+		Short: "Groups management",
+		Long:  `Groups management: create accounts and tokens"`,


create accounts and tokens?

manuio · 2020-09-21T11:16:30Z

cmd/users/main.go

+	envAdminPassword = "MF_USERS_ADMIN_PASSWORD"
+	envAdminGroup    = "MF_USERS_ADMIN_GROUP"
+
+	envJaegerURL = "MF_JAEGER_URL"


Why envJaegerURL is defined here?

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

dborovcanin · 2020-09-23T14:19:11Z

pkg/sdk/go/users.go

@@ -147,3 +156,33 @@ func (sdk mfSDK) UpdatePassword(oldPass, newPass, token string) error {

 	return nil
 }
+func (sdk mfSDK) Memberships(userID, token string, offset, limit uint64) (GroupsPage, error) {


Please add an empty line before this/after the previous method.

dborovcanin · 2020-09-23T14:31:15Z

users/api/endpoint.go

+		req := request.(listUserGroupReq)
+		if err := req.validate(); err != nil {
+			return users.UserPage{}, err
+		}
+
+		up, err := svc.Members(ctx, req.token, req.groupID, req.offset, req.limit, req.metadata)
+		if err != nil {
+			return users.UserPage{}, err
+		}
+		return buildUsersResponse(up), nil


Please be consistent when adding empty lines. I don't mind any combination of

request typ eassert errcheck <AN EMPTY LINE> svc method call errcheck <AN EMTPY LINE> prepare return <AN EMPTY LINE> return result

You can have all the empty lines, some of them, or none of them. As long as you're consistent, each option is OK for me. Please check all the other places in the code where this can be an issue.

dborovcanin · 2020-09-23T14:36:22Z

users/api/requests.go

 const minPassLen = 8
-
-type apiReq interface {
-	validate() error
-}
+const maxNameSize = 1024


Group:

const ( minPassLen = 8 maxNameLen = 1024 )

dborovcanin · 2020-09-23T14:37:03Z

users/api/requests.go

+	if req.token == "" {
+		return users.ErrUnauthorizedAccess
+	}
+
+	if len(req.Name) > maxNameSize || req.Name == "" {
+		return users.ErrMalformedEntity
+	}
+
+	return nil


The same remark for empty lines in validate methods.

users/api/transport.go

dborovcanin · 2020-09-23T14:41:53Z

users/api/transport.go

@@ -87,6 +105,69 @@ func MakeHandler(svc users.Service, tracer opentracing.Tracer, l log.Logger) htt
 		opts...,
 	))

+	mux.Post("/users/groups", kithttp.NewServer(


We probably don't need /users prefix here. I assume that you had Things Groups in mind, but this can be resolved in LB or internal Nginx. After all, it's two services.

dborovcanin · 2020-09-23T14:45:57Z

users/groups.go

+	// Update updates the group data.
+	Update(ctx context.Context, g Group) error
+
+	// Delete deletes group for given id


Either add . at the end of the comment or not. @drasko And I don't agree around this (I'm for it, he's not). Just be consistent, whatever you choose.

dborovcanin · 2020-09-23T15:00:21Z

users/postgres/groups.go

+	return group, nil
+}
+
+func (gr groupRepository) RetrieveAll(ctx context.Context, groupID string, offset, limit uint64, gm users.Metadata) (users.GroupPage, error) {


Consider renaming to RetrieveWithAncestors or something like that.

dborovcanin · 2020-09-23T15:04:28Z

users/postgres/groups.go

+		items = append(items, gr)
+	}
+
+	cq := fmt.Sprintf(`WITH RECURSIVE subordinates AS (


I'm not sure if we need this. I prefer just adding a limit to the depth to some arbitrary number (10?) and return the list with no need for pagination and complications. What do you think? I doubt that we'll ever need more than 10-15 ancestors, and we don't need pagination for that.

This implies that we'll need this COUNT to check the depth when adding a group.

it works like a charm

Do you want to keep it like this? I'm perfectly fine with it. @mainflux/maintainers Your opinion? It's ancestors pagination VS ancestors depth limit.

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

manuio · 2020-09-23T16:48:32Z

cli/groups.go

+		Use:   "membership",
+		Short: "membership <user_id> <user_auth_token>",


memberships

manuio · 2020-09-23T16:51:45Z

cli/groups.go

+	cobra.Command{
+		Use:   "membership",
+		Short: "membership <user_id> <user_auth_token>",
+		Long:  `List user groups membership`,


manuio · 2020-09-23T16:52:08Z

cli/groups.go

+		Short: "Groups management",
+		Long:  `Groups management: create groups and assigns user to groups"`,
+		Run: func(cmd *cobra.Command, args []string) {
+			logUsage("Usage: Groups [create | get | delete | assign | unassign | members | membership]")


if you change previous one be careful here

drasko · 2020-09-23T19:00:33Z

users/postgres/users.go

 }

-func toUser(dbu dbUser) users.User {
+func toUser(dbu dbUser) (users.User, error) {


Why is this function called toUser()? What does this to prefix mean?

Similarly - why is toDBUser() called like this?

Maybe there is a reason, but it is cryptic - if it is valid please put a comment above function names.

OK, this: https://github.com/mainflux/mainflux/blob/master/things/postgres/things.go#L344 shows consistency, so I'll approve.

drasko

LGTM

drasko · 2020-09-23T21:19:31Z

Congrats on this one. Thanks a lot @mteodor for great contribution!

nmarcetic · 2020-09-25T05:53:02Z

@mteodor In this impl, Is there something specific with this relation group <-> user (except owner ID)? This would be much simple in the future impl if you allow grouping of different entities. It will simplify access control a lot and general usage.
I would be able to group users, things, and channels in the same group (like floor 1, edge location 1, etc...). You did impl really generic, looks doable with little or no effort at all :) It will also simplify the code base. What do you think, Am I am missing something?

mteodor · 2020-09-25T11:23:57Z

there is associationt table group_relations which keeps info on user assignment.
we could use that same table to insert things, and channels
i also think is doable, a little problem may be that things and users are different service but I guess that we make grpc endpoints that could glue all that.

dborovcanin · 2020-09-25T13:21:00Z

Groups are usually used for access control and Authorization service looks like a good candidate if we decide to make groups generic.

drasko · 2020-09-26T01:09:03Z

I agree - this should be generalized.

nmarcetic · 2020-09-28T08:13:36Z

Great! Let's think about design and refactor in the next phase.

* adding group Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding user group Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding group Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add retrieve methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add default admin user Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add default admin user Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding endpoints Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding endpoints Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding tests Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * changes signature for AssignUser Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding tests Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * bug fixing retrieving groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove unused code Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * bug fixing retrieving groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * retrieve groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * change environment for admin Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * change environment for admin Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * retrieve groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove adding default group Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * expose port for debugging purposes Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix tests, and linter errors Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add prefix Users for groups endpoint Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix linter problems Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix endpoint prefix url Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix endpoint test Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add group features in cli Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove comments Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove println Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * when user is created return id in response Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * when user is created return id in response Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * adding default admin env Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * proper alignment Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * proper alignment Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix comments Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * rename method Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * return user id when created Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * return user id when created Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove unused variable Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * rename methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix to retrieve whole tree starting from parent Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add endpoint to list groups for user Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add readme for groups Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fixing bugs Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fixing bugs Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add group commands for add and remove user Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * replace default email, use example.com Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix capital letters beginning of sentence Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove warning for deprecated api, mistakenly copied Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * simplify repo methods, rely on db driver rather than the check before operation Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * check if group is valid Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * openapi spec 3.0 Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * remove check for existing users in groups before delete Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * change func signature Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * change func signature Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix bugs, resolve comments Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix bugs, resolve comments Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix alignment Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * add missing command Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * reorganize envs Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix doc Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix compile Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * reorganize cli commands Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * minor corrections Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * rename methods Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * fix naming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * renaming Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com> * resolve comments, minor changes Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

mteodor requested a review from a team as a code owner August 21, 2020 11:24

mteodor marked this pull request as draft August 21, 2020 11:24

mteodor added the work-in-progress label Aug 21, 2020

drasko requested changes Sep 7, 2020

View reviewed changes

manuio suggested changes Sep 8, 2020

View reviewed changes

nmarcetic reviewed Sep 9, 2020

View reviewed changes

mteodor force-pushed the add-group branch from d922885 to 6cb4c75 Compare September 10, 2020 17:00

manuio suggested changes Sep 10, 2020

View reviewed changes

mteodor removed the work-in-progress label Sep 14, 2020

mteodor marked this pull request as ready for review September 14, 2020 15:08

manuio suggested changes Sep 14, 2020

View reviewed changes

dborovcanin changed the title ~~NOISSUE - Adding user groups~~ NOISSUE - Add user groups Sep 15, 2020

manuio reviewed Sep 15, 2020

View reviewed changes

mteodor commented Sep 15, 2020

View reviewed changes

manuio suggested changes Sep 15, 2020

View reviewed changes

pkg/sdk/go/groups.go Outdated

}

return nil

Copy link

Contributor

manuio Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this blank line please

manuio suggested changes Sep 15, 2020

View reviewed changes

dborovcanin requested changes Sep 15, 2020

View reviewed changes

blokovi reviewed Sep 15, 2020

View reviewed changes

.env Outdated Show resolved Hide resolved

cli/groups.go Outdated Show resolved Hide resolved

users/api/responses.go Outdated Show resolved Hide resolved

dborovcanin requested changes Sep 17, 2020

View reviewed changes

manuio suggested changes Sep 17, 2020

View reviewed changes

cli/groups.go Outdated Show resolved Hide resolved

drasko requested changes Sep 20, 2020

View reviewed changes

manuio suggested changes Sep 21, 2020

View reviewed changes

manuio reviewed Sep 21, 2020

View reviewed changes

manuio suggested changes Sep 21, 2020

View reviewed changes

mteodor added 9 commits September 23, 2020 16:26

reorganize cli commands

28f7d41

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

minor corrections

f49e35d

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

renaming

063e165

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

renaming

b6caf7b

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

renaming

0fb9195

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

rename methods

2e1136b

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

fix naming

9f6158b

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

renaming

d6cc349

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

renaming

7f685b7

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

mteodor dismissed blokovi’s stale review via 7f685b7 September 23, 2020 14:26

mteodor force-pushed the add-group branch from a20945f to 7f685b7 Compare September 23, 2020 14:26

dborovcanin requested changes Sep 23, 2020

View reviewed changes

resolve comments, minor changes

b12dcbb

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

manuio reviewed Sep 23, 2020

View reviewed changes

cli/groups.go

Comment on lines +150 to +151

Use: "membership",

Short: "membership <user_id> <user_auth_token>",

Copy link

Contributor

manuio Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memberships

manuio reviewed Sep 23, 2020

View reviewed changes

dborovcanin approved these changes Sep 23, 2020

View reviewed changes

manuio approved these changes Sep 23, 2020

View reviewed changes

drasko requested changes Sep 23, 2020

View reviewed changes

drasko approved these changes Sep 23, 2020

View reviewed changes

drasko merged commit 8ea26c5 into absmach:master Sep 23, 2020

drasko mentioned this pull request Oct 14, 2020

Introduce organizations (user groups) for full multi-tenancy #649

Closed

dborovcanin mentioned this pull request Nov 17, 2020

Add Users Groups tests #1285

Closed

		}

		return svc.users.RetrieveAllForGroup(ctx, groupID, offset, limit, um)

		Group uuid.NullUUID `db:"group_id"`
		User uuid.NullUUID `db:"user_id"`

		ParentID - ID of a group that is a parent
		to the creating group

		Use: "membership",
		Short: "membership <user_id> <user_auth_token>",

NOISSUE - Add user groups #1228

NOISSUE - Add user groups #1228

Conversation

mteodor commented Aug 21, 2020 • edited Loading

What does this do?

List any changes that modify/break current functionality

Have you included tests for your changes?

manuio commented Sep 7, 2020

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-commenter commented Sep 12, 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

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

manuio Sep 15, 2020 • edited Loading

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

manuio Sep 15, 2020 • edited Loading

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

dborovcanin left a 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

mteodor commented Aug 21, 2020 •

edited

Loading

codecov-commenter commented Sep 12, 2020 •

edited

Loading

manuio Sep 15, 2020 •

edited

Loading

manuio Sep 15, 2020 •

edited

Loading