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

Ignore case of the userid when validating it. #396

Merged
merged 3 commits into from
Jun 21, 2018
Merged

Ignore case of the userid when validating it. #396

merged 3 commits into from
Jun 21, 2018

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Jun 14, 2018

The authentication code ignores the case of the userid. The PR ensures the API user validation
code does also.

All userids are created by the authentication code in lowercase. Users can be manually created with the UI in mixed case. However the UI will not allow a second user to be created with a mismatching case.

If User exists and one attempts to create user the UI will report: Userid is not unique

To test:

  1. Set authentication mode to: Database
  2. From the UI create a new user with at least one uppercase letter in Username. e.g.: User
  3. Try to login with this user.

Note: The case of the value specified for Username on the login screen is ignored. So if User was create one could log in with User or user

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1590398

@jvlcek
Copy link
Member Author

jvlcek commented Jun 14, 2018

@miq-bot assign @gtanzillo

@jvlcek
Copy link
Member Author

jvlcek commented Jun 14, 2018

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Jun 14, 2018
@jvlcek
Copy link
Member Author

jvlcek commented Jun 14, 2018

@miq-bot add_label gaprindashvili/yes

@jvlcek
Copy link
Member Author

jvlcek commented Jun 14, 2018

@h-kataria Just FYI. Thank you for the help.

@jvlcek
Copy link
Member Author

jvlcek commented Jun 14, 2018

@gtanzillo I had considered using User.in_my_region.where but this Api code never limited the check to the current region and I felt that doing so might introduce failures elsewhere. When the backend authenticates it does limit the check to the current region so that case is covered.

Let me know if you feel I should use: User.in_my_region.where

Thank you, JoeV

gtanzillo
gtanzillo suggested changes Jun 14, 2018
View reviewed changes
lib/services/api/user_token_service.rb Outdated
@@ -75,7 +75,7 @@ def log_init(mod, name, options)
end

def validate_userid(userid)
raise "Invalid userid #{userid} specified" unless User.exists?(:userid => userid)
raise "Invalid userid #{userid} specified" unless User.where('lower(userid) = ?', userid.downcase).exists?
Copy link
Member

Choose a reason for hiding this comment

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

@jvlcek You should add the region scoping here. I think it was missed wth the original code and is a bug waiting to happen. We should only allow users that are defined in the current region to log in.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gtanzillo Wildo. Thank you.

@jvlcek
Copy link
Member Author

jvlcek commented Jun 15, 2018 •
edited
Loading

@skateman Please review to ensure the specS that I am removing from file spec/requests/service_dialogs_spec.rb align with the PR 17550 that you made to remove dialog_edit and dialog_new. Thanks man!

@jvlcek
Copy link
Member Author

jvlcek commented Jun 15, 2018

@gtanzillo @abellotti

This PR makes a 1 line code change in lib/services/api/user_token_service.rb and updated
to the related spec to exercise this change.

However Travis builds failed for 2 specs due to PR: ManageIQ/manageiq#17550,
that has removed 2 product features, dialog_new and dialog_edit

The updates to files config/api.yml and spec/requests/service_dialogs_spec.rb were
necessary to compensate for the changes made in PR: ManageIQ/manageiq#17550

Please review. Thank you! JoeV

gtanzillo
gtanzillo reviewed Jun 15, 2018
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

This all looks ok to me but I'd like to have @abellotti give it a look before I merge.

@gtanzillo gtanzillo requested a review from abellotti June 15, 2018 13:04
@himdel
Copy link
Contributor

himdel commented Jun 15, 2018 •
edited
Loading

The test fixes look good, if they get merged here, we can close #395 :)

EDIT: actually, sorry, no, this is removing features...

@skateman
Copy link
Member

@jvlcek I am okay with this, however, I'm starting to feel that I deleted an API feature by dropping those featurs 😞 so I think this is more @abellotti's call than mine.

@himdel
Copy link
Contributor

himdel commented Jun 15, 2018 •
edited
Loading

Please use #395 instead to fix the tests.
(as soon as @skateman reviews)

EDIT: the changes are coming here

@skateman
Copy link
Member

@romanblanco can you please verify that this doesn't break your dialog editor?

@himdel himdel mentioned this pull request Jun 15, 2018
Closed
@jvlcek jvlcek force-pushed the bz1590398_mixed_case_userid branch from cc0936b to 4349932 Compare June 15, 2018 15:04
@miq-bot
Copy link
Member

miq-bot commented Jun 15, 2018

Checked commits jvlcek/manageiq-api@2ef3d85~...4349932 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@jvlcek
Copy link
Member Author

jvlcek commented Jun 15, 2018

I'm pretty sure we have this correctly updated to align with the changes to the dialogs in manageiq.

@himdel @skateman @gtanzillo and @abellotti can each of you please take a quick look?

Thank you! JoeV

@romanblanco
Copy link
Member

@skateman tested, Dialog Editor seems to work correctly 👍

@abellotti abellotti assigned abellotti and unassigned gtanzillo Jun 21, 2018
@abellotti abellotti added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 21, 2018
@abellotti
Copy link
Member

LGTM!! Thanks @jvlcek for fixing this. 👍

@abellotti abellotti merged commit 956c0d5 into ManageIQ:master Jun 21, 2018
simaishi pushed a commit that referenced this pull request Nov 5, 2018
@abellotti @simaishi
Merge pull request #396 from jvlcek/bz1590398_mixed_case_userid
e1c4682 
Ignore case of the userid when validating it.

(cherry picked from commit 956c0d5)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638501
@simaishi simaishi mentioned this pull request Nov 5, 2018
Merged
@simaishi
Copy link
Contributor

simaishi commented Nov 5, 2018

Skipped backport of spec file (which doesn't exist in G branch) as per discussion with @jvlcek

Gaprindashvili backport details:

$ git log -1
commit e1c46824d09267eb9241e9af019bebc8887e181f
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Thu Jun 21 11:38:03 2018 -0400

    Merge pull request #396 from jvlcek/bz1590398_mixed_case_userid
    
    Ignore case of the userid when validating it.
    
    (cherry picked from commit 956c0d571a16bc36e2df271ae4cf6a8e802f316c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638501

@jvlcek jvlcek deleted the bz1590398_mixed_case_userid branch June 3, 2019 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

@abellotti abellotti abellotti approved these changes

Assignees

@abellotti abellotti

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jvlcek @himdel @skateman @miq-bot @romanblanco @abellotti @simaishi @gtanzillo