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

[RORDEV-1259] User definition validation for duplicated entries with not matching credentials #1048

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mateuszkp96
Copy link
Collaborator

@mateuszkp96 mateuszkp96 commented Oct 26, 2024

🧐Enhancement (ES) Validation added for multiple username entries with different credentials in the users section


private type SupportedRule = AuthKeyRule | AuthKeyUnixRule | AuthKeyHashingRule

def similarity(x: BasicAuthenticationRule[?], y: BasicAuthenticationRule[?]): SimilarityMeasure = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation looks very nice, but I'm not sure if this is what the user needs.
IMO we should compare the username fields. In code, this is represented by UserIdPatterns case class.

We already have the withUserIdParamsCheck. It checks if the user defined in the auth rule matches the pattern defined in the usernme field. The user noticed that we have no validation of usenname patterns uniqueness.
In the client's example the validation won't help, because username in the auth rule is hashed.
But it seems there should be another validation, which can be defined like this:

  • if there are at least two the same usename pattens (without wildcard) it should failed to validate

And that's it. Is it? Or I'm missing sth?

TBH, the "username" seems to be a redundant setting. In case of auth rules with local users it seems that it is not necessary. In case of rules like LDAP auth we can use it to cut out some users. In case of local users it makes no sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants