This repository has been archived by the owner on Mar 11, 2021. It is now read-only.
Use whitelist to restrict permission to user profile override #495
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,43 +4,53 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
| "reflect" | ||
| "time" | ||
|
|
||
| "github.com/bitly/go-simplejson" | ||
| jwt "github.com/dgrijalva/jwt-go" | ||
| "github.com/fabric8-services/fabric8-tenant/app" | ||
| "github.com/fabric8-services/fabric8-tenant/auth" | ||
| "github.com/fabric8-services/fabric8-tenant/jsonapi" | ||
| "github.com/fabric8-services/fabric8-tenant/keycloak" | ||
| "github.com/fabric8-services/fabric8-tenant/openshift" | ||
| "github.com/fabric8-services/fabric8-tenant/tenant" | ||
| "github.com/fabric8-services/fabric8-tenant/toggles" | ||
| "github.com/fabric8-services/fabric8-wit/errors" | ||
| "github.com/fabric8-services/fabric8-wit/goasupport" | ||
| "github.com/fabric8-services/fabric8-wit/log" | ||
| "github.com/fabric8-services/fabric8-wit/rest" | ||
| "github.com/goadesign/goa" | ||
| goaclient "github.com/goadesign/goa/client" | ||
| goajwt "github.com/goadesign/goa/middleware/security/jwt" | ||
| errs "github.com/pkg/errors" | ||
| uuid "github.com/satori/go.uuid" | ||
| ) | ||
|
|
||
| // TenantController implements the status resource. | ||
| type TenantController struct { | ||
| *goa.Controller | ||
| httpClient *http.Client | ||
| tenantService tenant.Service | ||
| keycloakConfig keycloak.Config | ||
| openshiftConfig openshift.Config | ||
| templateVars map[string]string | ||
| authURL string | ||
| toggleClient toggles.Client | ||
| } | ||
|
|
||
| // NewTenantController creates a status controller. | ||
| func NewTenantController(service *goa.Service, tenantService tenant.Service, httpClient *http.Client, toggleClient *toggles.Client, keycloakConfig keycloak.Config, openshiftConfig openshift.Config, templateVars map[string]string, authURL string) *TenantController { | ||
| return &TenantController{ | ||
| Controller: service.NewController("TenantController"), | ||
| httpClient: httpClient, | ||
| tenantService: tenantService, | ||
| toggleClient: *toggleClient, | ||
| keycloakConfig: keycloakConfig, | ||
| openshiftConfig: openshiftConfig, | ||
| templateVars: templateVars, | ||
| authURL: authURL, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -81,7 +91,7 @@ func (c *TenantController) Setup(ctx *app.SetupTenantContext) error { | |
| return jsonapi.JSONErrorResponse(ctx, errors.NewInternalError(ctx, err)) | ||
| } | ||
|
|
||
| oc, err := c.loadUserTenantConfiguration(ctx, c.openshiftConfig) | ||
| if err != nil { | ||
| log.Error(ctx, map[string]interface{}{ | ||
| "err": err, | ||
|
|
@@ -145,7 +155,7 @@ func (c *TenantController) Update(ctx *app.UpdateTenantContext) error { | |
| if openshift.KubernetesMode() { | ||
| rawOC = &userConfig | ||
| } | ||
| oc, err := c.loadUserTenantConfiguration(ctx, *rawOC) | ||
| if err != nil { | ||
| log.Error(ctx, map[string]interface{}{ | ||
| "err": err, | ||
|
|
@@ -175,22 +185,30 @@ func (c *TenantController) Update(ctx *app.UpdateTenantContext) error { | |
| return ctx.Accepted() | ||
| } | ||
|
|
||
| // loadUserTenantConfiguration loads the tenant configuration for `auth`, | ||
| // allowing for config overrides based on the content of his profile (in auth) if the user is allowed | ||
| func (c *TenantController) loadUserTenantConfiguration(ctx context.Context, config openshift.Config) (openshift.Config, error) { | ||
| token := goajwt.ContextJWT(ctx) | ||
| // use the toggle service to verify that the user is allowed to change is profile based on the settings passed in the URL | ||
| if len(c.authURL) > 0 && c.toggleClient.IsEnabled(token, toggles.UserUpdateTenantFeature, false) { | ||
| log.Info(ctx, map[string]interface{}{"auth_url": c.authURL, "http_client_transport": reflect.TypeOf(c.httpClient.Transport)}, "retrieving user's profile...") | ||
| authClient, err := newAuthClient(ctx, c.httpClient, c.authURL) | ||
| if err != nil { | ||
| log.Error(ctx, map[string]interface{}{"auth_url": c.authURL}, "unable to parse auth URL") | ||
| return config, err | ||
| } | ||
| resp, err := authClient.ShowUser(ctx, auth.ShowUserPath(), nil, nil) | ||
|
There was a problem hiding this comment. Close response body in defer and make sure the body has been read. Otherwise you will leak FDs. There was a problem hiding this comment. oh yes, indeed! thanks, @alexeykazakov! |
||
| if err != nil { | ||
|
There was a problem hiding this comment. You forgot to remove lines 202-205 I guess (duplicates 208-211) There was a problem hiding this comment. no, these 2 errors are different: There was a problem hiding this comment. Oh, you are right! Looks good to me now. |
||
| log.Error(ctx, map[string]interface{}{"auth_url": auth.ShowUserPath()}, "unable to get user info") | ||
| return config, errs.Wrapf(err, "failed to GET url %s due to error", auth.ShowUserPath()) | ||
| } | ||
| if resp.StatusCode < 200 || resp.StatusCode > 300 { | ||
| return config, fmt.Errorf("failed to GET url %s due to status code %d", resp.Request.URL, resp.StatusCode) | ||
|
There was a problem hiding this comment. You need to read the body here too because otherwise it may leak FD even if you close the body :) There was a problem hiding this comment. ok, let me move this line just after There was a problem hiding this comment. If response is not a JSON (if it's not 200) then simplejson.NewFromReader(resp.Body) will fail I guess. So, instead do the following: Or Or simply use There was a problem hiding this comment. ok, thanks for the explanations, @alexeykazakov. Am I wrong or does reads the body but does not return it ? I thought we should read it once only ? Or at least once ? (In Java you cannot read the streams multiple times) There was a problem hiding this comment. That's right. It reads the stream to make sure it's empty and then closes the body. If stream is already empty (body has been read) that's OK. It doesn't do any harm. |
||
| } | ||
| js, err := simplejson.NewFromReader(resp.Body) | ||
| if err != nil { | ||
| return config, err | ||
| } | ||
| tenantConfig := js.GetPath("data", "attributes", "contextInformation", "tenantConfig") | ||
| if tenantConfig.Interface() != nil { | ||
| cheVersion := getJsonStringOrBlank(tenantConfig, "cheVersion") | ||
|
|
@@ -424,3 +442,16 @@ func convertTenant(tenant *tenant.Tenant, namespaces []*tenant.Namespace) *app.T | |
| } | ||
| return &result | ||
| } | ||
|
|
||
| // NewAuthClient initializes a new client to the `auth` service | ||
| func newAuthClient(ctx context.Context, httpClient *http.Client, authURL string) (*auth.Client, error) { | ||
| u, err := url.Parse(authURL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c := auth.New(goaclient.HTTPClientDoer(httpClient)) | ||
| c.Host = u.Host | ||
| c.Scheme = u.Scheme | ||
| c.SetJWTSigner(goasupport.NewForwardSigner(ctx)) | ||
| return c, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| package controller | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| jwt "github.com/dgrijalva/jwt-go" | ||
| "github.com/dnaeon/go-vcr/recorder" | ||
| "github.com/fabric8-services/fabric8-tenant/keycloak" | ||
| "github.com/fabric8-services/fabric8-tenant/openshift" | ||
| testtoggles "github.com/fabric8-services/fabric8-tenant/test/toggles" | ||
| "github.com/fabric8-services/fabric8-tenant/toggles" | ||
| "github.com/goadesign/goa" | ||
| goajwt "github.com/goadesign/goa/middleware/security/jwt" | ||
| uuid "github.com/satori/go.uuid" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/stretchr/testify/suite" | ||
| ) | ||
|
|
||
| type TenantControllerTestSuite struct { | ||
| suite.Suite | ||
| } | ||
|
|
||
| func TestTenantController(t *testing.T) { | ||
| // resource.Require(t, resource.Database) | ||
| suite.Run(t, &TenantControllerTestSuite{}) | ||
| } | ||
|
|
||
| func (s *TenantControllerTestSuite) TestLoadTenantConfiguration() { | ||
|
|
||
| // given | ||
| svc := goa.New("Tenants-service") | ||
| inputOpenShiftConfig := openshift.Config{ | ||
| CheVersion: "che-version", | ||
| JenkinsVersion: "jenkins-version", | ||
| MavenRepoURL: "maven-url", | ||
| TeamVersion: "team-version", | ||
| } | ||
| tenantID := uuid.NewV4() | ||
| tenantService := mockTenantService{ID: tenantID} | ||
| ctx := createValidUserContext(tenantID.String()) | ||
|
|
||
| s.T().Run("override disabled", func(t *testing.T) { | ||
|
|
||
| // given | ||
| userUpdateTenantFeature := testtoggles.NewMockFeature(toggles.UserUpdateTenantFeature, false) | ||
| toggleClient := toggles.NewCustomClient(testtoggles.NewMockUnleashClient(*userUpdateTenantFeature), true) | ||
| // ensure that the "override config" feature is disabled for the user | ||
| require.False(t, toggleClient.IsEnabled(goajwt.ContextJWT(ctx), toggles.UserUpdateTenantFeature, false)) | ||
|
|
||
| t.Run("user has config in profile", func(t *testing.T) { | ||
| // given | ||
| r, err := recorder.New("../test/data/tenant/auth_get_user.withconfig") | ||
| require.Nil(t, err) | ||
| defer r.Stop() | ||
| mockHTTPClient := &http.Client{ | ||
| Transport: r.Transport, | ||
| } | ||
| ctrl := NewTenantController(svc, tenantService, mockHTTPClient, toggleClient, keycloak.Config{}, inputOpenShiftConfig, map[string]string{}, "http://auth") | ||
| // when | ||
| resultConfig, err := ctrl.loadUserTenantConfiguration(ctx, inputOpenShiftConfig) | ||
| // then | ||
| require.NoError(t, err) | ||
| assert.Equal(t, inputOpenShiftConfig, resultConfig) | ||
| }) | ||
|
|
||
| t.Run("user has no config in profile", func(t *testing.T) { | ||
| // given | ||
| r, err := recorder.New("../test/data/tenant/auth_get_user.withoutconfig") | ||
| require.Nil(t, err) | ||
| defer r.Stop() | ||
| mockHTTPClient := &http.Client{ | ||
| Transport: r.Transport, | ||
| } | ||
| ctrl := NewTenantController(svc, tenantService, mockHTTPClient, toggleClient, keycloak.Config{}, inputOpenShiftConfig, map[string]string{}, "http://auth") | ||
| // when | ||
| resultConfig, err := ctrl.loadUserTenantConfiguration(ctx, inputOpenShiftConfig) | ||
| // then | ||
| require.NoError(t, err) | ||
| assert.Equal(t, inputOpenShiftConfig, resultConfig) | ||
| }) | ||
| }) | ||
|
|
||
| s.T().Run("override enabled", func(t *testing.T) { | ||
| // given | ||
| userUpdateTenantFeature := testtoggles.NewMockFeature(toggles.UserUpdateTenantFeature, true) | ||
| toggleClient := toggles.NewCustomClient(testtoggles.NewMockUnleashClient(*userUpdateTenantFeature), true) | ||
| // ensure that the "override config" feature is enabled for the user | ||
| require.True(t, toggleClient.IsEnabled(goajwt.ContextJWT(ctx), toggles.UserUpdateTenantFeature, false)) | ||
|
|
||
| t.Run("user has config in profile", func(t *testing.T) { | ||
| // given | ||
| r, err := recorder.New("../test/data/tenant/auth_get_user.withconfig") | ||
| require.Nil(t, err) | ||
| defer r.Stop() | ||
| mockHTTPClient := &http.Client{ | ||
| Transport: r.Transport, | ||
| } | ||
| ctrl := NewTenantController(svc, tenantService, mockHTTPClient, toggleClient, keycloak.Config{}, inputOpenShiftConfig, map[string]string{}, "http://auth") | ||
| // when | ||
| resultConfig, err := ctrl.loadUserTenantConfiguration(ctx, inputOpenShiftConfig) | ||
| // then | ||
| require.NoError(t, err) | ||
| expectedOpenshiftConfig := openshift.Config{ | ||
| CheVersion: "another-che-version", | ||
| JenkinsVersion: "another-jenkins-version", | ||
| MavenRepoURL: "another-maven-url", | ||
| TeamVersion: "another-team-version", | ||
| } | ||
| assert.Equal(t, expectedOpenshiftConfig, resultConfig) | ||
| }) | ||
|
|
||
| t.Run("user has no config in profile", func(t *testing.T) { | ||
| // given | ||
| r, err := recorder.New("../test/data/tenant/auth_get_user.withoutconfig") | ||
| require.Nil(t, err) | ||
| defer r.Stop() | ||
| mockHTTPClient := &http.Client{ | ||
| Transport: r.Transport, | ||
| } | ||
| ctrl := NewTenantController(svc, tenantService, mockHTTPClient, toggleClient, keycloak.Config{}, inputOpenShiftConfig, map[string]string{}, "http://auth") | ||
| // when | ||
| resultConfig, err := ctrl.loadUserTenantConfiguration(ctx, inputOpenShiftConfig) | ||
| // then | ||
| require.NoError(t, err) | ||
| assert.Equal(t, inputOpenShiftConfig, resultConfig) | ||
| }) | ||
| }) | ||
|
|
||
| } | ||
|
|
||
| func createValidUserContext(userID string) context.Context { | ||
| claims := jwt.MapClaims{} | ||
| claims["sub"] = userID | ||
| claims["session_state"] = "foo_session_id" | ||
| token := jwt.NewWithClaims(jwt.SigningMethodRS512, claims) | ||
| return goajwt.WithJWT(context.Background(), token) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is not really a good use of Toggles. Toggles != Permanent User config.
#229
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.
Sorry, I'm not following you here: the goal of the change is to allow (or not) user config override, based on the toggle state for the user. The user's tenants will be initialised with the default versions for Che/Jenkins/etc, unless toggle is enabled and user has some specific settings in his/her profile.
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.
@xcoulon Yes, I get that. But this is not a temporary setting, right? It's not related to 'let's try something out' or 'while we deploy this new feature' type of setting. It's a permanent feature of the user that he has more rights or whatever. Since it's permanent it's the wrong use of the Toggle Service.
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.
@aslakknutsen ok, got it, so using the toggle service is not the right way to handle this permission. So, should we set a flag in the user's profile or use
authto check the permission to override the profile ?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.
@xcoulon I would add some type of 'group' to the "tenant" table. That way we can use the info when doing automatic updates as well. E.g. roll out to group N first, then wait 24h and unless stopped, roll out to group X.. etc etc
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.
KC is not a good candidate for many reasons :)
Groups/teams/permissions will be handled by Auth eventually but we are not there yet.
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.
@xcoulon Auto update of tenants.
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.
@alexeykazakov @aslakknutsen ok, so what solution should I use in the mean time (i.e., until auth supports groups/team permissions), then ?
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 could enable custom jenkins/che only for specific users (by UUID) for now.
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.
yes, that's the point of this issue and using the toggle service was one way to implement it (i.e, verify that the user's ID belongs to a whitelist). However, I understand @aslakknutsen's argument that toggle service should be used to enable/disable features that are not released yet, which is not the use-case for this solution. Hence my question: where do we keep track of the permission ?