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

Allow authenticated users without Minder projects to accept credentials #3909

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

evankanderson
Copy link
Member

Summary

See discussion here: #3837 (comment)

When a user is invited to a project (and they haven't used Minder before),
minder auth invite accept <code> should set up what they need to use the
invited project, without requiring them to call CreateUser and set up a
"personal project".

Along the way, I ended up moving the login functions to internal/util/cli,
because it felt strange to have a bunch of shared logic in cmd/cli/app/auth.

Fixes #3795

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

Added a unit test and tested manually with the new minder auth invite accept.

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
Copy link

coveralls commented Jul 16, 2024

Coverage Status

coverage: 53.955% (-0.4%) from 54.311%
when pulling 0ef90f3 on evankanderson:accept-invitation
into e0b91af on stacklok:main.

@evankanderson
Copy link
Member Author

@kantord -- this changes the flow such that we don't need to call CreateUser before calling ResolveInvitation -- we just need the user to have logged in through Keycloak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of the remainder of this file is common table-rendering code, which I left in place because I didn't want to move too much at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't have tests before when it was in cmd/cli/app/auth, but it shows up in coverage now, because it's in internal. I think that's fair, but I'm not prepared to write tests for it yet.

@kantord kantord closed this Jul 17, 2024
@kantord kantord reopened this Jul 17, 2024
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

I don't think anything here would break the frontend, but there would need to be some frontend changes to make the behaviour consistent with the CLI.

return nil
// Create a user if necessary, see https://github.com/stacklok/minder/pull/3837/files#r1674108001
if errors.Is(err, sql.ErrNoRows) {
return store.CreateUser(ctx, sub)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an edge case where the invited user will also have installed the GitHub App from the Marketplace, in which case we would want to call claimGitHubInstalls as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch! Fixing!

@evankanderson evankanderson merged commit e017d7d into mindersec:main Jul 18, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docs for user management and invitations
5 participants