Skip to content
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.

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

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

Merged
merged 6 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cli/app/auth/invite/invite_accept.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func inviteAcceptCommand(ctx context.Context, cmd *cobra.Command, args []string,
if err != nil {
return cli.MessageAndError("Error resolving invitation", err)
}
cmd.Printf("Invitation %s for %s to become %s of project %s was accepted!\n", code, res.Email, res.Role, res.Project)
cmd.Printf("Invitation %s for %s to become %s of project %s was accepted!\n", code, res.Email, res.Role, res.ProjectDisplay)
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/app/auth/invite/invite_decline.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func inviteDeclineCommand(ctx context.Context, cmd *cobra.Command, args []string
if err != nil {
return cli.MessageAndError("Error resolving invitation", err)
}
cmd.Printf("Invitation %s for %s to become %s of project %s was declined!\n", code, res.Email, res.Role, res.Project)
cmd.Printf("Invitation %s for %s to become %s of project %s was declined!\n", code, res.Email, res.Role, res.ProjectDisplay)
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/cli/app/project/role/role_grant.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ func GrantCommand(ctx context.Context, cmd *cobra.Command, _ []string, conn *grp
Role: r,
Email: email,
}
failMsg = "Error creating/updating an invite"
successMsg = "Invite created/updated successfully."
failMsg = "Error creating an invite"
successMsg = "Invite created successfully."
}

ret, err := client.AssignRole(ctx, &minderv1.AssignRoleRequest{
Expand Down
23 changes: 19 additions & 4 deletions cmd/cli/app/project/role/role_grant_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package role
import (
"context"
"fmt"
"strconv"
"strings"
"time"

"github.com/spf13/cobra"
"github.com/spf13/viper"
Expand Down Expand Up @@ -78,17 +80,30 @@ func GrantListCommand(ctx context.Context, cmd *cobra.Command, _ []string, conn
}
cmd.Println(out)
case app.Table:
t := initializeTableForGrantList()
t := initializeTableForGrantListRoleAssignments()
for _, r := range resp.RoleAssignments {
t.AddRow(r.Subject, r.Role)
t.AddRow(r.Subject, r.Role, *r.Project)
}
t.Render()
if len(resp.Invitations) > 0 {
t := initializeTableForGrantListInvitations()
for _, r := range resp.Invitations {
t.AddRow(r.Email, r.Role, r.SponsorDisplay, r.ExpiresAt.AsTime().Format(time.RFC3339), strconv.FormatBool(r.Expired), r.Code)
}
t.Render()
} else {
cmd.Println("No pending invitations found.")
}
}
return nil
}

func initializeTableForGrantList() table.Table {
return table.New(table.Simple, layouts.Default, []string{"Subject", "Role"})
func initializeTableForGrantListRoleAssignments() table.Table {
return table.New(table.Simple, layouts.Default, []string{"Subject", "Role", "Project"})
}

func initializeTableForGrantListInvitations() table.Table {
return table.New(table.Simple, layouts.Default, []string{"Invitee", "Role", "Sponsor", "Expires At", "Expired", "Code"})
rdimitrov marked this conversation as resolved.
Show resolved Hide resolved
}

func init() {
Expand Down
51 changes: 38 additions & 13 deletions cmd/cli/app/project/role/role_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package role

import (
"context"
"time"

"github.com/spf13/cobra"
"github.com/spf13/viper"
Expand All @@ -38,40 +39,64 @@ to a user (subject) on a particular project.`,
func UpdateCommand(ctx context.Context, cmd *cobra.Command, _ []string, conn *grpc.ClientConn) error {
client := minderv1.NewPermissionsServiceClient(conn)

sub := viper.GetString("sub")
r := viper.GetString("role")
project := viper.GetString("project")
sub := viper.GetString("sub")
email := viper.GetString("email")
rdimitrov marked this conversation as resolved.
Show resolved Hide resolved

// No longer print usage on returned error, since we've parsed our inputs
// See https://github.com/spf13/cobra/issues/340#issuecomment-374617413
cmd.SilenceUsage = true

ret, err := client.UpdateRole(ctx, &minderv1.UpdateRoleRequest{
req := &minderv1.UpdateRoleRequest{
Context: &minderv1.Context{
Project: &project,
},
Roles: []string{r},
Subject: sub,
})
}
failMsg := "Error updating role"
successMsg := "Updated role successfully."
if email != "" {
req.Email = email
failMsg = "Error updating an invite"
successMsg = "Invite updated successfully."
}

ret, err := client.UpdateRole(ctx, req)
if err != nil {
return cli.MessageAndError("Error updating role", err)
return cli.MessageAndError(failMsg, err)
}

cmd.Println("Update role successfully.")
cmd.Printf(
"Subject \"%s\" is now assigned to role \"%s\" on project \"%s\"\n",
ret.RoleAssignments[0].Subject,
ret.RoleAssignments[0].Role,
*ret.RoleAssignments[0].Project,
)
cmd.Println(successMsg)

if email != "" {
rdimitrov marked this conversation as resolved.
Show resolved Hide resolved
t := initializeTableForGrantListInvitations()
for _, r := range ret.Invitations {
expired := "No"
if r.Expired {
expired = "Yes"
}
t.AddRow(r.Email, r.Role, r.Sponsor, r.ExpiresAt.AsTime().Format(time.RFC3339), expired, r.Code)
}
t.Render()
return nil
}
// Otherwise, print the role assignments if it was about updating a role
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Partially addressed it in #3710

t := initializeTableForGrantListRoleAssignments()
for _, r := range ret.RoleAssignments {
t.AddRow(r.Subject, r.Role, *r.Project)
}
t.Render()
return nil
}

func init() {
RoleCmd.AddCommand(updateCmd)

updateCmd.Flags().StringP("sub", "s", "", "subject to update role access for")
updateCmd.Flags().StringP("role", "r", "", "the role to update it to")
updateCmd.MarkFlagsRequiredTogether("sub", "role")
updateCmd.Flags().StringP("sub", "s", "", "subject to update role access for")
updateCmd.Flags().StringP("email", "e", "", "email to send invitation to")
Comment on lines +98 to +99
Copy link
Member

Choose a reason for hiding this comment

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

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.

updateCmd.MarkFlagsOneRequired("sub", "email")
updateCmd.MarkFlagsMutuallyExclusive("sub", "email")
}
32 changes: 16 additions & 16 deletions database/mock/store.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 17 additions & 7 deletions database/query/invitations.sql
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
-- the invitee.

-- name: ListInvitationsForProject :many
SELECT user_invites.email, role, users.identity_subject, user_invites.created_at, user_invites.updated_at
SELECT user_invites.email, role, users.identity_subject, user_invites.created_at, user_invites.updated_at, user_invites.code
rdimitrov marked this conversation as resolved.
Show resolved Hide resolved
FROM user_invites
JOIN users ON user_invites.sponsor = users.id
WHERE project = $1;
Expand All @@ -14,23 +14,33 @@ WHERE project = $1;
-- to allow them to accept invitations even if email delivery was not working.
-- Note that this requires that the destination email address matches the email
-- address of the logged in user in the external identity service / auth token.
-- This clarification is related solely for user's ListInvitations calls and does
-- not affect to resolving invitations intended for other mail addresses.

-- name: GetInvitationsByEmail :many
SELECT * FROM user_invites WHERE email = $1;
SELECT user_invites.*, users.identity_subject
FROM user_invites
JOIN users ON user_invites.sponsor = users.id
WHERE email = $1;

-- GetInvitationByEmailAndProjectAndRole retrieves an invitation by email, project,
-- and role.
-- GetInvitationsByEmailAndProject retrieves all invitations by email and project.

-- name: GetInvitationByEmailAndProjectAndRole :one
SELECT * FROM user_invites WHERE email = $1 AND project = $2 AND role = $3;
-- name: GetInvitationsByEmailAndProject :many
SELECT user_invites.*, users.identity_subject
FROM user_invites
JOIN users ON user_invites.sponsor = users.id
WHERE email = $1 AND project = $2;

-- GetInvitationByCode retrieves an invitation by its code. This is intended to
-- be called by a user who has received an invitation email and is following the
-- link to accept the invitation or when querying for additional info about the
-- invitation.

-- name: GetInvitationByCode :one
SELECT * FROM user_invites WHERE code = $1;
SELECT user_invites.*, users.identity_subject
FROM user_invites
JOIN users ON user_invites.sponsor = users.id
WHERE code = $1;

-- CreateInvitation creates a new invitation. The code is a secret that is sent
-- to the invitee, and the email is the address to which the invitation will be
Expand Down
Loading