-
Notifications
You must be signed in to change notification settings - Fork 21
feat: support --organization-slug flag for user/browser SSO login #143
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
base: main
Are you sure you want to change the base?
Changes from all commits
780fb0b
8240907
003476b
a108a6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,6 +227,31 @@ var loginCmd = &cobra.Command{ | |
| cliDefaultLogin(&userCredentialsToBeStored, email, password, organizationId) | ||
| } | ||
|
|
||
| // If --organization-slug is provided, re-scope the token to the specified organization/sub-organization | ||
| organizationSlug, err := cmd.Flags().GetString("organization-slug") | ||
| if err != nil { | ||
| util.HandleError(err) | ||
| } | ||
|
|
||
| // Validate that --organization-id and --organization-slug are not both set | ||
| if organizationSlug != "" && isDirectUserLoginFlagsAndEnvsSet { | ||
| orgIdFlag, orgIdErr := util.GetCmdFlagOrEnv(cmd, "organization-id", []string{"INFISICAL_ORGANIZATION_ID"}) | ||
| if orgIdErr != nil { | ||
| util.HandleError(orgIdErr) | ||
| } | ||
| if orgIdFlag != "" { | ||
| util.PrintErrorMessageAndExit("Cannot use both --organization-id and --organization-slug at the same time. Please use only one to specify the target organization.") | ||
| } | ||
| } | ||
|
|
||
| if organizationSlug != "" { | ||
| newToken, rescopeErr := rescopeTokenToOrgBySlug(userCredentialsToBeStored.JTWToken, organizationSlug) | ||
| if rescopeErr != nil { | ||
| util.HandleError(rescopeErr, "Unable to scope login to the specified organization") | ||
| } | ||
| userCredentialsToBeStored.JTWToken = newToken | ||
| } | ||
|
|
||
| err = util.StoreUserCredsInKeyRing(&userCredentialsToBeStored) | ||
| if err != nil { | ||
| log.Error().Msgf("Unable to store your credentials in system vault") | ||
|
|
@@ -405,7 +430,7 @@ func init() { | |
| loginCmd.Flags().String("method", "user", "login method [user, universal-auth, kubernetes, azure, gcp-id-token, gcp-iam, aws-iam, oidc-auth]") | ||
| loginCmd.Flags().String("client-id", "", "client id for universal auth") | ||
| loginCmd.Flags().String("client-secret", "", "client secret for universal auth") | ||
| loginCmd.Flags().String("organization-slug", "", "When set for machine identity login, this will scope the login session to the specified sub-organization the machine identity has access to. If left empty, the session defaults to the organization where the machine identity was created in.") | ||
| loginCmd.Flags().String("organization-slug", "", "When set, this will scope the login session to the specified sub-organization. Works for both user login (including browser/SSO) and machine identity login. If left empty, the session defaults to the organization selected during login.") | ||
| loginCmd.Flags().String("machine-identity-id", "", "machine identity id for these login methods [kubernetes, azure, gcp-id-token, gcp-iam, aws-iam]") | ||
| loginCmd.Flags().String("service-account-token-path", "", "service account token path for kubernetes auth") | ||
| loginCmd.Flags().String("service-account-key-file-path", "", "service account key file path for GCP IAM auth") | ||
|
|
@@ -875,6 +900,74 @@ func decodePastedBase64Token(token string) (*models.UserCredentials, error) { | |
| return &loginResponse, nil | ||
| } | ||
|
|
||
| // rescopeTokenToOrgBySlug resolves an organization slug to its ID using the accessible-with-sub-orgs | ||
| // endpoint, asks the user to confirm the matched organization, and then calls selectOrganization to | ||
| // get a new token scoped to that org. | ||
| func rescopeTokenToOrgBySlug(currentToken string, organizationSlug string) (string, error) { | ||
| httpClient, err := util.GetRestyClientWithCustomHeaders() | ||
| if err != nil { | ||
| return "", fmt.Errorf("unable to get resty client with custom headers: %w", err) | ||
| } | ||
| httpClient.SetAuthToken(currentToken) | ||
|
|
||
| // Fetch all accessible organizations including sub-orgs | ||
| orgsResponse, err := api.CallGetAccessibleOrganizationsWithSubOrgs(httpClient) | ||
| if err != nil { | ||
| return "", fmt.Errorf("unable to fetch accessible organizations: %w", err) | ||
| } | ||
|
|
||
| // Search for the matching organization by slug (both root orgs and sub-orgs) | ||
| var matchedOrgId string | ||
| var matchedOrgName string | ||
| for _, org := range orgsResponse.Organizations { | ||
| if org.Slug == organizationSlug { | ||
| matchedOrgId = org.ID | ||
| matchedOrgName = org.Name | ||
| break | ||
| } | ||
| for _, subOrg := range org.SubOrganizations { | ||
| if subOrg.Slug == organizationSlug { | ||
| matchedOrgId = subOrg.ID | ||
| matchedOrgName = subOrg.Name | ||
| break | ||
| } | ||
| } | ||
| if matchedOrgId != "" { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if matchedOrgId == "" { | ||
| return "", fmt.Errorf("organization with slug '%s' not found or not accessible", organizationSlug) | ||
| } | ||
|
|
||
| // Prompt user to confirm the organization before re-scoping | ||
| confirmLabel := fmt.Sprintf("You are about to scope your login to organization \"%s\" (slug: %s). Do you want to continue?", matchedOrgName, organizationSlug) | ||
| confirmPrompt := promptui.Select{ | ||
| Label: confirmLabel, | ||
| Items: []string{"Yes", "No"}, | ||
| } | ||
| _, confirmResult, promptErr := confirmPrompt.Run() | ||
| if promptErr != nil { | ||
| return "", fmt.Errorf("confirmation prompt failed: %w", promptErr) | ||
| } | ||
| if confirmResult != "Yes" { | ||
| return "", fmt.Errorf("organization scope selection cancelled by user") | ||
| } | ||
|
|
||
| // Call selectOrganization to get a new token scoped to the matched org | ||
| selectedOrgRes, err := api.CallSelectOrganization(httpClient, api.SelectOrganizationRequest{OrganizationId: matchedOrgId}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("unable to select organization: %w", err) | ||
| } | ||
|
|
||
| if selectedOrgRes.MfaEnabled { | ||
| return "", fmt.Errorf("organization '%s' requires MFA verification; please log in without --organization-slug and complete the MFA challenge during interactive org selection", organizationSlug) | ||
| } | ||
|
|
||
| return selectedOrgRes.Token, nil | ||
| } | ||
|
|
||
| // Manages the browser login flow. | ||
| // Returns a UserCredentials object on success and an error on failure | ||
| func browserCliLogin() (models.UserCredentials, error) { | ||
|
Comment on lines
900
to
973
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feature discoverability — no documentation found No documentation was found in the The existing flag help text in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8240907. Added early validation that errors out when both if organizationSlug != "" && isDirectUserLoginFlagsAndEnvsSet {
orgIdFlag, _ := util.GetCmdFlagOrEnv(cmd, "organization-id", []string{"INFISICAL_ORGANIZATION_ID"})
if orgIdFlag != "" {
util.PrintErrorMessageAndExit("Cannot use both --organization-id and --organization-slug at the same time. Please use only one to specify the target organization.")
}
}This prevents the silent override behavior. Note this conflict can only occur in direct user login mode (email/password via flags), since browser/interactive login doesn't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @greptile-apps please re-review with the latest changes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latest changes look good! Both rounds of feedback have been properly addressed: ✅ Round 1 feedback (commit 8240907):
✅ Round 2 feedback (commit 003476b):
Code quality observations: The
The integration point (lines 230-253) is logical:
No issues found. The implementation is solid and ready for the manual testing checklist outlined in the PR description. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MFA bypass risk on re-scoped org token
rescopeTokenToOrgBySlugcallsCallSelectOrganizationand returnsselectedOrgRes.Token, but ignores theMfaEnabled/MfaMethodfields fromSelectOrganizationResponse. If the target organization (or sub-organization) enforces MFA, the returned token may be an incomplete/pre-MFA token that grants less access than expected — or one that silently bypasses MFA challenge handling that is performed elsewhere in the interactive login flow (e.g., incliDefaultLogin).The interactive CLI path (
cliDefaultLogin) already handles MFA via a polling loop. IfrescopeTokenToOrgBySlugreturns a token that still requires MFA verification, subsequent API calls will silently fail with 401/403 rather than prompting the user for a code.Consider inspecting the
SelectOrganizationResponsefor MFA fields and triggering the appropriate MFA challenge before storing the token:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 8240907.
rescopeTokenToOrgBySlugnow checksselectedOrgRes.MfaEnabledand returns a clear error if the target org requires MFA:This ensures we never silently store a pre-MFA token.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greptile-apps can you re-run the review, taking into account all updated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work addressing the previous feedback! The MFA safety check and error handling improvements are now in place. I've reviewed the complete PR and the code looks solid. Here's my assessment:
✅ Previous Issues Resolved
MFA Safety (8240907): The rescoping function now properly checks
selectedOrgRes.MfaEnabledand returns a clear error when the target org requires MFA. This prevents storing pre-MFA tokens.Error Handling (003476b): The flag conflict validation now properly handles errors with
util.HandleError(orgIdErr)instead of discarding them.✅ Code Quality
API Integration: The new
CallGetAccessibleOrganizationsWithSubOrgsfunction follows existing patterns and has proper error handling.Model Definitions: Clean, well-structured types for sub-organizations.
Search Logic: The nested loop to find matching slugs (root orgs → sub-orgs) correctly breaks out of both loops when a match is found.
Flag Conflict Validation: Properly prevents users from specifying both
--organization-idand--organization-slugduring direct login, with clear error messaging.📝 Notes
Scope: The rescoping logic is correctly placed only in the user login path (
if loginMethod == "user"). Machine identity login already has separate org-slug handling.Timing: Rescoping happens after login but before storing credentials, which is the correct order - you need the initial token to query accessible orgs, and you want to store the rescoped token.
Backend Dependency: As noted in your PR description, this relies on the backend having the
/v1/organization/accessible-with-sub-orgsendpoint. If it doesn't exist on older instances, users will get a clear error from the API call.🚀 Ready to Merge
The implementation looks solid and all previous concerns have been addressed. The testing checklist in your PR description covers the important edge cases (backend endpoint existence, token permissions, MFA enforcement, flag conflicts).
One optional future consideration: if users report confusion when the backend endpoint doesn't exist (e.g., on older self-hosted instances), you could add a more helpful error message that suggests upgrading the Infisical instance. But that's not blocking for this PR.