Skip to content
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

Closed
wants to merge 17 commits into from
Closed

Use whitelist to restrict permission to user profile override #495

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1c93e3a
Use whitelist to restrict permission to user profile override
xcoulon Dec 8, 2017
ede7e01
fix: close resp.Body to avoid FD leak
xcoulon Dec 11, 2017
40e4623
Avoid FD leak after reading resp.Body
xcoulon Dec 12, 2017
06ade20
Avoid FD leak after reading resp.Body (attemtp#2)
xcoulon Dec 13, 2017
34a4fed
Merge branch 'master' into whitelist_profile_update
xcoulon Jan 15, 2018
85ac0b5
Use whitelist to restrict permission to user profile override
xcoulon Jan 16, 2018
2f01262
refactor tests
xcoulon Jan 16, 2018
1abdb8a
extract function to retrive current user from auth
xcoulon Jan 16, 2018
102434d
adding logs
xcoulon Jan 16, 2018
bd8bbda
fix cassette matcher and add logs
xcoulon Jan 16, 2018
e0bd158
refactor tests
xcoulon Jan 16, 2018
90c76db
refactor test
xcoulon Jan 16, 2018
488b055
more logs
xcoulon Jan 16, 2018
f9f7835
refactor cassette matcher, update go-vcr and more logs
xcoulon Jan 16, 2018
708fc53
more logs
xcoulon Jan 16, 2018
e6ae19b
rename go-vcr cassette folder to avoid being ignored in git
xcoulon Jan 16, 2018
47bc755
Review: remove double err check
xcoulon Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions controller/tenant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,16 @@ func (c *TenantController) loadUserTenantConfiguration(ctx context.Context, conf
return config, err
}
resp, err := authClient.ShowUser(ctx, auth.ShowUserPath(), nil, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close response body in defer and make sure the body has been read. Otherwise you will leak FDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, indeed! thanks, @alexeykazakov!

defer resp.Body.Close()

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove lines 202-205 I guess (duplicates 208-211)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, these 2 errors are different: err on line 201 comes from authClient.ShowUser() (i.e, something wrong happened while calling the auth service), whereas error on line 207 comes from reading the response body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to read the body here too because otherwise it may leak FD even if you close the body :)
General rule for making sure we don't have a FD leak - If err!=nil then read the body (once) and close it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let me move this line just after js, err := simplejson.NewFromReader(resp.Body), then. I assume this will respond to the golden rule you explained, right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

defer resp.Body.Close()
if err != nil {
   // return error
}
body := ioutil.ReadAll(resp.Body)
if resp.StatusCode < 200 || resp.StatusCode > 300 {
   // return error
}

// Unmarshal JSON from body.

Or

defer resp.Body.Close()
if err != nil {
   // return error
}
if resp.StatusCode < 200 || resp.StatusCode > 300 {
   ioutil.ReadAll(resp.Body)
   // return error
}

js, err := simplejson.NewFromReader(resp.Body)

Or simply use defer rest.CloseResponse(resp.Body) from https://github.com/fabric8-services/fabric8-auth/blob/master/rest/url.go#L46 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the explanations, @alexeykazakov. Am I wrong or does

func CloseResponse(response *http.Response) {
	ioutil.ReadAll(response.Body)
	response.Body.Close()
}

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 06ade20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand Down