Skip to content

fix: ensure user always belongs to an organization #9781

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 19, 2023
Merged

fix: ensure user always belongs to an organization #9781

merged 1 commit into from
Sep 19, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Sep 19, 2023

This PR modifies userOrganizationIDs to assume that the user should always belong to an organization.

Note:

I'm trying to bisect the problem with failing e2e tests when /api/v2/users/me does not return organization IDs:

2023-09-18T14:26:13.8439845Z [stdout] [restart workspace with ephemeral parameters]: [onResponse] url=http://localhost:3000/api/v2/users/me status=200 body={"id":"493bc069-93c0-4576-b3c1-48fd4674989d","username":"admin","email":"admin@coder.com","created_at":"2023-09-18T14:24:41.251247Z","last_seen_at":"2023-09-18T14:24:41.426339Z","status":"active","organization_ids":[],"roles":[{"name":"owner","display_name":"Owner"}],"avatar_url":"","login_type":"password"}

@mtojek mtojek self-assigned this Sep 19, 2023
@mtojek mtojek changed the title fix: user should always belong to an organization fix: ensure user always belongs to an organization Sep 19, 2023
mtojek
mtojek commented Sep 19, 2023
View reviewed changes
coderd/users.go
@@ -1165,8 +1165,8 @@ func convertUsers(users []database.User, organizationIDsByUserID map[uuid.UUID][

func userOrganizationIDs(ctx context.Context, api *API, user database.User) ([]uuid.UUID, error) {
organizationIDsByMemberIDsRows, err := api.Database.GetOrganizationIDsByMemberIDs(ctx, []uuid.UUID{user.ID})
if errors.Is(err, sql.ErrNoRows) || len(organizationIDsByMemberIDsRows) == 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that len(organizationIDsByMemberIDsRows) == 0 will never happen against the real database, and dbfake.go implementation handles this case:

	if len(getOrganizationIDsByMemberIDRows) == 0 {
		return nil, sql.ErrNoRows
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

Found a panic from this when missing org ids. The problem is that real db queries only return sql.ErrNoRows on :one queries, while :many just return zero rows without error.

@mtojek mtojek marked this pull request as ready for review September 19, 2023 14:10
@mtojek mtojek requested review from Emyrk and kylecarbs September 19, 2023 14:10
Emyrk
Emyrk approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Fair. If this is thrown, then the bug is elsewhere.

@mtojek mtojek merged commit ceb52ac into main Sep 19, 2023
@mtojek mtojek deleted the fix-e2e-1 branch September 19, 2023 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@coadler coadler coadler left review comments

@Emyrk Emyrk Emyrk approved these changes

@kylecarbs kylecarbs Awaiting requested review from kylecarbs

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mtojek @Emyrk @coadler