-
Notifications
You must be signed in to change notification settings - Fork 35
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 2 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,28 @@ 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, _ := 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.") | ||
| } | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
Comment on lines
+248
to
+253
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. MFA bypass risk on re-scoped org token
The interactive CLI path ( Consider inspecting the 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; re-scoping via --organization-slug is not supported for MFA-protected orgs", organizationSlug)
}
return selectedOrgRes.Token, nil
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. 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)
}This ensures we never silently store a pre-MFA token. 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 can you re-run the review, taking into account all updated 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. 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 ResolvedMFA Safety (8240907): The rescoping function now properly checks Error Handling (003476b): The flag conflict validation now properly handles errors with ✅ Code QualityAPI Integration: The new 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 📝 NotesScope: The rescoping logic is correctly placed only in the user login path ( 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 🚀 Ready to MergeThe 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. |
||
|
|
||
| err = util.StoreUserCredsInKeyRing(&userCredentialsToBeStored) | ||
| if err != nil { | ||
| log.Error().Msgf("Unable to store your credentials in system vault") | ||
|
|
@@ -405,7 +427,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 +897,56 @@ 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 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 | ||
| for _, org := range orgsResponse.Organizations { | ||
| if org.Slug == organizationSlug { | ||
| matchedOrgId = org.ID | ||
| break | ||
| } | ||
| for _, subOrg := range org.SubOrganizations { | ||
| if subOrg.Slug == organizationSlug { | ||
| matchedOrgId = subOrg.ID | ||
| break | ||
| } | ||
| } | ||
| if matchedOrgId != "" { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if matchedOrgId == "" { | ||
| return "", fmt.Errorf("organization with slug '%s' not found or not accessible", organizationSlug) | ||
| } | ||
|
|
||
| // 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.
Error silently discarded on conflict check
The error return from
util.GetCmdFlagOrEnvis discarded with_. If that call fails (e.g., because the flag is not registered or the env variable cannot be read),orgIdFlagwill be an empty string and the conflict validation at line 239 will silently pass — meaning a user who hasINFISICAL_ORGANIZATION_IDset and also provides--organization-slugwill NOT see the expected error. Compare to the identical call at line 221–224 which correctly handles the error.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.
Fixed in 003476b. The error is now properly handled instead of discarded:
This is now consistent with how the same call is handled at line 221-224.
@greptile-apps please re-review with the latest 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.
✅ Fix confirmed
The error handling is now correct and consistent with the pattern at lines 221-224. The validation will properly fail if
util.GetCmdFlagOrEnvencounters an error (e.g., flag not registered or environment variable issues), preventing the security concern where conflicts could be silently ignored.Good catch and clean fix!