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

File group provider #5028

Merged
merged 2 commits into from Sep 28, 2020
Merged

File group provider #5028

merged 2 commits into from Sep 28, 2020

Conversation

dain
Copy link
Member

@dain dain commented Aug 30, 2020

No description provided.

@dain dain requested a review from electrum August 30, 2020 19:52
@cla-bot cla-bot bot added the cla-signed label Aug 30, 2020
.. code-block:: none

group-provider.name=file
file.group-file=/path/to/group.txt
Copy link
Member

Choose a reason for hiding this comment

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

txt? We usually use json. I think using json make sense here because:

  • it is coherent with other file based configuration files
  • it is much easier to extend it in future
  • there is less parsing code

Copy link
Member Author

Choose a reason for hiding this comment

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

This is based on the Apache HTTP server format, which is the format we use for password files

Copy link
Member

Choose a reason for hiding this comment

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

We used the Apache password file format since it is a standard format and we could have people use htpasswd, rather than needing to write our own or have detailed instructions on how to create it. I don't think this applies here.

However, I can see the argument that this is a very simple mapping format that we do not expect to change, and using JSON is overkill.

``file.group-file`` Path of the group file.

``file.refresh-period`` How often to reload the group file.
Defaults to ``1m``.
Copy link
Member

Choose a reason for hiding this comment

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

I guess by default it shouldn't refresh, like it is implemented for cases like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

My goal is to have this consistent with password file, and people seem to like the refresh.

@@ -27,7 +27,7 @@ The following configuration properties are available:
==================================== ==============================================
Property Description
==================================== ==============================================
``file.password`` Path of the password file.
``file.password-file`` Path of the password file.

``file.refresh-period`` How often to reload the password file.
Defaults to ``1m``.
Copy link
Member

Choose a reason for hiding this comment

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

Oh. So this is where the default came from. Should we consider changing file based access rules to have this default set as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on feedback from david I changed the docs to 5s, and changed the file group provider to 5s also.


.. code-block:: none

group_name: user_1 user_2 user3
Copy link
Member

Choose a reason for hiding this comment

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

nit: user_3 for uniformity?

Copy link
Member

Choose a reason for hiding this comment

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

Not to bikeshed on the format, but I'd expect this to use commas, like the UNIX /etc/group file format:

group_name:user_1,user_2,user_3

This would be the same format, minus the group ID and password fields.

presto-docs/src/main/sphinx/security/password-file.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/security/group-file.rst Outdated Show resolved Hide resolved
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.prestosql.plugin.password.file;
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason to put a GroupProvider implementation in this package? this feature seems to be independent to the password authenticators. Should we create a separate package (or even module like other plugin implementations)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think they are close to a matched set, and will be commonly used together. Also, I don't think there is much of a problem since none of these plugins take up. much resources

@mosabua mosabua added the docs label Sep 17, 2020
@dain dain force-pushed the group-file branch 2 times, most recently from 2b43aea to 3bd05ea Compare September 24, 2020 03:11
@dain
Copy link
Member Author

dain commented Sep 24, 2020

All updated

@dain dain force-pushed the group-file branch 2 times, most recently from 6138b01 to 33defe8 Compare September 24, 2020 18:21
Copy link
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

Thanks for updating, looks good to me!

Fix property name in password file docs
Add missing config null checks
@dain dain merged commit 2fa1305 into trinodb:master Sep 28, 2020
@dain dain mentioned this pull request Sep 29, 2020
10 tasks
@dain dain deleted the group-file branch September 29, 2020 03:24
@martint martint added this to the 344 milestone Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants