Notes from the discussion today:

• separate options for name length
• clearer name for RegexInternal option
• treat max length setting of 0 as no limit
• treat blank regex as no regex check
• set the default regex values in kojixmlrpc

@mikem , @tkopecek all should be correct based on yesterday's discussions.

I would limit internal values to ASCII only (this one matches also unicode chars)

It is also worth some documentation in server setup.

regex could be compiled during reading options, so it is not compiled every time

:thumbsup: Only concern I've seen is MBS which is creating tags based on package names. In such case internal regexp would be the same as external. Not sure what we want to do about it but it should be part of another PR probably.

What is an "internal" name? Can we just call this a "resource" name?

I think we should remove the second sentence about "They can occur anywhere". I think most people will reasonably know that because that is how configuration files generally work.

Next step should be "external" ones - like package names where other rules (more general could be applied) But better name is still a good suggestion.

If "internal" names are different than vs "user" names, why is "user" in this list?

What are the default values if the values are commented out or deleted?

I think we should introduce another error class here, like "NameValidationError", instead of using the basic GenericError class for everything. It makes it difficult for callers to programmatically distinguish this (a user input problem) from anything else.

If a user sees a message that their chosen value "does not comply with the rules", that error message is is vauge and hard to understand at a glance. Would you please change this to "Name %s does not match RegexNameInternal setting %s"? That will make it much easier for the user to see the regex that the hub allows, and adjust their string accordingly.

I'm wondering why we give this much control to users over usernames?

Is it a security thing?

Why would users ever enable this setting?

What is an "internal" name? Can we just call this a "resource" name?

Internal means the field originates from Koji and so we can reasonably impose restrictions on it, unlike, say, the NVR fields where the data comes from systems outside of Koji.

I'm open to adjusting the terminology, but I'm not sure "resource" is quite right for what we're distinguishing.

If "internal" names are different than vs "user" names, why is "user" in this list?

user names were identified as not purely internal because they often come directly from external systems (i.e. kerberos). Hence we didn't feel we could apply the same rules to them as, say, tag names.

The inclusion in this list is probably just a typo, perhaps from an earlier version of the patch.

I'm wondering why we give this much control to users over usernames?

Is it a security thing?

Why would users ever enable this setting?

We're giving Koji system administrators control over what a valid username is. An admin might need this setting if their existing set of user names falls outside of our defaults.

I think we should introduce another error class here, like "NameValidationError"

There's a larger issue here of refactoring our exception classes. The tricky part is that adding a new class requires both server and client to know about it. If we add a new exception class, then a client might not know about it and will be unable to convert the Fault.

That's not to say we can't add new exception classes, but it would be better to add them in the client before we start returning them from the hub, if at all possible.

At any rate, I don't think this needs to be dealt with in this PR. No existing exception seems to be a better fit than GenericError for now.

What are the default values if the values are commented out or deleted?

Default values are set up in hub/kojixmlrpc.py. In hub.conf, there are commented out examples, how user can set up other rules for options.

I'm not sure about the docs placement. It's definitely good to document this, and it does make sense to have a doc page about the hub.conf options, but I'm not sure about starting out with a mostly incomplete doc that only covers the new options. It might be better to, for now, add these in the server howto doc, where some of the other options are currently documented. Later we could have a separate PR to pull out and expand that content into a complete doc.

Also, the config file is hub.conf, not kojihub.conf.

There are a couple places where verify_name_internal is applied for values that are effectively user names.

In new_group, we should check verify_name_user, because groups are implemented as a special type of user.

In addHost, we have a similar situation. Hosts get user entries with the same name, so we should apply user rules here. It might actually make sense to apply both.

I was a little puzzled as to why we reorder the word when we assign RegexNameInternal to the local var name_regex_internal (and similarly RegexUserName → name_regex_username).

I was a little surprised that we store the compiled form in context.opts. I guess it's good to compile at load time so that we'll have a startup error on bad config, but it does make the field somewhat misnamed.

The error messages "...does not comply with the rules" could be more specific. Would be good to indicate which configuration option is causing this.

I'm not sure about the docs placement. It's definitely good to document this, and it does make sense to have a doc page about the hub.conf options, but I'm not sure about starting out with a mostly incomplete doc that only covers the new options. It might be better to, for now, add these in the server howto doc, where some of the other options are currently documented. Later we could have a separate PR to pull out and expand that content into a complete doc.

Also, the config file is hub.conf, not kojihub.conf.

Mike, I have a plan to continue with this doc page after merge this PR for document hub.conf options, not only this one :) .

@tkopecek @mikem @ktdreyer all should be fixed, if it is not fixed, I commented here more up.

I see you've renamed the docs file. However the title line and first paragraph still refer to kojihub.conf.

It would be good to
1. clearly indicate that this file is incomplete and a work in progress
2. link it from the portion of server_howto that talks about hub.conf

A few grammar adjustments

is {+a+} standard .ini-like configuration file

contains {+the+} following options

-Verification host names is in the internal names and user name group, because we are using both
-verification there.
+Host names are listed in both groups because hosts always have an associated user entry.

The error messages are better, thank you, but I don't know if we need to include the actual regex. Maybe? On one hand, it might help the user self correct, on the other, it might make the error too busy.

In any case, we don't need to terminate the message with a period. Our other error messages do not do that.

A global in the koji namespace is the wrong place to store instance specific configuration data. Furthermore, it is not thread-safe.

Also, you don't set these values in the opts['Regex*'] == '' cases, which would lead to NameErrors. However, the unit tests pass. This suggests we are missing an important test case.

There is not a 100% ideal solution, but I think if we're going to compile the regexes at startup time, then storing them in opts under an alternate name is probably best. Perhaps RegexNameInternal.compiled or similar?

This seems to access context.opts['RegexNameInternal.compiled'] unconditionally, but it is not set if RegexNameInternal == ''. We can probably just wrap the check in if context.opts['RegexNameInternal]. Similar for RegexUserName.compiled.

I'm not sure the page title is the best place to mark the doc in progress. When the docs are viewed it shows in the nav column.

I think I'd prefer to just have a short section at the top that indicates the status. Perhaps something like:

Incomplete document
^^^^^^^^^^^^^^^^^^^
This document is a stub and does not cover all options.
Work to complete this document is tracked in `Issue 3073 <https://pagure.io/koji/issue/3073>`_
The old :doc:`Server HOWTO <server_howto>` doc also describes some hub configuration options.

/tmp/tmp_zel_vjq/docs/source/server_howto.rst:780: WARNING: unknown document: <hub_conf>

I think you need to include a title for the form of reference you've used.

Also, I've filed #3073 for the follow up doc work

@mikem updated and I taken #3073 and added to the next sprint.

The docs changes look good, but I don't see where you addressed this part:

This seems to access context.opts['RegexNameInternal.compiled'] unconditionally, but it is not set if RegexNameInternal == ''. We can probably just wrap the check in if context.opts['RegexNameInternal]. Similar for RegexUserName.compiled.

Ha, sorry, now updated with this change also. :-)

:thumbsup:

In the QE testing was found a problem, when user create for example user by cli/hub, and there is used some regex. After create it, user can see this new user in list of users on webUI and when he click to the user for more info, he can get an error message 'koji.GenericError: Invalid int/label value: xxx', because regex for webUI is not the same as for API/CLI. Because tomorrow is code freeze, we don't want to release it with some quick fix. There could be some next hidden problem with limit on name value.

Commit c160e483 fixes this pull-request