HTTP API: Allow assigning a specific organization when creating a new user #21775

Sytten · 2020-01-28T06:08:33Z

What this PR does / why we need it:
With this PR, the API can now create user and directly assign it to an organization. Previously, we had to:

Create the user
Remove it from the main org
Add it to another organization

This is very common task for multi-tenant grafana and it is currently prone to errors and edge cases (for example a user could get stuck without any org).

Which issue(s) this PR fixes:

Fixes #7114

aknuds1

Looks generally good to me, but we need to decide if we want the functionality.

Sytten · 2020-01-28T15:14:04Z

The way I see it: Grafana wants to go down the route of observability for large enterprise, companies like logz.io are starting to offer managed versions of grafana to their users for this purpose (potential stream of revenue), this requires management of a multi-tenant grafana which is currently quite hard (I have done it myself), this PR helps a bit in that direction (and is clearly wanted by the feature request).

Sytten · 2020-02-03T17:08:49Z

Any update? @papagian @marefr

papagian

Great, thank you for your contribution!
In addition to my inline comment, I would love to include some test for admin/users endpoint.
More specifically I would like to add the following tests in admin_users_test.go:

create user without providing orgId
create user by providing orgId
create user by providing non-existing orgId. To do so you the mock the response of CreateUserCommand to return ErrOrgNotFound similarly to what happens here.

Also for adding the above tests you need to implement a function
func adminCreateUserScenario(desc string, url string, routePattern string, cmd dtos.AdminCreateUserForm, fn scenarioFunc)
similar to the other scenarios defined in admin_users_test.go.
As an POST example you can check this one.

Please feel free to ping me if you need some additional information.

papagian · 2020-02-10T09:40:09Z

pkg/services/sqlstore/user_test.go

+				So(err, ShouldNotBeNil)
+			})
+
+			setting.AutoAssignOrg = false


You could do this inside a defer block at the beginning of the test just after setting AutoAssignOrg to true, like:

defer func() { setting.AutoAssignOrg = false }()

@papagian agree, but maybe it would be better to restore to the original value of setting.AutoAssignOrg, just in case it's true? Assumptions can be dangerous.

good point, that would be even better.

stale · 2020-02-24T21:30:49Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

aknuds1 · 2020-02-24T21:55:37Z

@Sytten Have you considered @papagian's requested changes?

Sytten · 2020-02-25T06:22:13Z

Sorry had a lot going on, I will try to get it do tomorrow

aknuds1 · 2020-02-25T06:59:36Z

Thanks!

bergquist · 2020-02-27T19:54:21Z

I'm not sure what happened here but it seems like there has been some kind of mistake of keeping up with the master branch. Please fix this PR to only contain your commit or open a new PR with only your commit :)

Sytten · 2020-02-28T00:03:57Z

Yeah its weird, most likely related to the issues github had today. I will fix it.

Sytten · 2020-02-28T03:51:06Z

@aknuds1 @papagian If you can review the new tests and let me know if thats ok for you, thanks!

Sytten · 2020-03-07T18:27:06Z

@aknuds1 @papagian just a small reminder of this PR

aknuds1 · 2020-03-09T13:53:50Z

For the future, could you please avoid force pushing to PRs? The comments from previous reviews have been wiped. We squash PRs anyway on commits, so cleaning branch history is unnecessary.

Edit: Ah, I guess it was in order to fix the issue mentioned by Carl. Never mind then.

aknuds1 · 2020-03-09T13:56:50Z

@marefr @bergquist Can you please weigh in on whether we want this new functionality? I remember we discussed before, but we didn't reach a conclusion I think.

papagian

Sorry for the delayed reply. Thanks for the modifications, I have some minor suggestions for fixing the tests.

pkg/api/admin_users.go

papagian · 2020-03-09T15:32:14Z

pkg/api/admin_users_test.go

+			userLogin = cmd.Login
+			orgId = cmd.OrgId
+
+			if orgId == 1000 {


I would suggest to declare a const NonExistingOrgID

@papagian Why should the const be exported? I.e., if it's capitalized it's exported. We should avoid exporting symbols unless there's a real need for it.

no reason to be exported

In that case, it should be named nonExistingOrgID :)

sorry for being unclear; I thought it goes without saying

Yeah never mind, I just wanted to be explicit so there's no room for misunderstanding.

papagian · 2020-03-09T16:04:57Z

pkg/services/sqlstore/user_test.go

+				}
+
+				err := CreateUser(context.Background(), cmd)
+				So(err, ShouldNotBeNil)


Additionally, you can check that the error is ErrOrgNotFound

pkg/api/admin_users_test.go

papagian · 2020-03-09T16:45:06Z

pkg/api/admin_users_test.go

+			createCmd := dtos.AdminCreateUserForm{
+				Login:    TestLogin,
+				Password: TestPassword,
+				OrgId:    1000,


I would suggest using the constant nonExistingOrgID

pkg/api/admin_users_test.go

Sytten · 2020-03-09T18:41:40Z

@papagian thanks for the review, I responded where I don't agree. I will fix the other comments. I will wait for your reply!

aknuds1

LGTM, although we should resolve the discussions.

stale · 2020-03-24T10:50:45Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Sytten · 2020-03-24T12:59:49Z

no worries I will get to fix it, didnt have time lately with covid and everything

bergquist · 2020-03-24T13:13:09Z

@Sytten Great to hear that you're still up for it. I hope your safe given current circumstances. No rush. take your time :)

stale · 2020-04-07T14:14:55Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

bergquist · 2020-04-07T14:24:14Z

Go away stale bot!

…-org-association

aknuds1

LGTM

Sytten requested a review from oddlittlebird as a code owner January 28, 2020 06:08

aknuds1 added area/backend area/backend/api pr/external This PR is from external contributor type/feature labels Jan 28, 2020

aknuds1 reviewed Jan 28, 2020

View reviewed changes

marefr requested review from marefr and papagian January 29, 2020 14:31

papagian requested changes Feb 10, 2020

View reviewed changes

stale bot added the stale Issue with no recent activity label Feb 24, 2020

aknuds1 self-assigned this Feb 24, 2020

stale bot removed the stale Issue with no recent activity label Feb 24, 2020

ryantxu requested a review from marcusolsson as a code owner February 27, 2020 16:54

Sytten added 3 commits February 27, 2020 22:49

Allow API to assign new user to a specific organization

612b265

Add defer block to test

8b2764f

Add API tests and return 400 instead of 500 for bad orgId

43a5b2d

aknuds1 requested a review from papagian March 9, 2020 13:55

papagian requested changes Mar 9, 2020

View reviewed changes

aknuds1 approved these changes Mar 10, 2020

View reviewed changes

stale bot added the stale Issue with no recent activity label Mar 24, 2020

stale bot removed the stale Issue with no recent activity label Mar 24, 2020

stale bot added the stale Issue with no recent activity label Apr 7, 2020

stale bot removed the stale Issue with no recent activity label Apr 7, 2020

papagian added 2 commits April 15, 2020 10:59

Merge remote-tracking branch 'origin/master' into feature/user-create…

df7ba96

…-org-association

Minor test improvements

f44fd1a

Sytten requested a review from a team as a code owner April 15, 2020 08:24

papagian approved these changes Apr 15, 2020

View reviewed changes

aknuds1 approved these changes Apr 15, 2020

View reviewed changes

papagian added the add to changelog label Apr 15, 2020

papagian added this to the 7.0 milestone Apr 15, 2020

papagian merged commit d721dd1 into grafana:master Apr 15, 2020

papagian added this to Done in Backend Platform Squad via automation Apr 15, 2020

marefr changed the title ~~Allow API to assign new user to a specific organization~~ HTTP API: Allow assigning a specific organization when creating a new user Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP API: Allow assigning a specific organization when creating a new user #21775

HTTP API: Allow assigning a specific organization when creating a new user #21775

Sytten commented Jan 28, 2020

aknuds1 left a comment

Sytten commented Jan 28, 2020

Sytten commented Feb 3, 2020

papagian left a comment

papagian Feb 10, 2020

aknuds1 Feb 10, 2020 •

edited

papagian Feb 10, 2020

stale bot commented Feb 24, 2020

aknuds1 commented Feb 24, 2020

Sytten commented Feb 25, 2020

aknuds1 commented Feb 25, 2020

bergquist commented Feb 27, 2020

Sytten commented Feb 28, 2020

Sytten commented Feb 28, 2020

Sytten commented Mar 7, 2020

aknuds1 commented Mar 9, 2020 •

edited

aknuds1 commented Mar 9, 2020

papagian left a comment

papagian Mar 9, 2020

aknuds1 Mar 10, 2020

papagian Mar 10, 2020

aknuds1 Mar 10, 2020

papagian Mar 10, 2020 •

edited

aknuds1 Mar 10, 2020

papagian Mar 9, 2020

papagian Mar 9, 2020 •

edited

Sytten commented Mar 9, 2020

aknuds1 left a comment

stale bot commented Mar 24, 2020

Sytten commented Mar 24, 2020

bergquist commented Mar 24, 2020

stale bot commented Apr 7, 2020

bergquist commented Apr 7, 2020

aknuds1 left a comment

Navigation Menu

HTTP API: Allow assigning a specific organization when creating a new user #21775

HTTP API: Allow assigning a specific organization when creating a new user #21775

Conversation

Sytten commented Jan 28, 2020

aknuds1 left a comment

Choose a reason for hiding this comment

Sytten commented Jan 28, 2020

Sytten commented Feb 3, 2020

papagian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Feb 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Feb 24, 2020

aknuds1 commented Feb 24, 2020

Sytten commented Feb 25, 2020

aknuds1 commented Feb 25, 2020

bergquist commented Feb 27, 2020

Sytten commented Feb 28, 2020

Sytten commented Feb 28, 2020

Sytten commented Mar 7, 2020

aknuds1 commented Mar 9, 2020 • edited

aknuds1 commented Mar 9, 2020

papagian 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

papagian Mar 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papagian Mar 9, 2020 • edited

Choose a reason for hiding this comment

Sytten commented Mar 9, 2020

aknuds1 left a comment

Choose a reason for hiding this comment

stale bot commented Mar 24, 2020

Sytten commented Mar 24, 2020

bergquist commented Mar 24, 2020

stale bot commented Apr 7, 2020

bergquist commented Apr 7, 2020

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 Feb 10, 2020 •

edited

aknuds1 commented Mar 9, 2020 •

edited

papagian Mar 10, 2020 •

edited

papagian Mar 9, 2020 •

edited