Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MF-739 - Add ID to the User entity #1152

Merged
merged 12 commits into from
Jun 4, 2020
10 changes: 10 additions & 0 deletions idp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright (c) Mainflux
// SPDX-License-Identifier: Apache-2.0

package mainflux

// IdentityProvider specifies an API for generating unique identifiers.
type IdentityProvider interface {
// ID generates the unique identifier.
ID() (string, error)
}
1 change: 1 addition & 0 deletions sdk/go/sdk.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ var _ SDK = (*mfSDK)(nil)

// User represents mainflux user its credentials.
type User struct {
ID string `json:"id,omitempty"`
Email string `json:"email,omitempty"`
Password string `json:"password,omitempty"`
Metadata map[string]interface{} `json:"metadata,omitempty"`
Expand Down
6 changes: 2 additions & 4 deletions things/postgres/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,14 @@ func migrateDB(db *sqlx.DB) error {
Id: "things_2",
Up: []string{
`ALTER TABLE IF EXISTS things ALTER COLUMN
metadata TYPE JSONB using metadata::text::jsonb
`,
metadata TYPE JSONB using metadata::text::jsonb`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda liked that this comma was in the new line. I am not saying that this is good solution, but I think it would be worth to check best practices when writing SQL strings in the code...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would follow the standard which is to don't add a new line. We are also following this standard in other services.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manuio I agree, I just suggested to check where this standard commens and if it widely accepted, not something just invented by us. How other big Go projects handle this problem?

Copy link
Contributor Author

@manuio manuio May 1, 2020

Choose a reason for hiding this comment

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

i think that there are 2 versions:

				Up: []string{
					`ALTER TABLE IF EXISTS channels ALTER COLUMN
					 metadata TYPE JSONB using metadata::text::jsonb`,
				},

or

				Up: []string{`
                                         ALTER TABLE IF EXISTS channels ALTER COLUMN
					 metadata TYPE JSONB using metadata::text::jsonb`,
				},

And I like the one that we are using

Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting some research links... And my remark was about comma (,) at the end of the statement, not backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drasko the , still there

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it, but it is not in the new line. So this is what I am trying to figure out - should it be in a new line or not.

Copy link
Contributor Author

@manuio manuio May 1, 2020

Choose a reason for hiding this comment

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

In this case it's not a good proposition since it doesn't build, Lines are closed with ,

Copy link
Contributor

Choose a reason for hiding this comment

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

How did it build before then?

Copy link
Contributor Author

@manuio manuio May 1, 2020

Choose a reason for hiding this comment

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

Because of the `,

},
},
{
Id: "things_3",
Up: []string{
`ALTER TABLE IF EXISTS channels ALTER COLUMN
metadata TYPE JSONB using metadata::text::jsonb
`,
metadata TYPE JSONB using metadata::text::jsonb`,
},
},
},
Expand Down
4 changes: 3 additions & 1 deletion things/postgres/things_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,9 @@ func TestThingUpdate(t *testing.T) {
Key: thkey,
}

sths, _ := thingRepo.Save(context.Background(), thing)
sths, err := thingRepo.Save(context.Background(), thing)
require.Nil(t, err, fmt.Sprintf("unexpected error: %s", err))

thing.ID = sths[0].ID

nonexistentThingID, err := uuid.New().ID()
Expand Down
6 changes: 5 additions & 1 deletion users/api/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ func userInfoEndpoint(svc users.Service) endpoint.Endpoint {
return nil, err
}

return identityRes{u.Email, u.Metadata}, nil
return identityRes{
ID: u.ID,
Email: u.Email,
Metadata: u.Metadata,
}, nil
}
}

Expand Down
1 change: 1 addition & 0 deletions users/api/responses.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func (res updateUserRes) Empty() bool {
}

type identityRes struct {
ID string `json:"id"`
Email string `json:"email"`
Metadata map[string]interface{} `json:"metadata,omitempty"`
}
Expand Down
13 changes: 10 additions & 3 deletions users/postgres/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ func migrateDB(db *sqlx.DB) error {
Id: "users_1",
Up: []string{
`CREATE TABLE IF NOT EXISTS users (
email VARCHAR(254) PRIMARY KEY,
password CHAR(60) NOT NULL
)`,
email VARCHAR(254) PRIMARY KEY,
password CHAR(60) NOT NULL)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Move closing brackets in the next line, to mark clearly the block closing.

},
Down: []string{"DROP TABLE users"},
},
Expand All @@ -61,6 +60,14 @@ func migrateDB(db *sqlx.DB) error {
`ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS metadata JSONB`,
},
},
{
Id: "users_3",
Up: []string{
`CREATE EXTENSION IF NOT EXISTS "uuid-ossp";
ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS
id UUID DEFAULT uuid_generate_v4()`,
},
},
},
}

Expand Down
8 changes: 5 additions & 3 deletions users/postgres/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func New(db Database) users.UserRepository {
}

func (ur userRepository) Save(ctx context.Context, user users.User) error {
q := `INSERT INTO users (email, password, metadata) VALUES (:email, :password, :metadata)`
q := `INSERT INTO users (id, email, password, metadata) VALUES (:id, :email, :password, :metadata)`

dbu := toDBUser(user)
if _, err := ur.db.NamedExecContext(ctx, q, dbu); err != nil {
Expand Down Expand Up @@ -71,7 +71,7 @@ func (ur userRepository) UpdateUser(ctx context.Context, user users.User) error
}

func (ur userRepository) RetrieveByID(ctx context.Context, email string) (users.User, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If its RetrieveByID why param is email ?

Copy link
Contributor Author

@manuio manuio May 5, 2020

Choose a reason for hiding this comment

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

@nmarcetic because in Users service we use email as unique ID. I can maybe rename the function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea because we didn't have ID but now when we introduce ID, this should be changed.

q := `SELECT password, metadata FROM users WHERE email = $1`
q := `SELECT id, password, metadata FROM users WHERE email = $1`

dbu := dbUser{
Email: email,
Expand Down Expand Up @@ -121,7 +121,6 @@ func (m *dbMetadata) Scan(value interface{}) error {
}

if err := json.Unmarshal(b, m); err != nil {
m = &dbMetadata{}
return err
}

Expand All @@ -142,13 +141,15 @@ func (m dbMetadata) Value() (driver.Value, error) {
}

type dbUser struct {
ID string `db:"id"`
Email string `db:"email"`
Password string `db:"password"`
Metadata dbMetadata `db:"metadata"`
}

func toDBUser(u users.User) dbUser {
return dbUser{
ID: u.ID,
Email: u.Email,
Password: u.Password,
Metadata: u.Metadata,
Expand All @@ -157,6 +158,7 @@ func toDBUser(u users.User) dbUser {

func toUser(dbu dbUser) users.User {
return users.User{
ID: dbu.ID,
Email: dbu.Email,
Password: dbu.Password,
Metadata: dbu.Metadata,
Expand Down
21 changes: 17 additions & 4 deletions users/postgres/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,17 @@ import (
"github.com/mainflux/mainflux/errors"
"github.com/mainflux/mainflux/users"
"github.com/mainflux/mainflux/users/postgres"
"github.com/mainflux/mainflux/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestUserSave(t *testing.T) {
email := "user-save@example.com"

uid, err := uuid.New().ID()
require.Nil(t, err, fmt.Sprintf("got unexpected error: %s", err))

cases := []struct {
desc string
user users.User
Expand All @@ -26,6 +30,7 @@ func TestUserSave(t *testing.T) {
{
desc: "new user",
user: users.User{
ID: uid,
Email: email,
Password: "pass",
},
Expand All @@ -34,6 +39,7 @@ func TestUserSave(t *testing.T) {
{
desc: "duplicate user",
user: users.User{
ID: uid,
Email: email,
Password: "pass",
},
Expand All @@ -51,14 +57,21 @@ func TestUserSave(t *testing.T) {
}

func TestSingleUserRetrieval(t *testing.T) {
email := "user-retrieval@example.com"

dbMiddleware := postgres.NewDatabase(db)
repo := postgres.New(dbMiddleware)
err := repo.Save(context.Background(), users.User{

email := "user-retrieval@example.com"

uid, err := uuid.New().ID()
require.Nil(t, err, fmt.Sprintf("got unexpected error: %s", err))

user := users.User{
ID: uid,
Email: email,
Password: "pass",
})
}

err = repo.Save(context.Background(), user)
require.Nil(t, err, fmt.Sprintf("unexpected error: %s", err))

cases := map[string]struct {
Expand Down
20 changes: 16 additions & 4 deletions users/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/mainflux/mainflux"
"github.com/mainflux/mainflux/authn"
"github.com/mainflux/mainflux/errors"
"github.com/mainflux/mainflux/uuid"
)

var (
Expand All @@ -31,20 +32,23 @@ var (
ErrUserNotFound = errors.New("non-existent user")

// ErrScanMetadata indicates problem with metadata in db.
ErrScanMetadata = errors.New("Failed to scan metadata")
ErrScanMetadata = errors.New("failed to scan metadata")

// ErrMissingEmail indicates missing email for password reset request.
ErrMissingEmail = errors.New("missing email for password reset")

// ErrMissingResetToken indicates malformed or missing reset token
// for reseting password.
ErrMissingResetToken = errors.New("error missing reset token")
ErrMissingResetToken = errors.New("missing reset token")

// ErrRecoveryToken indicates error in generating password recovery token.
ErrRecoveryToken = errors.New("error generating password recovery token")
ErrRecoveryToken = errors.New("failed to generate password recovery token")

// ErrGetToken indicates error in getting signed token.
ErrGetToken = errors.New("Get signed token failed")
ErrGetToken = errors.New("failed to fetch signed token")

// ErrCreateUser indicates error in creating User
ErrCreateUser = errors.New("failed to create user")
)

// Service specifies an API that must be fullfiled by the domain service
Expand Down Expand Up @@ -106,6 +110,13 @@ func (svc usersService) Register(ctx context.Context, user User) error {
}

user.Password = hash

uid, err := uuid.New().ID()
if err != nil {
return errors.Wrap(ErrCreateUser, err)
}
user.ID = uid

return svc.users.Save(ctx, user)
}

Expand Down Expand Up @@ -134,6 +145,7 @@ func (svc usersService) UserInfo(ctx context.Context, token string) (User, error
}

return User{
ID: dbUser.ID,
Email: id,
Password: "",
Metadata: dbUser.Metadata,
Expand Down
44 changes: 32 additions & 12 deletions users/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/mainflux/mainflux/users/mocks"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const wrong string = "wrong-value"
Expand Down Expand Up @@ -114,30 +115,41 @@ func TestLogin(t *testing.T) {
func TestUserInfo(t *testing.T) {
svc := newService()
svc.Register(context.Background(), user)
key, _ := svc.Login(context.Background(), user)

token, err := svc.Login(context.Background(), user)
require.Nil(t, err, fmt.Sprintf("unexpected error: %s", err))

u := user
u.Password = ""

cases := map[string]struct {
user users.User
key string
err error
user users.User
token string
err error
}{
"valid token's user info": {u, key, nil},
"invalid token's user info": {users.User{}, "", users.ErrUnauthorizedAccess},
"valid token's user info": {
user: u,
token: token,
err: nil,
},
"invalid token's user info": {
user: users.User{},
token: "",
err: users.ErrUnauthorizedAccess,
},
}

for desc, tc := range cases {
u, err := svc.UserInfo(context.Background(), tc.key)
assert.True(t, errors.Contains(err, tc.err), fmt.Sprintf("expected %s got %s\n", tc.err, err))
assert.Equal(t, tc.user, u, fmt.Sprintf("%s: expected %s got %s\n", desc, tc.user, u))
_, err := svc.UserInfo(context.Background(), tc.token)
assert.True(t, errors.Contains(err, tc.err), fmt.Sprintf("%s: expected %s got %s\n", desc, tc.err, err))
}
}

func TestUpdateUser(t *testing.T) {
svc := newService()
svc.Register(context.Background(), user)
key, _ := svc.Login(context.Background(), user)
token, err := svc.Login(context.Background(), user)
require.Nil(t, err, fmt.Sprintf("unexpected error: %s", err))

user.Metadata = map[string]interface{}{"role": "test"}

Expand All @@ -146,8 +158,16 @@ func TestUpdateUser(t *testing.T) {
token string
err error
}{
"update user with valid token": {user, key, nil},
"update user with invalid token": {user, "non-existent", users.ErrUnauthorizedAccess},
"update user with valid token": {
user: user,
token: token,
err: nil,
},
"update user with invalid token": {
user: user,
token: "non-existent",
err: users.ErrUnauthorizedAccess,
},
}

for desc, tc := range cases {
Expand Down
1 change: 1 addition & 0 deletions users/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var (
// User represents a Mainflux user account. Each user is identified given its
// email and password.
type User struct {
ID string
Email string
Password string
Metadata map[string]interface{}
Expand Down
32 changes: 32 additions & 0 deletions uuid/idp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Mainflux
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we call this top leve package uuid? It does not really implement uuid, but rather our IdP...

I do not know, does not bother me much, but I'd like to her form @mainflux/maintainers

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not an Identity provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should rename it to uuid/uuid.go ?

Copy link
Collaborator

@nmarcetic nmarcetic May 5, 2020

Choose a reason for hiding this comment

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

Make more sense to me @manuio or put it under /pkg its really shared lib

// SPDX-License-Identifier: Apache-2.0

// Package uuid provides a UUID identity provider.
package uuid

import (
"github.com/gofrs/uuid"
"github.com/mainflux/mainflux"
"github.com/mainflux/mainflux/errors"
)

// ErrGeneratingID indicates error in generating UUID
var ErrGeneratingID = errors.New("generating id failed")

var _ mainflux.IdentityProvider = (*uuidIdentityProvider)(nil)

type uuidIdentityProvider struct{}

// New instantiates a UUID identity provider.
func New() mainflux.IdentityProvider {
return &uuidIdentityProvider{}
}

func (idp *uuidIdentityProvider) ID() (string, error) {
id, err := uuid.NewV4()
if err != nil {
return "", errors.Wrap(ErrGeneratingID, err)
}

return id.String(), nil
}