Implement ListInvitations and ResolveInvitation APIs for UserService #3666
Conversation
| } | ||
|
|
||
| // Check if the invitation matches the user email | ||
| if token.Email() != ret.Email { |
There was a problem hiding this comment.
Implemented this per - https://github.com/stacklok/minder/blob/07f93e0c1dcb59f74b6f15beb24cf684161287bb/database/query/invitations.sql#L15
which ensures if the email of the logged-in user corresponds to the one of the invite.
@evankanderson - I know we discussed this to be more loose but then I saw that you made that comment so I implemented it that way. I'll ping you to see which is the desired behavior we want to keep.
For the reviewers, don't take this comment as blocking as we can remove this check easily.
There was a problem hiding this comment.
No, we want to remove this for Resolve.
The comment was for ListInvitations -- a user might have an invitation that doesn't match up with their "known" email address. We still want to honor that code, even though it doesn't show up in ListInvitations. Feel free to clarify the comment in the schema to help make that more clear.
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
There was a problem hiding this comment.
It doesn't look like we have any unit test coverage for these, and there's enough new logic here that I feel like it would benefit from testing.
I wonder if splitting the logic between the handler, and an InviteService struct/interface would simplify writing tests too.
| // Extracts the user email from the token | ||
| tokenString, err := gauth.AuthFromMD(ctx, "bearer") | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "no auth token: %v", err) | ||
| } | ||
| token, err := s.jwt.ParseAndValidate(tokenString) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Unauthenticated, "failed to parse bearer token: %v", err) | ||
| } |
There was a problem hiding this comment.
It would be nice to have "give me a parsed JWT" as a method, rather than needing this in the business logic.
There was a problem hiding this comment.
Can you add a GetUserEmailFromContext method to internal/auth/jwtauth.go? We already have GetUserSubjectFromContext and GetUserClaimFromContext, so this feels like it should belong there.
There was a problem hiding this comment.
(We've already parsed the JWT in TokenValidationInterceptor, so we should leverage that.)
| // Get the list of invitations for the user | ||
| invites, err := s.store.GetInvitationsByEmail(ctx, token.Email()) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { |
There was a problem hiding this comment.
sql.ErrNoRows is only returned for :one result types, not :many, so this code should never be hit.
| UserID: sponsorUser.IdentitySubject, | ||
| HumanName: sponsorUser.IdentitySubject, | ||
| } | ||
| if flags.Bool(ctx, s.featureFlags, flags.IDPResolver) { |
There was a problem hiding this comment.
Not sure whether we need to leave this behind this flag or not... I was thinking of removing (retiring) this flag, since we went a different approach.
There was a problem hiding this comment.
(Which would allow you to simplify the code by simply calling s.idClient.Resolve, rather than constructing a default Identity object.)
| Email: invite.Email, | ||
| Project: invite.Project.String(), | ||
| CreatedAt: timestamppb.New(invite.CreatedAt), | ||
| ExpiresAt: timestamppb.New(invite.UpdatedAt.Add(7 * 24 * time.Hour)), |
There was a problem hiding this comment.
Do you want to extract the ExpiresAt math, since it's used in two fields?
| CreatedAt: timestamppb.New(invite.CreatedAt), | ||
| ExpiresAt: timestamppb.New(invite.UpdatedAt.Add(7 * 24 * time.Hour)), | ||
| Expired: time.Now().After(invite.UpdatedAt.Add(7 * 24 * time.Hour)), | ||
| Sponsor: identity.UserID, |
There was a problem hiding this comment.
Use identity.String() here, I think. It doesn't do anything right now, but if we decided to add other identity sources like GitHub Actions, that would allow us to refer to them uniquely.
| ret, err := s.store.GetInvitationByCode(ctx, req.Code) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, util.UserVisibleError(codes.NotFound, "invitation not found") |
There was a problem hiding this comment.
This should probably say "not found or already used"
| } | ||
|
|
||
| // Check if the invitation matches the user email | ||
| if token.Email() != ret.Email { |
There was a problem hiding this comment.
No, we want to remove this for Resolve.
The comment was for ListInvitations -- a user might have an invitation that doesn't match up with their "known" email address. We still want to honor that code, even though it doesn't show up in ListInvitations. Feel free to clarify the comment in the schema to help make that more clear.
| // Delete the invitation since its resolved | ||
| ret, err = s.store.DeleteInvitation(ctx, req.Code) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to delete invitation: %s", err) |
There was a problem hiding this comment.
What happens in this case -- the user has been added to the OpenFGA database; if they try calling ResolveInvitation again, will they create a second tuple? Will OpenFGA error with "already exists"?
We may want to return a util.UserVisibleError(codes.Internal, "permission granted, but unable to revoke invitation") after logging the error internally here.
We may also want to check what the result of attempting to grant the same permission twice is in terms of OpenFGA errors, and account for that specially.
| return nil, util.UserVisibleError(codes.PermissionDenied, "invitation expired") | ||
| } | ||
|
|
||
| isAccepted := false |
There was a problem hiding this comment.
Do we need isAccepted, or can we just use req.Accept below?
Summary
The following PR implements ListInvitations and ResolveInvitation APIs for UserService.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: