Add API endpoint definitions for Minder invitation flow #3450

evankanderson · 2024-05-28T21:22:54Z

Summary

Define API endpoints for user invitations (nonce + destination email address). There is currently no backing implementation. Attemptedto add enough documentation to the endpoints that the semantics were clear.

Fixes #3448

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

Ran make gen, since this is only an IDL and code generation change.

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.

coveralls · 2024-05-28T21:31:08Z

coverage: 52.114% (+0.005%) from 52.109%
when pulling 0fd5822 on evankanderson:3448-implement-api-endpoints-to-support-inviting-users-to-projects
into 5065d65 on stacklok:main.

jhrozek · 2024-05-29T12:37:21Z

proto/minder/v1/minder.proto

+        };
+
+        option (rpc_options) = {
+            target_resource: TARGET_RESOURCE_USER


Can you remind me what is the default relation? I see that we don't specify one with the other methods in the same service either.

The default 0 value is TARGET_RESOURCE_UNSPECIFIED. The other options are TARGET_RESOURCE_NONE and TARGET_RESOURCE_PROJECT.

IIRC, except for TARGET_RESOURCE_PROJECT, all the other options currently use the same Authz code. Additionally, all of these requests operate on an implicit subject of the logged in user, rather than operating on a project, provider, repository, etc.

jhrozek · 2024-05-29T12:39:14Z

proto/minder/v1/minder.proto

+    // nonce is the nonce of the invitation to resolve.
+    string nonce = 1;
+    // accept is true if the invitation is accepted, false if it is rejected.
+    bool accept = 2;


Honest question: Could we ever have another action? I wonder if the bool would bite us if we ever need another action like e.g. view (but I guess we can add another field then slowly deprecate this one)

I thought about this. 😁 I'm generally suspicious of boolean fields, so I don't feel strongly about keeping this, but I couldn't construct a meaningful difference between a bool and an enum, so I opted for simplicity.

This RPC is about retiring an invitation. It feels like accept or not accept are the two options that cause the invitation to be removed. I can imagine a "delegate" or "forward" action in the future, but I think I would express those with accept = false and a new field indicating the new recipient of the invitation.

I welcome @kantord sharing perspective about how using a boolean here (with default false) would affect the UI implementation.

jhrozek

I think the API looks good, just the CI failures must be resolved. I added some questions inline, but they are mostly for my education.

kantord · 2024-05-29T13:33:33Z

pkg/api/openapi/minder/v1/minder.swagger.json

@@ -2396,6 +2403,57 @@
          "UserService"
        ]
      }
+    },
+    "/api/v1/user/invitation/{nonce}": {


Suggested change

"/api/v1/user/invitation/{nonce}": {

"/api/v1/user/invitation/{invite}": {

my understanding that a more specific name for the nonce could be "invitation code" or "invite" or "invite code".

Let's call it code here, so that we don't conflate it with e.g. a database record.

kantord · 2024-05-29T13:36:02Z

pkg/api/openapi/minder/v1/minder.swagger.json

+          },
+          {
+            "name": "accept",
+            "description": "accept is true if the invitation is accepted, false if it is rejected.",


hmm, do we actually have a use case for rejecting an invitation? 🤔 if I receive an invitation for something that I don't recognize/was not intended for me I will probably not click any links in the email 🤔

but I guess this is not something that would negatively affect FE

The invitation may also show up in the invite list (presented in the UI or as a prompt on the CLI) if it's sent to the primary email address associated with your account. You might want to be able to dismiss that invitation if you think it's junk.

kantord · 2024-05-29T13:37:12Z

pkg/api/openapi/minder/v1/minder.swagger.json

+          "type": "string",
+          "description": "project is the project to which the user is invited."
+        },
+        "nonce": {


does this mean that we are returning the nonce/invitation code, so basically the someone with admin permissions is able to accept someone else's invitation if they request the code through the API? 🤔

Yes, this does mean that a user with the ability to grant access to a project can create an invitation and immediately use the code to accept that invitation and grant themselves that permission. They could also just grant themselves that permission without an invitation.

This is to support cases like "share the invitation via slack" or for self-hosted Minder environments that haven't set up email sending but want to invite another user to a project.

kantord · 2024-05-29T13:42:14Z

pkg/api/openapi/minder/v1/minder.swagger.json

@@ -3902,6 +4014,10 @@
          "type": "string",
          "description": "subject is the subject to which the role is assigned."
        },
+        "displayName": {


I don't know if we are in the position to release a breaking change right now, but I think it would make more sense to nest displayName under subject, somehow like this:

{ "subject": { "id": // what is now called subject "displayName": // etc } }

also I think "user" could be a simpler naming, at least as long as users are the only possible subjects for role assignment (maybe groups or projects will be also possible in the future?)

I'm not looking to make breaking API changes. Sorry! (Yeah, some of the API naming is very technical rather than friendly. /shrug)

kantord · 2024-05-29T13:48:02Z

proto/minder/v1/minder.proto

@@ -1987,12 +2028,57 @@ message RoleAssignment {
    string role = 1;


I think that one thing that we are missing is a displayName for the role, because we should display "Read-only" instead of "read_only" on the UI

(and the same applies not only for invitations but also for the endpoint that lists existing role assignments)

Look at ListRoles / ListRolesResponse and the contents of the Role message there.

I'm assuming that you're requesting that we duplicate that data into each RoleAssignment, but I'm going to push back on that, at least for right now -- from an API consumer perspective, it raises the question as to whether roles with the same name can have different descriptions.

in that response description is akin to displayName? In other words it's a short name that we can use to display in the UI? (I assumed that description is potentially some multi-line text, or at least something too long to use as a name)

I'm fine with the idea of just combining 2 responses, that is not a problem for us, but we should be aware that such design would require all clients to duplicate that logic. If we think about all of Minder as a complex system (GUI, CLI and BE) then often a good tradeoff might be to deal with that complexity in the BE side. (I don't have deep knowledge of the CLI codebase of course, I'm just assuming that the client data fetching logic must be very similar to what we have in the web gui)

kantord · 2024-05-29T14:00:54Z

going by the design, for invitation we need to display:

user email
who invited that user
when they were invited
their role, using display name
the color of the role

I think it's possible that we need to change the design to adapt to the API in some respects and not vice versa, at least as long as design and product are aware of it as well. cc. @peppescg were there any already agreed changes here that I am not aware of?

Regarding the color and display name of the roles, I believe that we could hardcode those on FE side, at least as long as custom role creation is not permitted. But probably it is better to hardcode it on BE side for various reasons such as having a unified color in all clients and giving BE the full control to change role config without help from FE etc. I think it would be a more future proof design

evankanderson · 2024-05-29T21:28:41Z

I think it's possible that we need to change the design to adapt to the API in some respects and not vice versa, at least as long as design and product are aware of it as well. cc. @peppescg were there any already agreed changes here that I am not aware of?

I think it's reasonable to add the additional fields to ListRoleAssignmentsResponse -> RoleAssignment, except for the color value. If we think it's important that different clients use the same color scheme, we could add that to ListRolesResponse, but I think that's a presentation detail that doesn't need to be consistent between clients.

…endpoints-to-support-inviting-users-to-projects

rdimitrov · 2024-05-30T07:35:09Z

proto/minder/v1/minder.proto

+    string email = 2;
+    // project is the project to which the user is invited.
+    string project = 3;
+}


Should we have one more field with the invitation code for which this response is confirmed? I'm okay with not useful, so just asking.

I'm inclined not to repeat data from the request into the response here, and the code is in the request.

To avoid seeming hypocrisy, something like "GetProject" returns a project object, which may happen to contain the ID of the project that's also in the request. We "repeat" the project ID in the response because the ID is part of the object, and we want to transfer consistent objects (REST = representational state transfer) where possible, rather than incomplete subsets.

peppescg · 2024-05-30T08:52:45Z

I think it's reasonable to add the additional fields to ListRoleAssignmentsResponse -> RoleAssignment, except for the color value. If we think it's important that different clients use the same color scheme, we could add that to ListRolesResponse, but I think that's a presentation detail that doesn't need to be consistent between clients.

I think that the color is a presentational concern, it is a logic that should be handle on the client not on API layer, we can create a map role/color as we made for severity

About role @evankanderson my concern is that if the role is not an enum | union type, and it is only a string, we cannot having a color map on FE, cause it is a dynamic value.
The only way to avoid put the enum role in the ListInvitations, is to calling before this API a Role API that return the list of possibles roles, but we can refer to the same role model in several part of the APIs, so avoiding to call a role API before the invitations list

kantord · 2024-05-30T09:08:37Z

I think it's reasonable to add the additional fields to ListRoleAssignmentsResponse -> RoleAssignment, except for the color value. If we think it's important that different clients use the same color scheme, we could add that to ListRolesResponse, but I think that's a presentation detail that doesn't need to be consistent between clients.

I think that the color is a presentational concern, it is a logic that should be handle on the client not on API layer, we can create a map role/color as we made for severity

what about the following concern:

if the list or configuration of roles changes on BE side, the hardcoded mapping on FE side will be outdated, resulting in a production error/flaw until FE released a fix
we should try to display the same color across clients, so the best way to make sure that there are no discrepancies is to couple it in the very beginning

Another thing is that perhaps in the future we might want to support custom roles, or perhaps some big client will ask us to create different roles just specifically for them.

kantord · 2024-05-30T09:10:29Z

I think it's possible that we need to change the design to adapt to the API in some respects and not vice versa, at least as long as design and product are aware of it as well. cc. @peppescg were there any already agreed changes here that I am not aware of?

I think it's reasonable to add the additional fields to ListRoleAssignmentsResponse -> RoleAssignment, except for the color value. If we think it's important that different clients use the same color scheme, we could add that to ListRolesResponse, but I think that's a presentation detail that doesn't need to be consistent between clients.

yeah, I think that is reasonable, at least until we assume that there will be only a small set of built in roles and nothing more complicated 🤔

well I guess if we just assume that rules are a fixed small set, and we don't care about consistency across clients then we can leave out the color code completely like @peppescg says

peppescg · 2024-05-30T09:15:04Z

if the list or configuration of roles changes on BE side, the hardcoded mapping on FE side will be outdated, resulting in a production error/flaw until FE released a fix

This is a communication/process issue IMO, and also we have the openapi code generation, I am expecting that if the enum | union type change we will have a not matching issue in tsc

we should try to display the same color across clients, so the best way to make sure that there are no discrepancies is to couple it in the very beginning

yup I see what you mean, and there is value on having the same color across all the clients, but let's say the opposite, if we change the color on BE and someone go crazy and use a wrong color shade that breaks the UI in term of accessibility?
For this kind of stuff IMO the FE should remain the unique source of truth in terms UI/UX.

kantord · 2024-05-30T09:25:32Z

if the list or configuration of roles changes on BE side, the hardcoded mapping on FE side will be outdated, resulting in a production error/flaw until FE released a fix

This is a communication/process issue IMO, and also we have the openapi code generation, I am expecting that if the enum | union type change we will have a not matching issue in tsc

we should try to display the same color across clients, so the best way to make sure that there are no discrepancies is to couple it in the very beginning

yup I see what you mean, and there is value on having the same color across all the clients, but let's say the opposite, if we change the color on BE and someone go crazy and use a wrong color shade that breaks the UI in term of accessibility? For this kind of stuff IMO the FE should remain the unique source of truth in terms UI/UX.

yeah, the possibility of using the wrong color is a concern too for sure. I would say that as long as the role is an enum or tagged union type (like Rust enum) then we should map it in FE, but if it's an arbitrary string we should ask the BE for the color instead of trying to map an arbitrary string with an enum value

evankanderson · 2024-05-30T18:06:09Z

if the list or configuration of roles changes on BE side, the hardcoded mapping on FE side will be outdated, resulting in a production error/flaw until FE released a fix

This is a communication/process issue IMO, and also we have the openapi code generation, I am expecting that if the enum | union type change we will have a not matching issue in tsc

we should try to display the same color across clients, so the best way to make sure that there are no discrepancies is to couple it in the very beginning

yup I see what you mean, and there is value on having the same color across all the clients, but let's say the opposite, if we change the color on BE and someone go crazy and use a wrong color shade that breaks the UI in term of accessibility? For this kind of stuff IMO the FE should remain the unique source of truth in terms UI/UX.

yeah, the possibility of using the wrong color is a concern too for sure. I would say that as long as the role is an enum or tagged union type (like Rust enum) then we should map it in FE, but if it's an arbitrary string we should ask the BE for the color instead of trying to map an arbitrary string with an enum value

I think you're over-estimating the ability of a wire-format enum to actually enumerate all the possibilities (this is a pretty common mistake in designing protocols -- protobufs has a specific style and section on enums). The problem is that a message receiver (client or server) may have built from an older version of the interface definition than the message sender. From the sender's point of view, the new enum value is just fine. From the receiver's point of view, this is a value that's outside the known set of values, so is a violation of the "enum property".

Since the backend doesn't know things like the color scheme used by the UI (which may not be consistent between clients), it feels like the UI might want to pick a random (consistent) color for unknown role values (for example, hash the role name and use that to pick a color). These might not be consistent between clients or maybe even between page loads. It should also be a rare event, and maybe we can measure when / if it happens.

evankanderson · 2024-05-30T18:08:09Z

@kantord @peppescg -- if you buy my argument on not including colors in the backend API, can you approve the PR if my additions addressed the other data you need.

kantord · 2024-05-31T11:25:14Z

if the list or configuration of roles changes on BE side, the hardcoded mapping on FE side will be outdated, resulting in a production error/flaw until FE released a fix

This is a communication/process issue IMO, and also we have the openapi code generation, I am expecting that if the enum | union type change we will have a not matching issue in tsc

we should try to display the same color across clients, so the best way to make sure that there are no discrepancies is to couple it in the very beginning

yup I see what you mean, and there is value on having the same color across all the clients, but let's say the opposite, if we change the color on BE and someone go crazy and use a wrong color shade that breaks the UI in term of accessibility? For this kind of stuff IMO the FE should remain the unique source of truth in terms UI/UX.

yeah, the possibility of using the wrong color is a concern too for sure. I would say that as long as the role is an enum or tagged union type (like Rust enum) then we should map it in FE, but if it's an arbitrary string we should ask the BE for the color instead of trying to map an arbitrary string with an enum value

I think you're over-estimating the ability of a wire-format enum to actually enumerate all the possibilities (this is a pretty common mistake in designing protocols -- protobufs has a specific style and section on enums). The problem is that a message receiver (client or server) may have built from an older version of the interface definition than the message sender. From the sender's point of view, the new enum value is just fine. From the receiver's point of view, this is a value that's outside the known set of values, so is a violation of the "enum property".

Since the backend doesn't know things like the color scheme used by the UI (which may not be consistent between clients), it feels like the UI might want to pick a random (consistent) color for unknown role values (for example, hash the role name and use that to pick a color). These might not be consistent between clients or maybe even between page loads. It should also be a rare event, and maybe we can measure when / if it happens.

yes, that is a good point about enums and probably it's fair to say that it's best to avoid enums if there is a better alternative for this reason 🤔 tho I think that would actually be an argument in favor of returning the color from the BE 🤔

that being said despite this "false promise" aspect I think it still does have a lot of advantages. In a previous company I worked in, we had a rule that all API consumers had to maintain a simple CI pipeline for the BE repo that verifies that runs checks on the BE (building the SDK client without bugs, checking for type errors and breaking changes with the new version of the SDK etc). In this example if we had that (and I think it would be very easy for us to create it) then BE we would be warned in such situations

kantord · 2024-05-31T11:56:39Z

@kantord @peppescg -- if you buy my argument on not including colors in the backend API, can you approve the PR if my additions addressed the other data you need.

I kinda buy it. Though I think that the color codes have a meaning on their own, probably it's not the most universal/semantic representation for that meaning. I guess in the end there is no better abstract representation than the role name itself since there aren't different "categories" of roles that share a color. So I agree with not including the color in the response

kantord

💯 approve from me. We are in agreement about not including the color code and I think that the discussion about enums does not need to be in the scope of this PR

Add API endpoint definitions for Minder invitation flow

46c5c9c

evankanderson requested a review from a team as a code owner May 28, 2024 21:22

evankanderson linked an issue May 28, 2024 that may be closed by this pull request

Implement API endpoints to support inviting users to projects #3448

Closed

jhrozek reviewed May 29, 2024

View reviewed changes

evankanderson requested a review from kantord May 29, 2024 13:08

kantord reviewed May 29, 2024

View reviewed changes

evankanderson added 2 commits May 29, 2024 14:32

Merge remote-tracking branch 'upstream/main' into 3448-implement-api-…

be254e8

…endpoints-to-support-inviting-users-to-projects

Address kantord@ feedback, fix accidentally-included file

0fd5822

rdimitrov reviewed May 30, 2024

View reviewed changes

kantord approved these changes May 31, 2024

View reviewed changes

evankanderson merged commit 2d6a0ca into mindersec:main May 31, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API endpoint definitions for Minder invitation flow #3450

Add API endpoint definitions for Minder invitation flow #3450

evankanderson commented May 28, 2024

coveralls commented May 28, 2024 •

edited

Loading

jhrozek May 29, 2024

evankanderson May 29, 2024

evankanderson May 29, 2024

jhrozek May 29, 2024

evankanderson May 29, 2024

jhrozek left a comment

kantord May 29, 2024

evankanderson May 29, 2024

kantord May 29, 2024

evankanderson May 29, 2024

kantord May 30, 2024

kantord May 29, 2024

evankanderson May 29, 2024

kantord May 29, 2024

evankanderson May 29, 2024

kantord May 29, 2024

evankanderson May 29, 2024

kantord May 30, 2024

kantord May 30, 2024

kantord commented May 29, 2024

evankanderson commented May 29, 2024

rdimitrov May 30, 2024

evankanderson May 30, 2024

peppescg commented May 30, 2024 •

edited

Loading

kantord commented May 30, 2024

kantord commented May 30, 2024

peppescg commented May 30, 2024 •

edited

Loading

kantord commented May 30, 2024

evankanderson commented May 30, 2024

evankanderson commented May 30, 2024

kantord commented May 31, 2024

kantord commented May 31, 2024

kantord left a comment

	"/api/v1/user/invitation/{nonce}": {
	"/api/v1/user/invitation/{invite}": {

		@@ -1987,12 +2028,57 @@ message RoleAssignment {
		string role = 1;

Add API endpoint definitions for Minder invitation flow #3450

Add API endpoint definitions for Minder invitation flow #3450

Conversation

evankanderson commented May 28, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented May 28, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantord commented May 29, 2024

evankanderson commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppescg commented May 30, 2024 • edited Loading

kantord commented May 30, 2024

kantord commented May 30, 2024

peppescg commented May 30, 2024 • edited Loading

kantord commented May 30, 2024

evankanderson commented May 30, 2024

evankanderson commented May 30, 2024

kantord commented May 31, 2024

kantord commented May 31, 2024

kantord left a comment

Choose a reason for hiding this comment

coveralls commented May 28, 2024 •

edited

Loading

peppescg commented May 30, 2024 •

edited

Loading

peppescg commented May 30, 2024 •

edited

Loading