New connector 'keystone federation' for sso using shibboleth#44
New connector 'keystone federation' for sso using shibboleth#44
Conversation
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #225776
Actionable Suggestions - 2
-
connector/keystonefed/keystonefed_test.go - 1
- Incorrect struct tag syntax in test code · Line 33-54
-
connector/keystonefed/keystonefed.go - 1
- Incomplete HTTP client configuration for SAML · Line 363-368
Additional Suggestions - 2
-
connector/keystone/keystone_test.go - 2
-
Semantic duplication between testDomain and testDomainKeystone · Line 27-27The new `testDomainKeystone` variable is semantically duplicating the `testDomain` constant. Since `testDomainKeystone` is just a wrapper around `testDomain`, it creates redundant data that must be kept in sync.
Code suggestion
@@ -24,7 +24,6 @@ testDomain = "default" ) -var testDomainKeystone = domainKeystone{ID: testDomain} var ( keystoneURL = "" keystoneAdminURL = "" -
Unnecessary duplication of domain struct creation · Line 298-300The code creates a new `domainKeystone` struct with the domain ID, but there's already a global `testDomainKeystone` variable defined for this purpose. Using the existing variable would avoid duplication.
Code suggestion
@@ -297,7 +297,6 @@ userID := createUser(t, token, tt.input.username, tt.input.email, tt.input.password) defer deleteResource(t, token, userID, usersURL) - testD := domainKeystone{ID: tt.input.domain} c := conn{ - Host: keystoneURL, Domain: testD, + Host: keystoneURL, Domain: testDomainKeystone, AdminUsername: adminUser, AdminPassword: adminPass, }
-
Review Details
-
Files reviewed - 7 · Commit Range:
c3d27cc..a2b550a- connector/keystone/keystone_test.go
- connector/keystonefed/config.go
- connector/keystonefed/errors.go
- connector/keystonefed/keystonefed.go
- connector/keystonefed/keystonefed_test.go
- connector/keystonefed/types.go
- server/server.go
-
Files skipped - 1
- connector/keystonefed/doc.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
| _ = json.NewEncoder(w).Encode(authTokensResp{ | ||
| Token: struct { | ||
| User struct { | ||
| ID string "json:\"id\"" | ||
| Name string "json:\"name\"" | ||
| Email string "json:\"email\"" | ||
| Domain struct { | ||
| ID string "json:\"id\"" | ||
| } "json:\"domain\"" | ||
| } "json:\"user\"" | ||
| Project struct { | ||
| ID string "json:\"id\"" | ||
| } "json:\"project\"" | ||
| }{ | ||
| User: struct { | ||
| ID string "json:\"id\"" | ||
| Name string "json:\"name\"" | ||
| Email string "json:\"email\"" | ||
| Domain struct { | ||
| ID string "json:\"id\"" | ||
| } "json:\"domain\"" | ||
| }{ |
There was a problem hiding this comment.
The struct literals in the test use raw string tags (string "json:\"id\"") instead of proper Go struct tags (string \json:"id"``). This causes incorrect JSON serialization in tests.
Code suggestion
Check the AI-generated fix before applying
| _ = json.NewEncoder(w).Encode(authTokensResp{ | |
| Token: struct { | |
| User struct { | |
| ID string "json:\"id\"" | |
| Name string "json:\"name\"" | |
| Email string "json:\"email\"" | |
| Domain struct { | |
| ID string "json:\"id\"" | |
| } "json:\"domain\"" | |
| } "json:\"user\"" | |
| Project struct { | |
| ID string "json:\"id\"" | |
| } "json:\"project\"" | |
| }{ | |
| User: struct { | |
| ID string "json:\"id\"" | |
| Name string "json:\"name\"" | |
| Email string "json:\"email\"" | |
| Domain struct { | |
| ID string "json:\"id\"" | |
| } "json:\"domain\"" | |
| }{ | |
| _ = json.NewEncoder(w).Encode(authTokensResp{ | |
| Token: struct { | |
| User struct { | |
| ID string `json:"id"` | |
| Name string `json:"name"` | |
| Email string `json:"email"` | |
| Domain struct { | |
| ID string `json:"id"` | |
| } `json:"domain"` | |
| } `json:"user"` | |
| Project struct { | |
| ID string `json:"id"` | |
| } `json:"project"` | |
| }{ | |
| User: struct { | |
| ID string `json:"id"` | |
| Name string `json:"name"` | |
| Email string `json:"email"` | |
| Domain struct { | |
| ID string `json:"id"` | |
| } `json:"domain"` | |
| }{ |
Code Review Run #225776
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
connector/keystonefed/keystonefed.go
Outdated
| clientNoRedirect := &http.Client{ | ||
| Timeout: c.client.Timeout, | ||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
| return http.ErrUseLastResponse | ||
| }, | ||
| } |
There was a problem hiding this comment.
The client created for SAML POST doesn't inherit all properties from the main client, only the timeout. This could lead to missing configurations like TLS settings or proxies.
Code suggestion
Check the AI-generated fix before applying
| clientNoRedirect := &http.Client{ | |
| Timeout: c.client.Timeout, | |
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | |
| return http.ErrUseLastResponse | |
| }, | |
| } | |
| // Use a client that doesn't automatically follow redirects | |
| clientNoRedirect := *c.client | |
| clientNoRedirect.CheckRedirect = func(req *http.Request, via []*http.Request) error { | |
| return http.ErrUseLastResponse | |
| } |
Code Review Run #225776
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Code Review Agent Run #c12a9f
Actionable Suggestions - 2
-
connector/keystonefed/keystonefed.go - 2
- Removed conditional federation flow check changes behavior · Line 57-78
- Removed EnableFederation check changes behavior · Line 102-136
Review Details
-
Files reviewed - 1 · Commit Range:
a2b550a..a5c9630- connector/keystonefed/keystonefed.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
connector/keystonefed/keystonefed.go
Outdated
| // Use Shibboleth SSO login path for federation | ||
| ssoLoginPath := c.cfg.ShibbolethLoginPath | ||
| // Replace any {IdP} placeholder with the actual IdentityProviderID | ||
| ssoLoginPath = strings.Replace(ssoLoginPath, "{IdP}", c.cfg.IdentityProviderID, -1) | ||
|
|
||
| // Construct the Shibboleth login URL | ||
| u, err := url.Parse(fmt.Sprintf("%s%s", ksBase, ssoLoginPath)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("parsing SSO login URL: %w", err) | ||
| } | ||
|
|
||
| // Add the relay state containing our callback URL and state | ||
| // The relay state will be passed through the entire federation flow | ||
| //relayState := url.QueryEscape(fmt.Sprintf("callback=%s&state=%s", | ||
| // url.QueryEscape(callbackURL), | ||
| // url.QueryEscape(state))) | ||
| relayState := fmt.Sprintf("callback=%s&state=%s", callbackURL, state) | ||
| q := u.Query() | ||
| q.Set("RelayState", relayState) | ||
| c.logger.Infof("Setting RelayState=%s for federation login", relayState) | ||
| u.RawQuery = q.Encode() | ||
| return u.String(), nil |
There was a problem hiding this comment.
The code now always uses the federation flow, removing the conditional if c.cfg.EnableFederation check. This changes behavior by eliminating the standard Keystone WebSSO endpoint option.
Code suggestion
Check the AI-generated fix before applying
| // Use Shibboleth SSO login path for federation | |
| ssoLoginPath := c.cfg.ShibbolethLoginPath | |
| // Replace any {IdP} placeholder with the actual IdentityProviderID | |
| ssoLoginPath = strings.Replace(ssoLoginPath, "{IdP}", c.cfg.IdentityProviderID, -1) | |
| // Construct the Shibboleth login URL | |
| u, err := url.Parse(fmt.Sprintf("%s%s", ksBase, ssoLoginPath)) | |
| if err != nil { | |
| return "", fmt.Errorf("parsing SSO login URL: %w", err) | |
| } | |
| // Add the relay state containing our callback URL and state | |
| // The relay state will be passed through the entire federation flow | |
| //relayState := url.QueryEscape(fmt.Sprintf("callback=%s&state=%s", | |
| // url.QueryEscape(callbackURL), | |
| // url.QueryEscape(state))) | |
| relayState := fmt.Sprintf("callback=%s&state=%s", callbackURL, state) | |
| q := u.Query() | |
| q.Set("RelayState", relayState) | |
| c.logger.Infof("Setting RelayState=%s for federation login", relayState) | |
| u.RawQuery = q.Encode() | |
| return u.String(), nil | |
| if c.cfg.EnableFederation { | |
| // Use Shibboleth SSO login path for federation | |
| ssoLoginPath := c.cfg.ShibbolethLoginPath | |
| // Replace any {IdP} placeholder with the actual IdentityProviderID | |
| ssoLoginPath = strings.Replace(ssoLoginPath, "{IdP}", c.cfg.IdentityProviderID, -1) | |
| // Construct the Shibboleth login URL | |
| u, err := url.Parse(fmt.Sprintf("%s%s", ksBase, ssoLoginPath)) | |
| if err != nil { | |
| return "", fmt.Errorf("parsing SSO login URL: %w", err) | |
| } | |
| // Add the relay state containing our callback URL and state | |
| // The relay state will be passed through the entire federation flow | |
| //relayState := url.QueryEscape(fmt.Sprintf("callback=%s&state=%s", | |
| // url.QueryEscape(callbackURL), | |
| // url.QueryEscape(state))) | |
| relayState := fmt.Sprintf("callback=%s&state=%s", url.QueryEscape(callbackURL), url.QueryEscape(state)) | |
| q := u.Query() | |
| q.Set("RelayState", relayState) | |
| c.logger.Infof("Setting RelayState=%s for federation login", relayState) | |
| u.RawQuery = q.Encode() | |
| return u.String(), nil | |
| } else { | |
| // Standard Keystone WebSSO endpoint: | |
| // /v3/auth/OS-FEDERATION/websso/{protocol}?origin=<dex-callback> | |
| u, err := url.Parse(fmt.Sprintf("%s/v3/auth/OS-FEDERATION/websso/%s", ksBase, c.cfg.ProtocolID)) | |
| if err != nil { | |
| return "", err | |
| } | |
| q := u.Query() | |
| // Include Dex's state in origin so we can recover it. | |
| q.Set("origin", callbackURL+"?state="+url.QueryEscape(state)) | |
| u.RawQuery = q.Encode() | |
| return u.String(), nil | |
| } |
Code Review Run #c12a9f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
connector/keystonefed/keystonefed.go
Outdated
| c.logger.Infof("Processing federation flow with EnableFederation=true") | ||
| // Check if this is a direct SAML response from an IdP | ||
| if r.Method == "POST" && r.FormValue("SAMLResponse") != "" { | ||
| // Extract the SAML response | ||
| samlResponse := r.FormValue("SAMLResponse") | ||
| c.logger.Infof("Received direct SAML response (truncated): %s...", samlResponse[:min(len(samlResponse), 50)]) | ||
| c.logger.Infof("RelayState from SAML response: %s", r.FormValue("RelayState")) | ||
|
|
||
| // Use the SAML response to get a token from Keystone | ||
| ksToken, err = c.getKeystoneTokenFromSAML(samlResponse, r.FormValue("RelayState")) | ||
| if err != nil { | ||
| c.logger.Errorf("Error getting token from SAML: %v", err) | ||
| return connector.Identity{}, fmt.Errorf("getting token from SAML: %w", err) | ||
| } | ||
| c.logger.Infof("Successfully obtained token from SAML response") | ||
| } else { | ||
| // Check for a federation cookie indicating we've completed authentication | ||
| cookie, err := r.Cookie("_shibsession_") // This name might vary | ||
| if err == nil && cookie != nil { | ||
| // Get a token using the federation auth endpoint | ||
| ksToken, err = c.getKeystoneTokenFromFederation(r) | ||
| if err != nil { | ||
| return connector.Identity{}, fmt.Errorf("getting keystone token from federation: %w", err) | ||
| } | ||
| } else { | ||
| c.logger.Info("No SAML response found, checking for federation cookies") | ||
| // Extract federation cookies and use them to get a token | ||
| ksToken, err = c.getKeystoneTokenFromFederation(r) | ||
| if err != nil { | ||
| c.logger.Errorf("Error getting token from federation cookies: %v", err) | ||
| return connector.Identity{}, fmt.Errorf("getting token from federation cookies: %w", err) | ||
| } | ||
| c.logger.Infof("Successfully obtained token from federation cookies") | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR removes the conditional check for c.cfg.EnableFederation but keeps the federation flow logic, making the code always execute the federation flow regardless of configuration.
Code suggestion
Check the AI-generated fix before applying
@@ -101,6 +101,7 @@
// Handle federation flow if enabled
+ if c.cfg.EnableFederation {
c.logger.Infof("Processing federation flow with EnableFederation=true")
// Check if this is a direct SAML response from an IdP
if r.Method == "POST" && r.FormValue("SAMLResponse") != "" {
@@ -134,6 +135,14 @@
c.logger.Infof("Successfully obtained token from federation cookies")
}
}
+ } else {
+ // Standard token-in-query flow
+ if !c.cfg.TokenInQuery {
+ c.logger.Error("TokenInQuery=false path not implemented")
+ return connector.Identity{}, fmt.Errorf("tokenInQuery=false path not implemented")
+ }
+ ksToken = r.URL.Query().Get("ks_token")
+ c.logger.Infof("ks_token from query: %s", truncateToken(ksToken))
+ }
Code Review Run #c12a9f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #7a9259Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #67dadfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #ef9b3aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #97d4cfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
srm6867
changed the title
There was a problem hiding this comment.
Code Review Agent Run #335aff
Actionable Suggestions - 4
-
connector/keystone/utils.go - 2
- Incorrect HTTP status code handling logic · Line 54-60
- Missing error creation for non-200 status · Line 279-285
-
connector/keystone/federation.go - 1
- Improper error handling for invalid tokens · Line 280-290
-
connector/keystone/keystone.go - 1
- Incorrect API endpoint URL path construction · Line 189-190
Additional Suggestions - 5
-
connector/keystone/utils.go - 5
-
Missing error logging before returning error · Line 351-354The `getAllGroupsForUser` function fetches all Keystone groups but doesn't handle the error properly. If `getAllKeystoneGroups` fails, it returns immediately without logging the error, making troubleshooting difficult.
Code suggestion
@@ -351,6 +351,7 @@ allGroups, err := getAllKeystoneGroups(ctx, client, baseURL, token) if err != nil { + logger.Errorf("failed to fetch all keystone groups: %s", err) return nil, err } -
Inconsistent error logging pattern in function · Line 377-380The `getAllGroupsForUser` function doesn't log errors when `getUserLocalGroups` fails, unlike other error cases in the same function. This inconsistent error handling makes troubleshooting difficult.
Code suggestion
@@ -377,6 +377,7 @@ localGroups, err := getUserLocalGroups(ctx, client, baseURL, tokenInfo.User.ID, token) if err != nil { + logger.Errorf("failed to fetch user local groups for userID %s: %s", tokenInfo.User.ID, err) return nil, err } -
Missing error logging for getRoles failure · Line 425-428The `getAllGroupsForUser` function doesn't log errors when `getRoles` fails, unlike other error cases in the same function. This inconsistent error handling makes troubleshooting difficult.
Code suggestion
@@ -425,6 +425,7 @@ roles, err := getRoles(ctx, client, baseURL, token, logger) if err != nil { + logger.Errorf("failed to fetch roles: %s", err) return userGroups, err } -
Missing error logging for getProjects failure · Line 434-437The `getAllGroupsForUser` function doesn't log errors when `getProjects` fails, unlike other error cases in the same function. This inconsistent error handling makes troubleshooting difficult.
Code suggestion
@@ -434,6 +434,7 @@ projects, err := getProjects(ctx, client, baseURL, token, logger) if err != nil { + logger.Errorf("failed to fetch projects: %s", err) return userGroups, err } -
Missing error logging for getHostname failure · Line 448-453The `getAllGroupsForUser` function doesn't log errors when `getHostname` fails, unlike other error cases in the same function. This inconsistent error handling makes troubleshooting difficult.
Code suggestion
@@ -449,6 +449,7 @@ if customerName == "" { customerName, err = getHostname(baseURL) if err != nil { + logger.Errorf("failed to get hostname from baseURL %s: %s", baseURL, err) return userGroups, err } }
-
Review Details
-
Files reviewed - 10 · Commit Range:
77b4b95..4274d8e- connector/keystone/federation.go
- connector/keystone/keystone.go
- connector/keystone/types.go
- connector/keystone/utils.go
- connector/keystonefed/config.go
- connector/keystonefed/errors.go
- connector/keystonefed/keystonefed.go
- connector/keystonefed/keystonefed_test.go
- connector/keystonefed/types.go
- server/server.go
-
Files skipped - 1
- connector/keystonefed/doc.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
| if resp.StatusCode/100 != 2 { | ||
| return "", fmt.Errorf("keystone login: error %v", resp.StatusCode) | ||
| } | ||
| if resp.StatusCode != 201 { | ||
| return "", nil | ||
| } | ||
| return resp.Header.Get("X-Subject-Token"), nil |
There was a problem hiding this comment.
The function returns nil on non-201 status codes but should return an error. This causes silent failures when authentication succeeds but returns an unexpected status code.
Code suggestion
Check the AI-generated fix before applying
| if resp.StatusCode/100 != 2 { | |
| return "", fmt.Errorf("keystone login: error %v", resp.StatusCode) | |
| } | |
| if resp.StatusCode != 201 { | |
| return "", nil | |
| } | |
| return resp.Header.Get("X-Subject-Token"), nil | |
| if resp.StatusCode/100 != 2 { | |
| return "", fmt.Errorf("keystone login: error %v", resp.StatusCode) | |
| } | |
| if resp.StatusCode != 201 && resp.Header.Get("X-Subject-Token") == "" { | |
| return "", fmt.Errorf("keystone login: expected status code 201, got %v", resp.StatusCode) | |
| } | |
| return resp.Header.Get("X-Subject-Token"), nil |
Code Review Run #335aff
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != 200 { | ||
| return nil, err | ||
| } | ||
|
|
||
| data, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
The getUser function doesn't properly handle non-200 status codes. It returns nil, err where err is still nil, causing silent failures when user retrieval fails.
Code suggestion
Check the AI-generated fix before applying
| defer resp.Body.Close() | |
| if resp.StatusCode != 200 { | |
| return nil, err | |
| } | |
| data, err := io.ReadAll(resp.Body) | |
| defer resp.Body.Close() | |
| if resp.StatusCode != 200 { | |
| return nil, fmt.Errorf("keystone: get user details: error status code %d", resp.StatusCode) | |
| } | |
| data, err := io.ReadAll(resp.Body) |
Code Review Run #335aff
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| // If we have a stored token, try to use it to get token info | ||
| if len(data.Token) > 0 { | ||
| c.logger.Infof("Using stored token to get token info: %s", truncateToken(data.Token)) | ||
| tokenInfoFromStored, err := getTokenInfo(ctx, c.client, ksBase, data.Token, c.logger) | ||
| if err == nil { | ||
| // Only use the stored token info if we could retrieve it successfully | ||
| tokenInfo = tokenInfoFromStored | ||
| } else { | ||
| c.logger.Warnf("Could not get token info from stored token: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The stored token info retrieval doesn't handle HTTP status codes properly. If the token is invalid or expired, the code will continue with basic user info instead of returning an error for authentication failure.
Code suggestion
Check the AI-generated fix before applying
| // If we have a stored token, try to use it to get token info | |
| if len(data.Token) > 0 { | |
| c.logger.Infof("Using stored token to get token info: %s", truncateToken(data.Token)) | |
| tokenInfoFromStored, err := getTokenInfo(ctx, c.client, ksBase, data.Token, c.logger) | |
| if err == nil { | |
| // Only use the stored token info if we could retrieve it successfully | |
| tokenInfo = tokenInfoFromStored | |
| } else { | |
| c.logger.Warnf("Could not get token info from stored token: %v", err) | |
| } | |
| } | |
| // If we have a stored token, try to use it to get token info | |
| if len(data.Token) > 0 { | |
| c.logger.Infof("Using stored token to get token info: %s", truncateToken(data.Token)) | |
| tokenInfoFromStored, err := getTokenInfo(ctx, c.client, ksBase, data.Token, c.logger) | |
| if err == nil { | |
| // Only use the stored token info if we could retrieve it successfully | |
| tokenInfo = tokenInfoFromStored | |
| } else { | |
| // If we can't validate the stored token, this could indicate an authentication issue | |
| // such as an expired or revoked token, which should fail the refresh | |
| c.logger.Errorf("Failed to validate stored token during refresh: %v", err) | |
| return identity, fmt.Errorf("keystone federation: stored token validation failed: %v", err) | |
| } | |
| } |
Code Review Run #335aff
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #b8dc16Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
indradhanush
left a comment
indradhanush
left a comment
There was a problem hiding this comment.
Requesting changes because there are some comments I'm not sure are critical or not. Happy to discuss.
In the meantime, can you please remove redundant in-line comments that reiterate what is more or less self evident? Docstrings are useful for functions and attributes, but in line comments explaining the code start feeling like noise. Some comments are still useful however, and I leave it to your judgement.
Also, please don't create a new error by embedding the error inside it as a string. Instead wrap it. I pointed this out in a couple of places, but didn't specifically point it out in all such instances. Please take a look.
| return &FederationConnector{ | ||
| cfg: cfg, | ||
| client: &http.Client{ | ||
| Timeout: time.Duration(30) * time.Second, |
There was a problem hiding this comment.
| Timeout: time.Duration(30) * time.Second, | |
| Timeout: 30 * time.Second, |
Unnecessary conversion to time.Duration.
| q := u.Query() | ||
| q.Set("target", target) | ||
| u.RawQuery = q.Encode() | ||
| c.logger.Debugf("Shibboleth login URL with dex callback=%s", u.String()) |
There was a problem hiding this comment.
Can this leak secrets in the logs?
| } | ||
|
|
||
| func (c *FederationConnector) HandleCallback(scopes connector.Scopes, r *http.Request) (connector.Identity, error) { | ||
| c.logger.Debugf("Dex Callback received: URL=%s, Method=%s", r.URL.String(), r.Method) |
There was a problem hiding this comment.
Same as above. Does the URL contain any secrets?
| @@ -0,0 +1,491 @@ | |||
| package keystone | |||
There was a problem hiding this comment.
I'd discourage you from creating a utils.go file and instead leave this in keystone.go itself. This particular change not only creates a larger diff for the PR, but also now opens up a kitchen sink for the future. utils.go is a general name and one could argue that everything is a utility func. It's fine if the function is used in another file within the package. By looking at keystone.go, I'm able to tell that the functions belong to keystone related operations. With utils.go, its hard to derive a meaning.
Same thought for a utils package (for the future). 😊
| // normalizeKeystoneURL removes trailing '/keystone' or trailing '/' from the baseURL | ||
| // This ensures consistent URL handling regardless of how the URL was provided | ||
| func normalizeKeystoneURL(baseURL string) string { | ||
| // Remove trailing slash if present | ||
| baseURL = strings.TrimSuffix(baseURL, "/") | ||
|
|
||
| // Remove trailing '/keystone' if present | ||
| baseURL = strings.TrimSuffix(baseURL, "/keystone") | ||
|
|
||
| return baseURL | ||
| } |
There was a problem hiding this comment.
Please add a unit test for this function. While simple on paper, this is now very critical to everything keystone by the looks of it.
| // Copy all cookies from the original request to maintain the federation session | ||
| for _, cookie := range r.Cookies() { | ||
| req.AddCookie(cookie) | ||
| } |
There was a problem hiding this comment.
Do we really need to copy all the cookies here? Or just the related ones? Is there a security risk by copying other cookies that's not needed?
| }, nil | ||
| } | ||
|
|
||
| func (c *FederationConnector) LoginURL(scopes connector.Scopes, callbackURL, state string) (string, error) { |
| // Store the callback URL and state in the connector for use during callback handling | ||
| c.callbackURL = callbackURL | ||
| c.state = state |
There was a problem hiding this comment.
Is this even used anywhere? I couldn't find any other references to this.
|
closing this PR. we have New PR for this - #53 |
3 participants
Overview
Added New Connector
keystone federationWhat this PR does / why we need it
This PR adds new connector for keystone federation which is required to login with SSO configured in keystone (via shibboleth)
WIKI Reference -
Refer this sequence diagram to understand how Dex-keystone-shibboleth-IDP connects each other in the new connector.
https://platform9.atlassian.net/wiki/spaces/~61ccb5f6bce5e00069e66647/pages/5091819551/KAAPI+SSO+login+for+kubectl#Proposed-design---IMPLEMENTED
Does this PR introduce a user-facing change?
Yes, with this change user will see a new option in dex login page to select SSO login -
Change Summary
connector/keystone now contains 2 connectors -
keystone.gofederation.gofederation.gounder connector/keystone - This contains all the new connector related fns like connector details and callback handling.utils.gotypes.go-registered new connector in
server.goTESTING
a) Tested that
kubectl get nodescommand works -- It redirects to dex login page in browser
- we get 2 options Local vs SSO
- after choosing SSO, it redirects to Okta (configured IDP in shibboleth)
- after user authenticates with okta we get the final dex token.
- Dex token for the SSO user -