MF-922 - Add UpdateUser endpoint #923

manuio · 2019-10-30T08:31:57Z

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Closes #922

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

users/api/http/transport.go

drasko · 2019-10-30T08:37:01Z

users/tracing/users.go

+	defer span.Finish()
+	ctx = opentracing.ContextWithSpan(ctx, span)
+
+	return urm.repo.Update(ctx, user)


Does this update whole user structure or just Metadata?

If it updates whole user structure, it means that User can change his own "constructive" information - which is contrary to the ide that users are created and managed by the admins - and can lead to security breaches.

@nmarcetic please review this one and advise.

@nmarcetic please review and advise here so we can move further.

codecov-io · 2019-10-30T08:40:14Z

Codecov Report

Merging #923 into master will decrease coverage by 0.66%.
The diff coverage is 13.72%.

@@            Coverage Diff             @@
##           master     #923      +/-   ##
==========================================
- Coverage   83.59%   82.93%   -0.67%     
==========================================
  Files          75       75              
  Lines        5341     5385      +44     
==========================================
+ Hits         4465     4466       +1     
- Misses        610      652      +42     
- Partials      266      267       +1

Impacted Files	Coverage Δ
users/users.go	`68.42% <ø> (ø)`	⬆️
users/api/http/requests.go	`53.33% <0%> (-8.21%)`	⬇️
users/postgres/users.go	`48.64% <0%> (-5.9%)`	⬇️
users/api/http/responses.go	`50% <0%> (-16.67%)`	⬇️
users/service.go	`25.64% <0%> (-2.94%)`	⬇️
users/api/http/transport.go	`78.01% <46.15%> (-4.08%)`	⬇️
users/api/http/endpoint.go	`73.91% <9.09%> (-12.3%)`	⬇️

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 8be2516...0851a28. Read the comment docs.

users/postgres/users.go

users/service.go

users/api/http/requests.go

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

…date

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01 · 2019-10-31T10:26:19Z

users/api/logging.go

+
+func (lm *loggingMiddleware) UpdateMetadata(ctx context.Context, token string, metadata map[string]interface{}) (email string, err error) {
+	defer func(begin time.Time) {
+		message := fmt.Sprintf("Method user_update for user %s took %s to complete", email, time.Since(begin))


Change user_update to update_metadata.

anovakovic01 · 2019-10-31T10:26:30Z

users/api/metrics.go

+
+func (ms *metricsMiddleware) UpdateMetadata(ctx context.Context, token string, metadata map[string]interface{}) (email string, err error) {
+	defer func(begin time.Time) {
+		ms.counter.With("method", "user_update").Add(1)


Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

…date

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01 · 2019-10-31T13:52:08Z

users/api/http/endpoint.go

@@ -90,6 +90,23 @@ func userInfoEndpoint(svc users.Service) endpoint.Endpoint {
 	}
 }

+func userUpdateMetaEndpoint(svc users.Service) endpoint.Endpoint {


Please rename this method name.

anovakovic01 · 2019-10-31T13:54:12Z

users/api/http/requests.go

@@ -28,6 +28,18 @@ func (req viewUserInfoReq) validate() error {
 	return nil
 }

+type updateUserReq struct {
+	Token string
+	User  users.User


First, I want to ask why is Token public? Second, I still think that here you should have fields that you're updating (for now just metadata). If you leave it this way, I can pass email and pass, and the request will be accepted.

anovakovic01 · 2019-10-31T13:54:41Z

users/api/logging.go

+
+func (lm *loggingMiddleware) UpdateUser(ctx context.Context, token string, u users.User) (err error) {
+	defer func(begin time.Time) {
+		message := fmt.Sprintf("Method update_user_info for user %s took %s to complete", u.Email, time.Since(begin))


Change to update_user

anovakovic01 · 2019-10-31T13:54:57Z

users/api/metrics.go

+
+func (ms *metricsMiddleware) UpdateUser(ctx context.Context, token string, u users.User) (err error) {
+	defer func(begin time.Time) {
+		ms.counter.With("method", "update_user_info").Add(1)


Change to update_user.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01

LGTM!

drasko

LGTM

drasko

Tests need to be added for this new endpoint.

manuio · 2019-10-31T15:34:39Z

Add missing tests in this issue: https://github.com/mainflux/mainflux/issues/926

* MF-922 - Add User Update endpoint Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Revert Update function Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix typo Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Update swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix Things swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * update Things swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix users swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix mocks Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix method name Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix swagger Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix typo Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Use User instead of metadata Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

MF-922 - Add User Update endpoint

072146c

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

manuio requested a review from a team as a code owner October 30, 2019 08:31

drasko requested changes Oct 30, 2019

View reviewed changes

anovakovic01 suggested changes Oct 30, 2019

View reviewed changes

users/postgres/users.go Show resolved Hide resolved

users/service.go Outdated Show resolved Hide resolved

users/api/http/requests.go Outdated Show resolved Hide resolved

manuio added 10 commits October 30, 2019 17:07

Fix reviews

f50c6c0

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Merge branch 'master' of https://github.com/mainflux/mainflux into up…

6e01334

…date

Revert Update function

8e19fba

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix typo

f71880d

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Update swagger

2b5a71e

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix Things swagger

d36191f

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix swagger

036c4d7

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

update Things swagger

c01e98a

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix users swagger

58551e5

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix mocks

ee9754d

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01 suggested changes Oct 31, 2019

View reviewed changes

manuio changed the title ~~MF-922 - Add User Update endpoint~~ MF-922 - Add User UpdateMetadata endpoint Oct 31, 2019

manuio added 7 commits October 31, 2019 11:42

Fix method name

5f3863f

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix swagger

ec7ed27

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix swagger

67f0ee8

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix swagger

8de7f9c

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix typo

067ff7c

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Merge branch 'master' of https://github.com/mainflux/mainflux into up…

13354d7

…date

Use User instead of metadata

ebfdde4

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01 suggested changes Oct 31, 2019

View reviewed changes

manuio added 4 commits October 31, 2019 15:08

Fix reviews

ab8897b

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix reviews

4f7a62f

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix reviews

cfdfc7b

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix reviews

0851a28

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

anovakovic01 approved these changes Oct 31, 2019

View reviewed changes

drasko approved these changes Oct 31, 2019

View reviewed changes

drasko requested changes Oct 31, 2019

View reviewed changes

manuio changed the title ~~MF-922 - Add User UpdateMetadata endpoint~~ MF-922 - Add User UpdateUser endpoint Oct 31, 2019

manuio changed the title ~~MF-922 - Add User UpdateUser endpoint~~ MF-922 - Add UpdateUser endpoint Oct 31, 2019

drasko approved these changes Oct 31, 2019

View reviewed changes

drasko merged commit 28a176a into absmach:master Oct 31, 2019

manuio deleted the update branch October 31, 2019 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-922 - Add UpdateUser endpoint #923

MF-922 - Add UpdateUser endpoint #923

manuio commented Oct 30, 2019 •

edited

Loading

drasko Oct 30, 2019

drasko Oct 30, 2019

codecov-io commented Oct 30, 2019 •

edited

Loading

anovakovic01 Oct 31, 2019

anovakovic01 Oct 31, 2019

anovakovic01 Oct 31, 2019

anovakovic01 Oct 31, 2019

anovakovic01 Oct 31, 2019

anovakovic01 Oct 31, 2019

anovakovic01 left a comment

drasko left a comment

drasko left a comment

manuio commented Oct 31, 2019 •

edited

Loading

MF-922 - Add UpdateUser endpoint #923

MF-922 - Add UpdateUser endpoint #923

Conversation

manuio commented Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 30, 2019 • 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

anovakovic01 left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

manuio commented Oct 31, 2019 • edited Loading

manuio commented Oct 30, 2019 •

edited

Loading

codecov-io commented Oct 30, 2019 •

edited

Loading

manuio commented Oct 31, 2019 •

edited

Loading