-
Notifications
You must be signed in to change notification settings - Fork 4.8k
CNTRLPLANE-945: Add tests for ExternalOIDC and ExternalOIDCWithUIDAndExtraClaimMappings features #29917
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
CNTRLPLANE-945: Add tests for ExternalOIDC and ExternalOIDCWithUIDAndExtraClaimMappings features #29917
Changes from 2 commits
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 | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,328 @@ | ||||||||||||||||||||||||||||||
| package authentication | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||
| "bytes" | ||||||||||||||||||||||||||||||
| "crypto/tls" | ||||||||||||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||
| "io" | ||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||
| "net/url" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| "k8s.io/apimachinery/pkg/runtime" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type keycloakClient struct { | ||||||||||||||||||||||||||||||
| realm string | ||||||||||||||||||||||||||||||
| client *http.Client | ||||||||||||||||||||||||||||||
| adminURL *url.URL | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| accessToken string | ||||||||||||||||||||||||||||||
| idToken string | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| func keycloakClientFor(keycloakURL string) (*keycloakClient, error) { | ||||||||||||||||||||||||||||||
| baseURL, err := url.Parse(keycloakURL) | ||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("parsing url: %w", err) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| transport := &http.Transport{ | ||||||||||||||||||||||||||||||
| TLSClientConfig: &tls.Config{ | ||||||||||||||||||||||||||||||
| InsecureSkipVerify: true, | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return &keycloakClient{ | ||||||||||||||||||||||||||||||
| realm: "master", | ||||||||||||||||||||||||||||||
| client: &http.Client{ | ||||||||||||||||||||||||||||||
| Transport: transport, | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| adminURL: baseURL.JoinPath("admin", "realms", "master"), | ||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| func (kc *keycloakClient) CreateGroup(name string) error { | ||||||||||||||||||||||||||||||
| groupURL := kc.adminURL.JoinPath("groups") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| group := map[string]interface{}{ | ||||||||||||||||||||||||||||||
| "name": name, | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| group := map[string]interface{}{ | |
| "name": name, | |
| } | |
| group := struct{ | |
| Name string `json:"name"` | |
| }{ | |
| Name: name, | |
| } |
| group := map[string]interface{}{ | |
| "name": name, | |
| } | |
| group := map[string]any{ | |
| "name": name, | |
| } |
Applies to similar code in this file.
Outdated
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.
Nit:
user := struct {
Username string `json:"username"`
Email string `json:"email"`
Enabled bool `json:"enabled"`
EmailVerified bool `json:"emailVerified"`
Groups []string `json:"groups"`
Credentials []struct {
Temporary bool `json:"temporary"`
Type string `json:"type"`
Value string `json:"value"`
} `json:"credentials"`
}{
Username: username,
Email: fmt.Sprintf("%[email protected]", username),
Enabled: true,
EmailVerified: true,
Groups: groups,
Credentials: []struct {
Temporary bool `json:"temporary"`
Type string `json:"type"`
Value string `json:"value"`
}{
{
Temporary: false,
Type: "password",
Value: password,
},
},
}
Outdated
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.
Nit:
| data := url.Values{ | |
| "username": []string{username}, | |
| "password": []string{password}, | |
| "grant_type": []string{"password"}, | |
| "client_id": []string{clientID}, | |
| "scope": []string{"openid"}, | |
| } | |
| data := url.Values{} | |
| data.Set("username", username) | |
| data.Set("password", password) | |
| data.Set("grant_type", "password") | |
| data.Set("client_id", clientID) | |
| data.Set("scope", "openid") |
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.
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.
We only post form data for this one type of request. If we find ourselves repeating this logic quite frequently I could see value in a common method.
Outdated
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.
nit: I am a strong believer in anonymous structs for type-safety 😄
var tokenResponse struct {
AccessToken string `json:"access_token"`
IDToken string `json:"id_token"`
TokenType string `json:"token_type,omitempty"`
ExpiresIn int `json:"expires_in,omitempty"`
Scope string `json:"scope,omitempty"`
}
Outdated
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.
Streams without buffering entire reponse:
| respBodyData, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("reading response data: %w", err) | |
| } | |
| err = json.Unmarshal(respBodyData, &respBody) | |
| if err != nil { | |
| return fmt.Errorf("unmarshalling response body %s: %w", respBodyData, err) | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil { | |
| return fmt.Errorf("unmarshalling response data: %w", err) | |
| } |
Applies to other occurrences as well. Only thing that I feel strongly about. But I would not block the PR as this is not production code.
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.
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.
I had intentionally left off the end bracket because it seems to be the pattern other test suites follow.
Presumably this is so you can run sub-suites of this as part of this suite (i.e if I did something like
[Suite:openshift/auth/external-oidc/some-sub-thing]the test with this "tag" would still run as part of theopenshift/auth/external-oidctest suite.If we think this is unnecessary and that we should deviate from existing convention, I can add the closing bracket.
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.
Right, that's a good point -- I just thought this was a typo, but this makes sense 👍