clear user name and password on failed login attempt. #2514

ohadlevy · 2017-10-25T14:45:57Z

I've cleared both user and password, but maybe clearing
just the password is sufficent.

also, this is my first attempt in writing jasmin specs
any comments are welcomed

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

https://www.pivotaltracker.com/n/projects/1613907/stories/152252967

ohadlevy · 2017-10-25T14:47:21Z

ohadlevy · 2017-10-25T14:56:50Z

app/assets/javascripts/miq_application.js

@@ -722,6 +722,8 @@ function miqAjaxAuth(url) {
  })
  .then(null, function() {
    add_flash(__('Incorrect username or password'), 'error', { id: 'auth_failed' });
+    $('#user_name').val('');


I've noticed the service UI does not clear the username, in the sake of consistency it probably makes sense to do the same or change in both. any opinions? /cc @serenamarie125

ohadlevy · 2017-10-25T15:12:31Z

test failures seems unrelated?

chessbyte · 2017-10-25T15:15:35Z

See related PR in Service UI

serenamarie125 · 2017-10-25T17:49:09Z

@ohadlevy I can check if there are any conventions on the PatternFly side, but I would guess just clearing the password is enough.

Can you change the button label to "Log In" as part of this PR ?

ohadlevy · 2017-10-25T18:53:57Z

@serenamarie125 updated to only reset password field, changed all login buttons text (and hover) to "Log in"

serenamarie125 · 2017-10-26T01:16:16Z

@ohadlevy I did check with PatternFly, and although there is currently no "guidance" on what to do about clearing, the consensus is to add documentation around clearing BOTH the username and password upon failed login attempt.

My initial suggestion of only clearing password was around the user experience, meaning if i type in my password incorrectly, do I really have to retype both user name and password ?! But Catherine brought up the fact that she has had discussions with security folks around this, and that the suggestion was to clear both.

Sorry bout that! Thanks for updating the "Log In" as well for button label & tooltip!

ohadlevy · 2017-10-26T11:27:20Z

@serenamarie125 re-added user field. this is now also consistent with other projects (such as foreman) :)
I'll send a PR to the service-ui shortly

based on discussion on a similar PR on the ops UI (ManageIQ/manageiq-ui-classic#2514 (comment)) I'm suggesting we align across the two.

serenamarie125 · 2017-10-26T12:10:44Z

I see the updated video with the new behavior, but "Login" doesn't seem to be updated?

ohadlevy · 2017-10-26T12:16:05Z

@serenamarie125 I didnt update the video, but here is an image

h-kataria · 2017-10-26T12:59:40Z

@ohadlevy this looks good. Verified in UI.

ohadlevy · 2017-10-26T13:00:58Z

@h-kataria any idea why tests are failing? doesn't seem related - is there any better way to test this kind of change? thanks.

h-kataria · 2017-10-26T13:25:16Z

@ohadlevy spec test failures seem related to changes in this PR, maybe there is a syntax error in the html string. i will try to look into it.

h-kataria · 2017-10-26T13:28:55Z

spec/javascripts/miq_application_spec.js

+      it('removes password field form log in form', function () {
+        deferred = $q.defer();
+        // simulate authentication api call
+        spyOn(API 'get').andReturn(deferred.promise);


@ohadlevy comma missing here
spyOn(API, 'get').andReturn(deferred.promise);
that should fix the test failures

h-kataria · 2017-10-26T15:01:33Z

This is odd, same tests pass for me locally.

dclarizio · 2017-10-26T15:35:33Z

FWIW, I've seen all 3 scenarios on web based logins: clear both, clear password only, clear none. Since we only clear password in the SUI PR linked above, we should match that for now. Unless we want to change SUI to clear both.

I tend towards only clearing the password, to make it easier for the user, since the user can actually see the userid to know if it's correct.

ohadlevy · 2017-10-26T16:31:14Z

@dclarizio SUI was updated at ManageIQ/manageiq-ui-service#1162

Also renamed Login to Log in

miq-bot · 2017-10-26T17:49:04Z

Checked commit ohadlevy@5c0c24d with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 1 offense detected

app/views/dashboard/login.html.haml

⚠️ - Line 28 - Missing curly braces around a hash parameter.

ohadlevy · 2017-10-26T18:17:22Z

@h-kataria fixed the test failure issue - this is mostly due to the fact that our javascript test infrastructure does not support ES6 js / babel. going forward I would suggest to check alternatives to the way we currently do JS testing. hopefully its ready to merge now - thanks!

serenamarie125 · 2017-10-26T20:41:45Z

"Log In" should have a capital "I" @ohadlevy can you do a separate PR to update?

ohadlevy · 2017-10-29T10:21:54Z

@serenamarie125 fixed at #2539

himdel · 2018-01-31T17:34:43Z

spec/javascripts/miq_application_spec.js

+          deferred = $q.defer();
+
+          // simulate failed authentication api call
+          spyOn(API, 'get').and.callFake(function() {return deferred.reject();});


Sorry for the late response.. this spec doesn't do anything... (thanks @mzazrivec for noticing :))

Fixed in #3355, but just so you know :)

can't use inject(API) because this is not angular code - login is using vanillaJsAPI, not the angular service

can't mock API.get because it is not actually used, vanillaJsAPI.login is called and does the get internally

any time we use a promise.then/catch, the code is run asynchronously - and asynchronous expects are ignored in synchronous tests - missing a done param for the it function, jasmine uses the arity of the function to determine whether it's synchronous (arity 0) or asynchronous (arity 1 and you need to call the provided callback when done), so this was a synchronous spec with 0 expects

having the mock return a fake promise would make sense if we didn't have to wait, but we can't wait on a then.then.then of a promise without getting the new promise - so miqAjaxAuth needs to return the right promise so that we can wait on it

missing spy on miqClearLoginFields

badly escaped quotes-within-quotes-within-apostrophes (but we can't really use the onclick anyway)

ohadlevy force-pushed the reset-user-pass branch from cb92023 to 011c257 Compare October 25, 2017 14:48

ohadlevy commented Oct 25, 2017

View reviewed changes

ohadlevy force-pushed the reset-user-pass branch 2 times, most recently from b930de4 to f2e3c91 Compare October 25, 2017 18:52

mzazrivec added bug enhancement labels Oct 26, 2017

ohadlevy force-pushed the reset-user-pass branch from f2e3c91 to 3b0f116 Compare October 26, 2017 11:25

ohadlevy added a commit to ohadlevy/manageiq-ui-service that referenced this pull request Oct 26, 2017

clear user name field on failed login

cfa68cc

based on discussion on a similar PR on the ops UI (ManageIQ/manageiq-ui-classic#2514 (comment)) I'm suggesting we align across the two.

ohadlevy mentioned this pull request Oct 26, 2017

clear user name field on failed login ManageIQ/manageiq-ui-service#1162

Merged

h-kataria reviewed Oct 26, 2017

View reviewed changes

ohadlevy force-pushed the reset-user-pass branch from 3b0f116 to 2b3e093 Compare October 26, 2017 13:31

ohadlevy force-pushed the reset-user-pass branch 3 times, most recently from 80ccdc8 to fb6e25f Compare October 26, 2017 15:12

dclarizio added the rbac label Oct 26, 2017

dclarizio assigned h-kataria Oct 26, 2017

ohadlevy force-pushed the reset-user-pass branch from fb6e25f to ab806c1 Compare October 26, 2017 17:05

clear user and password on failed login attempt.

5c0c24d

Also renamed Login to Log in

ohadlevy force-pushed the reset-user-pass branch from ab806c1 to 5c0c24d Compare October 26, 2017 17:44

h-kataria added the fine/no label Oct 26, 2017

dclarizio removed the bug label Oct 26, 2017

h-kataria added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 26, 2017

h-kataria merged commit 7fe3ae7 into ManageIQ:master Oct 26, 2017

ohadlevy deleted the reset-user-pass branch October 26, 2017 18:26

ohadlevy mentioned this pull request Nov 14, 2017

Redirect to login screen on API 401 #2715

Merged

himdel reviewed Jan 31, 2018

View reviewed changes

himdel mentioned this pull request Jan 31, 2018

miq_application_spec - fix "login failed" spec to run the expects #3355

Merged

himdel mentioned this pull request Jan 21, 2019

Use cookie authentication against the API, drop the token renewal #5164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clear user name and password on failed login attempt. #2514

clear user name and password on failed login attempt. #2514

ohadlevy commented Oct 25, 2017 •

edited by mzazrivec

Loading

ohadlevy commented Oct 25, 2017

ohadlevy Oct 25, 2017

ohadlevy commented Oct 25, 2017

chessbyte commented Oct 25, 2017

serenamarie125 commented Oct 25, 2017

ohadlevy commented Oct 25, 2017 •

edited

Loading

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

h-kataria commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

h-kataria commented Oct 26, 2017

h-kataria Oct 26, 2017 •

edited

Loading

h-kataria commented Oct 26, 2017

dclarizio commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

miq-bot commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 29, 2017

himdel Jan 31, 2018

clear user name and password on failed login attempt. #2514

clear user name and password on failed login attempt. #2514

Conversation

ohadlevy commented Oct 25, 2017 • edited by mzazrivec Loading

ohadlevy commented Oct 25, 2017

ohadlevy Oct 25, 2017

Choose a reason for hiding this comment

ohadlevy commented Oct 25, 2017

chessbyte commented Oct 25, 2017

serenamarie125 commented Oct 25, 2017

ohadlevy commented Oct 25, 2017 • edited Loading

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

h-kataria commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

h-kataria commented Oct 26, 2017

h-kataria Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

h-kataria commented Oct 26, 2017

dclarizio commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

miq-bot commented Oct 26, 2017

ohadlevy commented Oct 26, 2017

serenamarie125 commented Oct 26, 2017

ohadlevy commented Oct 29, 2017

himdel Jan 31, 2018

Choose a reason for hiding this comment

ohadlevy commented Oct 25, 2017 •

edited by mzazrivec

Loading

ohadlevy commented Oct 25, 2017 •

edited

Loading

h-kataria Oct 26, 2017 •

edited

Loading