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

Fix regexp in user/group name #82

Closed
wants to merge 2 commits into from
Closed

Fix regexp in user/group name #82

wants to merge 2 commits into from

Conversation

MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti commented Sep 14, 2016

Regexp should not enforce lenght of string, we have different checks for
that. Secondly regexp with length specified produces an incorrect error
message.

https://fedorahosted.org/freeipa/ticket/5822

@tkrizek tkrizek self-assigned this Sep 14, 2016
@stlaz stlaz assigned stlaz and unassigned tkrizek Sep 16, 2016
@stlaz
Copy link
Contributor

stlaz commented Sep 16, 2016

Would the gist of this patch also apply to any of idviews, servicedelegation or topology plugins where a similar or slightly different regexp is used? Could we rather have the regexp as a constant somewhere so it does not have to be changed at all places it appears every time we want to modify it?

@abbra
Copy link
Contributor

abbra commented Sep 16, 2016

'uid' in user object and 'cn' in group object have meaning in POSIX environments. 'cn' in other objects is not subject for strict limits.

@stlaz
Copy link
Contributor

stlaz commented Sep 16, 2016

Sure. But from what I see, this patch may possibly break idviews plugin for users of username of about 255 characters (see idoverrideuser - 'uid'), same for groups (with 'cn' ofc).
NACK until that is fixed.

Regexp should not enforce lenght of string, we have different checks for
that. Secondly regexp with length specified produces an incorrect error
message.

https://fedorahosted.org/freeipa/ticket/5822
User and groups regexp are the same and constant should be used to avoid
any future misconfigurations.

https://fedorahosted.org/freeipa/ticket/5822
@abbra
Copy link
Contributor

abbra commented Sep 20, 2016

LGTM. Thanks for first fixing the regexp and then replacing it by a constant, this will help with backports.

@abbra abbra added the ack Pull Request approved, can be merged label Sep 20, 2016
@stlaz
Copy link
Contributor

stlaz commented Sep 20, 2016

The tests seem to pass as well.

@martbab martbab added the pushed Pull Request has already been pushed label Sep 20, 2016
@martbab martbab closed this Sep 20, 2016
@MartinBasti MartinBasti deleted the fix-regexp-5822 branch October 4, 2016 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants