Refactor UpdateRole and add display names to invite-related responses #3689

rdimitrov · 2024-06-21T15:35:50Z

Summary

The following PR includes a few updates on the user management implementation.

The changes that are introduced are:

Moved the invite update logic to happen under the UpdateRole handler instead of being part of the AssignRole handler
Added a few display names based on the FE feedback
Reflected these changes on the CLI
Refactored the part of create/deleting an invitation to ensure proper behaviour
Added a protection so minder discards processing the invite if the user tries to accept an invite for a role he already has and another one where we delete his current role assignment and accept the new one ensuring he should have only one assignment per project.
Fixed the missing invite code in ListRoleAssignments and a bug where the Sponsor field was empty
Updated the minder role grant list command to also print current invitations along with current role assignments
Removed the checks for flags.IDPResolver

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Locally.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

coveralls · 2024-06-24T13:08:59Z

coverage: 52.497% (-0.2%) from 52.736%
when pulling 7808543 on user-fixes
into ef45f63 on main.

internal/controlplane/handlers_authz.go

coveralls · 2024-06-24T14:47:26Z

coverage: 52.492% (-0.2%) from 52.736%
when pulling 60711c5 on user-fixes
into b5398b9 on main.

internal/controlplane/handlers_authz.go

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

jhrozek

A bunch of questions inline

jhrozek · 2024-06-24T15:11:45Z

internal/controlplane/handlers_authz.go

-	prj := targetPrj.String()
-	// Get all invitations for this email, project and role
-	inviteToRemove, err := s.store.GetInvitationByEmailAndProjectAndRole(ctx, db.GetInvitationByEmailAndProjectAndRoleParams{
+	// Get all invitations for this email and project


A bit of a side-note, but I wonder if extracting all those non-exported methods like removeInvite, removeRole etc into interfaces (RoleService and InviteService maybe?) might make testing the area of code easier and would encapsulate the code better to only rely on what they need and not the whole big Server structure. Not for this PR though.

👍 on this suggestion (and to do it in a separate PR)

I think we could have a single big user-management service, at least for now.

internal/controlplane/handlers_authz.go

jhrozek · 2024-06-24T15:16:18Z

internal/controlplane/handlers_user.go

 	}

 	// Get the list of invitations for the user
-	invites, err := s.store.GetInvitationsByEmail(ctx, token.Email())
+	invites, err := s.store.GetInvitationsByEmail(ctx, tokenEmail)


This works only for invites for the address the user has in their token right?

Is there a way to list the invites by the identity instead and look up the identity by e-mail?

Should we expect that the same user identity can have multiple emails in their token (they get that from Keycloak right?) The idea was to check that for listing invitations, but still allow the user to accept/decline invitations by their code (without cross-checking the email)

OK, maybe I'm missing what is the list invitations for. Is it supposed to be used by the user after they had logged in? Or by the UI?

I'm concerned about 2 things with respect to the invitations - since we would get the e-mail from keycloak we 1) can't rely on having the e-mail available I think or 2) it could be unrelated to the e-mail where invites are supposed to be sent (e.g. my e-mail address is available but it's my personal e-mail)

OK, but looking at the invite table, I don't think we have any other data to cross-reference the invite with. So this is more to be careful where/how we use that endpoint - it might not give us a complete list I think.

Yes, this is supposed to be called from a logged in user to check their pending invitations (both cli and ui). My assumption is we can rely on having the mail in the token since I don't think we have a use case where this is not true, i.e logging through GitHub does set the primary mail there. If the invite is sent to your company email and for whatever reason you want to accept it from your minder account using your personal mail you can still do that through the accept/decline flow but you are correct that it will not be shown via ListInvitations (so far this is the intended behaviour).

Also invitations work solely through using the email(for the time being you cannot send an invite to a user using their minder account)

coveralls · 2024-06-24T15:26:35Z

coverage: 52.492% (-0.2%) from 52.736%
when pulling 1d54fdc on user-fixes
into b5398b9 on main.

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

coveralls · 2024-06-24T15:37:15Z

coverage: 52.479% (-0.3%) from 52.746%
when pulling 8cda230 on user-fixes
into 66c47f5 on main.

coveralls · 2024-06-24T15:38:02Z

coverage: 52.465% (-0.3%) from 52.746%
when pulling 8cda230 on user-fixes
into 66c47f5 on main.

internal/controlplane/handlers_authz.go

jhrozek

Only two more small comments, but non-blocking.

jhrozek · 2024-06-24T20:33:15Z

internal/controlplane/handlers_authz.go

+	identity, err := s.idClient.Resolve(ctx, sponsorUser.IdentitySubject)
+	if err != nil {
+		zerolog.Ctx(ctx).Error().Err(err).Msg("error resolving identity")
+		return nil, util.UserVisibleError(codes.NotFound, "could not find identity %q", sponsorUser.IdentitySubject)


Does this need to be a fatal error? (Ditto for getting the sponsorUser above) - could we just return some raw ID? Or do you think it's better to just return all well-formed as the UI expects it or fail?

Addressed in #3710

evankanderson

I'm fine with addressing some/all of my feedback in another PR. I am slightly concerned that our final user management will end up feeling complex to users because of the number of different states and transitions we support -- it would be nice if AssignRole could basically do something like the following:

if user in project.roles:
  # This is not "upsert", because the user already exists.
  project.update_role(user, role)
else:
  project.upsert_invite(user, role)

cmd/cli/app/project/role/role_grant_list.go

cmd/cli/app/project/role/role_update.go

evankanderson · 2024-06-24T19:19:40Z

cmd/cli/app/project/role/role_update.go

+		t.Render()
+		return nil
+	}
+	// Otherwise, print the role assignments if it was about updating a role


This feels a little weird -- it feels like we should either show the output of role grant list, or only include details for the current invitation / grant.

In particular, if we're updating a grant, we will get back the code as a one-time thing, and we should specifically print something like:

Created an invite for $EMAIL to $ROLE on $PROJECT. They can access it via $URL, or with $CODE.

Where $URL is something that includes the code and will redirect them to the Stacklok UI if needed.

(Some part of this could be a TODO for later...)

Partially addressed it in #3710

evankanderson · 2024-06-24T19:21:15Z

cmd/cli/app/project/role/role_update.go

+	updateCmd.Flags().StringP("sub", "s", "", "subject to update role access for")
+	updateCmd.Flags().StringP("email", "e", "", "email to send invitation to")


I'm not a big fan of having separate sub and email parameters -- can we make this just one parameter (e.g. sub), and then have the backend determine whether this should be an invite or just a role grant?

That avoids issues where (for example), the user is already a project member, but then the admin gets an invite code when it should have been a direct grant.

cmd/cli/app/project/role/role_update.go

evankanderson · 2024-06-24T20:02:02Z

internal/controlplane/handlers_authz.go

+	// Validate the subject and email - decide if it's about updating an invitation or a role assignment
+	if sub == "" && email != "" {


IMO, this feels like it should be "if specified user is not in current OpenFGA roles, then look for invitations..."

But I'm okay leaving it for now.

evankanderson · 2024-06-24T20:38:37Z

internal/controlplane/handlers_authz.go

+func (s *Server) updateInvite(
+	ctx context.Context,
+	targetProject uuid.UUID,
+	authzRole authz.Role,
+	email string,
+) (*minder.UpdateRoleResponse, error) {


I wish this could be an upsert... I'm assuming that doesn't make sense given our RPCs, though, right?

evankanderson · 2024-06-24T20:39:09Z

internal/controlplane/handlers_authz.go

+		return nil, status.Errorf(codes.Internal, "error getting invitations: %v", err)
+	}
+
+	// Exit early if there are no or multiple existing invitations for this email and project


What's the recovery method if there are multiple existing invites? Is this something a user can resolve on their own?

Yes. if this happens the user can list all invitations and remove the ones that are conflicting. We do have protections against it when creating invites but still a valid point 👍

evankanderson · 2024-06-24T20:43:52Z

internal/controlplane/handlers_authz.go

+	// At this point, there should be exactly 1 invitation. We should either update its expiration or
+	// discard it and create a new one


I'm confused about this logic, and when we extend an invite vs re-create it. I think this code does:

User A invites foo@email.com to be a Viewer

8 days pass (expires) and then User A invites foo@email.com as a Viewer again (Update)

2 days pass and User A updates the invite for foo@email.com to Editor (Replace)

5 days pass and User B updates the invite for foo@email.com to Editor (Update)

12 days pass (expires) and User C renews the invite for foo@email.com to Editor (Update)

Is that correct? Why do we preserve the original invite for users B and C, but reset the creation date when A updates the role?

Do we need both a creation and and an update for the invitation?

internal/controlplane/handlers_authz.go

rdimitrov requested a review from a team as a code owner June 21, 2024 15:35

rdimitrov force-pushed the user-fixes branch from 46aeb87 to 65f52ff Compare June 21, 2024 19:37

This was referenced Jun 24, 2024

Proto updates related to user management #3696

Merged

Add helper functions for getting token and mail from context #3697

Merged

rdimitrov force-pushed the user-fixes branch from f9a20cf to f13d756 Compare June 24, 2024 09:43

rdimitrov mentioned this pull request Jun 24, 2024

Add helper functions for checking/setting invite expiration #3698

Merged

10 tasks

rdimitrov force-pushed the user-fixes branch from f13d756 to e0a51ff Compare June 24, 2024 10:07

rdimitrov added 2 commits June 24, 2024 15:43

Refactor UpdateRole and add display names to invite-related responses

7b943cb

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Update unit tests for user management

7808543

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

rdimitrov force-pushed the user-fixes branch from e0a51ff to 7808543 Compare June 24, 2024 13:00

Merge branch 'main' into user-fixes

60711c5

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Show resolved Hide resolved

rdimitrov self-assigned this Jun 24, 2024

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Outdated Show resolved Hide resolved

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Outdated Show resolved Hide resolved

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Show resolved Hide resolved

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Show resolved Hide resolved

Create the invite after resolving the project and sponsor

1d54fdc

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

jhrozek reviewed Jun 24, 2024

View reviewed changes

rdimitrov added 2 commits June 24, 2024 18:28

Use DB transaction to ensure that the invitation is updated atomically

4eb2fe6

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Merge branch 'main' into user-fixes

8cda230

jhrozek reviewed Jun 24, 2024

View reviewed changes

internal/controlplane/handlers_authz.go Show resolved Hide resolved

jhrozek approved these changes Jun 24, 2024

View reviewed changes

evankanderson approved these changes Jun 24, 2024

View reviewed changes

rdimitrov merged commit 7212761 into main Jun 25, 2024
21 checks passed

rdimitrov deleted the user-fixes branch June 25, 2024 07:18

rdimitrov mentioned this pull request Jun 25, 2024

Do not return the invite code when listing role assignments and set display name defaults #3710

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor UpdateRole and add display names to invite-related responses #3689

Refactor UpdateRole and add display names to invite-related responses #3689

rdimitrov commented Jun 21, 2024 •

edited

Loading

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

jhrozek left a comment

jhrozek Jun 24, 2024

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

jhrozek Jun 24, 2024

jhrozek Jun 24, 2024

rdimitrov Jun 24, 2024

jhrozek Jun 24, 2024

jhrozek Jun 24, 2024

rdimitrov Jun 24, 2024

rdimitrov Jun 24, 2024

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

jhrozek left a comment

jhrozek Jun 24, 2024

rdimitrov Jun 25, 2024

evankanderson left a comment

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

rdimitrov Jun 25, 2024

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

evankanderson Jun 24, 2024

rdimitrov Jun 25, 2024

evankanderson Jun 24, 2024

		updateCmd.Flags().StringP("sub", "s", "", "subject to update role access for")
		updateCmd.Flags().StringP("email", "e", "", "email to send invitation to")

		// Validate the subject and email - decide if it's about updating an invitation or a role assignment
		if sub == "" && email != "" {

		// At this point, there should be exactly 1 invitation. We should either update its expiration or
		// discard it and create a new one

Refactor UpdateRole and add display names to invite-related responses #3689

Refactor UpdateRole and add display names to invite-related responses #3689

Conversation

rdimitrov commented Jun 21, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

jhrozek 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

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

coveralls commented Jun 24, 2024

jhrozek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson 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

rdimitrov commented Jun 21, 2024 •

edited

Loading