Update AssignRole, RemoveRole and add new UpdateRole handlers

[rdimitrov]

Summary

The following PR update AssignRole, RemoveRole and add new UpdateRole handlers

Fixes #(related issue)

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

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

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.

[rdimitrov]

Issue for creating unit tests for these handlers - #3673

[rdimitrov]

Obviously don't merge as I haven't updated the unit tests but do review the code if you decide so 👍

[puerco]

Should this be idempotent and just return 200 OK?

[rdimitrov]

It is what we do for other handlers, such as - https://github.com/stacklok/minder/blob/d08b8f68f4a308c3c8c69f49fbdb81710596ddcb/internal/controlplane/handlers_profile.go#L95

Let's discuss if we should change this and we can do that in a follow-up for all similar use cases so it's consistent 👍

[puerco]

if isEmail() returns false here I think we should return an internal error as it points to an internal data inconsistency already present in the database.

[rdimitrov]

The email in this case is received as an argument that is part of the request message, so I think it makes sense to validate it and return codes.InvalidArgument if it doesn't look alright

[puerco]

Here we should check the invite expiration date before determining if we should update it. We want to avoid replaying an invite in case someone gets access to an old email for example

[rdimitrov]

Let me give some context. So the lifespan of the invites should be 7 days for expiration, but we'll keep them for 30 days before deleting them entirely so an admin can update them once they see they have expired. This bumps their expires time only so previous send links continue to work and this is the intended behavior.

This flow happens when an administrator invites someone else again and there was an existing invitation (meaning it is less than 30 days old, otherwise it should have been deleted).

The scenarios are:

• If the invite expires <= 7 days old, they are fine and can accept.
• If the invite is 7 days < expires < 30 days, the invite will not work unless the admin went and updated it through this flow in the last 7 days
• If the invite is 30 < expires, it shouldn't be present in the db, that is for both the admin and the invitee.

In that sense, I don't think we need to check the expiration because this flow is called by the administrator explicitly and is about updating the expiration of an invite.

[puerco]

Compile the email regexp just once to reuse it across calls

Suggested change

	// isEmail checks if the subject is an email address or not
	func isEmail(subject string) bool {
	// Define the regular expression for validating an email address
	const emailRegexPattern = `^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$`
	emailRegex := regexp.MustCompile(emailRegexPattern)
	var emailRegexPattern *regexp.Regexp
	// isEmail checks if the subject is an email address or not
	func isEmail(subject string) bool {
	// Define the regular expression for validating an email address
	const emailRegexPattern = `^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$`
	if emailRegexPattern == nil {
	emailRegex = regexp.MustCompile(emailRegexPattern)
	}

[rdimitrov]

Good tip! Let me update this manually though as I think some of the env names in the suggestion are not correct.

[jhrozek]

@puerco the email regex was actually removed to let the e-mail sending service verify it, not minder (it's unlikely we'd ever get the regex right for all weird cases)

[puerco]

Just as in the other, if somehow an un parseable role is being returned by the client this points to an internal inconsistency, not an argument from the client

Suggested change

	return nil, util.UserVisibleError(codes.InvalidArgument, err.Error())
	return nil, util.UserVisibleError(codes.Internal, err.Error())

[rdimitrov]

Thanks! I agree on this since the respond comes from calling openfga 👍

[puerco]

I think this should also be checking for the role? We should match user + role before calling out that the assignment already exists

[rdimitrov]

It was decided that a user can have only one role within a project so in this case this should be enough 👍

[puerco]

And here I think it is a case for InvalidArgument as it is querying using the input from the user :D

Suggested change

	return nil, status.Errorf(codes.Internal, "error getting project: %v", err)
	return nil, status.Errorf(codes.InvalidArgument, "error getting project: %v", err)

[rdimitrov]

We have a deal on this one! 😃

[jhrozek]

but in this case it's some unexpected error, I think Internal is OK in this case? e.g. the database connection was lost and such

[jhrozek]

@puerco not to roll over your review, but since the frontend team was waiting on the API to land, I did a review of the PR with @rdimitrov. If you feel like your comments need addressing let's bring them up as follow-up PRs

[rdimitrov]

@jhrozek, @puerco - thanks for the reviews! 🙏