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.

Sign up for GitHub

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

Jump to bottom

Fixes #24839: We cannot login with a user login containing uppercase letter if the option case-sensitivity is set to false #5655

Merged
merged 1 commit into from
May 6, 2024
Merged

Fixes #24839: We cannot login with a user login containing uppercase letter if the option case-sensitivity is set to false #5655

merged 1 commit into from
May 6, 2024

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented May 6, 2024 •
edited

https://issues.rudder.io/issues/24839

To load a user we both need to consider case-sensitivity when :

  • getting the user id from the database => we need to search with LOWER(id)
  • getting the user name from the users xml file (which is a map with the original file usernames as keys) => we search the key matching a case-insensitive comparison, there will only be one result because we already ignore duplicates when validating the file

@clarktsiory clarktsiory requested a review from fanf May 6, 2024 14:11
@clarktsiory clarktsiory marked this pull request as draft May 6, 2024 14:19
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as ready for review May 6, 2024 14:45
@fanf
Copy link
Member

fanf commented May 6, 2024

If two user login are the same modulo case and you set case-sensitivity to false, trying to log leads to an horrible stack trace :

[2024-05-06 17:36:11+0200] DEBUG sql - Failed Resultset Processing [10 ms total (0 ms exec + 9 ms processing)]
  select * from users where lower(id) = ? 
 arguments = [foo2]
   failure = Expected ResultSet exhaustion, but more rows were available.
        
[2024-05-06 17:36:11+0200] INFO  application - Rudder authentication attempt for principal 'foo2' with backend 'file': failure
[2024-05-06 17:36:11+0200] WARN  org.eclipse.jetty.server.HttpChannel - /rudder-web/j_spring_security_check
zio.FiberFailure: SystemError(Error when retrieving user information for 'foo2',doobie.util.invariant$UnexpectedContinuation$: Expected ResultSet exhaustion, but more rows were available.)
	at zio.interop.ZioMonadError.raiseError.trace(cats.scala:545)
	at .onError(ApplicativeError.scala:241:0)
	at .guaranteeCase(MonadCancel.scala:375:0)
	at com.normation.rudder.db.Doobie.transactIOResult(Doobie.scala:92)
	at bootstrap.liftweb.RudderInMemoryUserDetailsService.loadUserByUsername(AppConfigAuth.scala:410)
	at com.normation.zio.ZioRuntime.unsafeRun(ZioCommons.scala:445)

fanf
fanf reviewed May 6, 2024
View reviewed changes
...pp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala Outdated
val sql = sql"""select * from users where id = ${userId}"""
override def get(userId: String, isCaseSensitive: Boolean): IOResult[Option[UserInfo]] = {
val filter = if (isCaseSensitive) fr"id = ${userId} " else fr"lower(id) = ${userId.toLowerCase()}"
val sql = fr"""select * from users where""" ++ filter

transactIOResult(s"Error when retrieving user information for '${userId}'")(xa => sql.query[UserInfo].option.transact(xa))
Copy link
Member

Choose a reason for hiding this comment

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

here, you need to manage the case where there is more than one result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a well seen edge case !
✅ Fixed with 1c5f6d4 and a325e13, I made it throw an Inconsistency and added test for both in-memory and jdbc repository

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

1 similar comment
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

fanf
fanf approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Good!

@fanf
Copy link
Member

fanf commented May 6, 2024

OK, squash merging this PR

@clarktsiory @fanf
Fixes #24839: We cannot login with a user login containing uppercase …
7cc61e3 
…letter if the option case-sensitivity is set to false
@fanf fanf force-pushed the bug_24839/we_cannot_login_with_a_user_login_containing_uppercase_letter_if_the_option_case_sensitivity_is_set_to_false branch from a325e13 to 7cc61e3 Compare May 6, 2024 19:56
@fanf fanf merged commit 7cc61e3 into Normation:branches/rudder/7.3 May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf fanf approved these changes

Assignees
No one assigned
Labels
Ready for merge
Projects
None yet
Milestone
No milestone
2 participants
@clarktsiory @fanf