#11371 - Password protection for fileserver repositories

#11371 - Password protection for fileserver repositories

[mathias, 2018-06-01 21:14:42]

I like the approach a lot, just some caveats to address here.

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
File modules/adblockplus/manifests/web/fileserver/repository.pp (right):

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:27: $auth_users = [],
This should be a Hash that allows for fine-tuning the File[$auth_file]
parameters, so one can either use the $source or $content parameters. You could
let it default to `undef` and use that in the condition in the template where
you decide whether to include the htpasswd configuration or not.

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:37: $auth_file =
"$adblockplus::directory/${name}_htpasswd"
I would prefer this to be either in `/var/www/${name}_htpasswd` (just to make it
easier to find) or in a sub-directory like
`${::adblockplus::directory}/htpasswd/${name}` (in order to not clutter the ABP
directory).

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:39:
nginx::hostconfig{ "$repository_host":
Nit: The white-space around the curly bracket is inconsistent with the other
Puppet code in this repository.

[f.lopez, 2018-06-02 18:29:41]

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
File modules/adblockplus/manifests/web/fileserver/repository.pp (right):

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:27: $auth_users = [],
On 2018/06/01 21:14:41, mathias wrote:
> This should be a Hash that allows for fine-tuning the File[$auth_file]
> parameters, so one can either use the $source or $content parameters. You
could
> let it default to `undef` and use that in the condition in the template where
> you decide whether to include the htpasswd configuration or not.

Acknowledged.

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:37: $auth_file =
"$adblockplus::directory/${name}_htpasswd"
On 2018/06/01 21:14:41, mathias wrote:
> I would prefer this to be either in `/var/www/${name}_htpasswd` (just to make
it
> easier to find) or in a sub-directory like
> `${::adblockplus::directory}/htpasswd/${name}` (in order to not clutter the
ABP
> directory). 

Acknowledged.

https://codereview.adblockplus.org/29795567/diff/29796572/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:39:
nginx::hostconfig{ "$repository_host":
On 2018/06/01 21:14:42, mathias wrote:
> Nit: The white-space around the curly bracket is inconsistent with the other
> Puppet code in this repository.

Acknowledged.

[mathias, 2018-06-05 15:02:58]

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
File modules/adblockplus/manifests/web/fileserver/repository.pp (right):

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:50: }, $auth_file))
This might be undef now, to indicate that there's no authentication setup, hence
it requires a conditional statement or similar.

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
File modules/adblockplus/templates/web/fileserver.conf.erb (right):

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
modules/adblockplus/templates/web/fileserver.conf.erb:5: <% if @auth_file -%>
Shouldn't this check for undef/nil instead?

[f.lopez, 2018-06-05 18:00:44]

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
File modules/adblockplus/manifests/web/fileserver/repository.pp (right):

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
modules/adblockplus/manifests/web/fileserver/repository.pp:50: }, $auth_file))
On 2018/06/05 15:02:58, mathias wrote:
> This might be undef now, to indicate that there's no authentication setup,
hence
> it requires a conditional statement or similar.

Acknowledged.

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
File modules/adblockplus/templates/web/fileserver.conf.erb (right):

https://codereview.adblockplus.org/29795567/diff/29797555/modules/adblockplus...
modules/adblockplus/templates/web/fileserver.conf.erb:5: <% if @auth_file -%>
On 2018/06/05 15:02:58, mathias wrote:
> Shouldn't this check for undef/nil instead?

Acknowledged.

[mathias, 2018-06-06 07:21:11]

LGTM.