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

Allow re-authentication in editor after 401 response #4602

Merged
merged 2 commits into from
Dec 15, 2014
Merged

Allow re-authentication in editor after 401 response #4602

merged 2 commits into from
Dec 15, 2014

Conversation

jaswilli
Copy link
Contributor

@jaswilli jaswilli commented Dec 8, 2014

This PR supersedes #3492. The commit from #3492 has been rebased and is now working. There are some UX and other minor details that need to be sorted out:

A couple things I noticed right away (there are likely others as well):

  • There's currently no feedback after the re-authentication succeeds or fails.
  • Credentials need to be cleared from the controller after the re-auth attempt.
  • The modal needs to be styled.

@ErisDS
Copy link
Member

ErisDS commented Dec 9, 2014

@JohnONolan & @PaulAdamDavis this PR includes a new modal that allows you to sign back in on the editor in the event that you get logged out. It currently looks like this:

Needs some ❤️ 😉

<div class="password-wrap">
{{input class="password" type="password" placeholder="Password" name="password" value=password}}
</div>
<button class="button-save" type="submit" {{action "validateAndAuthenticate"}} {{bind-attr disabled=submitting}}>Log in</button>

This comment was marked as abuse.

@PaulAdamDavis
Copy link
Member

Along with changing the class on the button, add this to the bottom of core/client/assets/sass/components/modals.scss

The extra spacing under the title comes from #4632

// Modal login styles
.modal-body .login-form {
    display: block; // Override inherited `display: table-cell;`

    .email-wrap,
    .password-wrap {
        input {
            width: 100%;
        }
    }

    @media (max-width: 900px) {
        margin: 0 auto;
        max-width: 264px;

        .email-wrap,
        .password-wrap {
            width: 100%;
            margin: 0 auto 1em;
        }

        .btn {
            margin: 0;
            width: 100%;
            margin-bottom: 1em;
        }
    }

    @media (min-width: 901px) {
        .email-wrap,
        .password-wrap {
            width: calc(50% - 5px);
        }

        .email-wrap {
            float: left;
        }

        .password-wrap {
            float: right;
        }

        .btn {
            margin-top: 8px;
            float: right;
        }
    }
}

screen shot 2014-12-12 at 11 42 52

screen shot 2014-12-12 at 11 22 12

@@ -0,0 +1,14 @@
{{#gh-modal-dialog action="closeModal" showClose=true type="action" style="wide,centered" animation="fade"

This comment was marked as abuse.

@jaswilli
Copy link
Contributor Author

Thanks @PaulAdamDavis -- updated with those changes.

@jaswilli
Copy link
Contributor Author

What do we think about only allowing the re-auth to be against the user that was previously logged in by either pre-filling the username field (and making it readonly) or just only showing a password prompt?

It's really not appropriate to log in as a different user in this situation and from a technical standpoint--who knows what would happen. Basically we'd have to reset the application, which is exactly the opposite of what we're trying to accomplish here.

@JohnONolan
Copy link
Member

+1

@jaswilli
Copy link
Contributor Author

Updated to only allow re-auth as the previously logged in user.

I also added a notification after a successful re-auth because it felt really...abrupt without any feedback. But let me know if no notification is preferred.

Anyway, I think this in a pretty workable state right now so it'd be great if people could take it for a spin and see if there are any issues.

@jaswilli
Copy link
Contributor Author

Also, for anyone who wants to test this, you can get the modal to come up by opening the editor, deleting the access token from the database (delete from accesstokens;) and then clicking the save button in the editor.

@jaswilli
Copy link
Contributor Author

reauth

@jaswilli jaswilli changed the title [WIP] Allow re-authentication in editor after 401 response Allow re-authentication in editor after 401 response Dec 13, 2014
@ErisDS
Copy link
Member

ErisDS commented Dec 13, 2014

@jaswilli are you happy with this now?

Just saw you took the WIP tag off so I assume yes, Sorry!

@jaswilli
Copy link
Contributor Author

Yeah, I took off the WIP because it seems pretty good to me. Definitely open to feedback and more changes though.

@ErisDS
Copy link
Member

ErisDS commented Dec 13, 2014

I think it might be worth adding the user-select false CSS rule to the disabled input? Just because if you try to select the email and hit backspace it actually goes back and gets a bit confusing.

@ErisDS
Copy link
Member

ErisDS commented Dec 13, 2014

Otherwise seems to be working pretty awesomely!

@jaswilli
Copy link
Contributor Author

As far as I know user-select doesn't work on input elements. I could change the email address to be in a span instead and make it unselectable:

screen shot 2014-12-13 at 12 30 07 pm

@JohnONolan
Copy link
Member

I don't think we should display email at all, just "Please re-authenticate" (for the heading) and password field?

@jaswilli
Copy link
Contributor Author

@JohnONolan how about this?:

edit (I can change the header to please re-authenticate)
screen shot 2014-12-13 at 1 01 26 pm

@jaswilli
Copy link
Contributor Author

Screenshot from PR as it stands right now:
screen shot 2014-12-13 at 4 44 25 pm

@JohnONolan
Copy link
Member

Perfect - just need @PaulAdamDavis to followup with CSS on the form

@JohnONolan
Copy link
Member

Also what happens if I click on the X ?

@jaswilli
Copy link
Contributor Author

Also what happens if I click on the X ?

The modal just goes away and you're back in the editor. From there if you try to navigate away you'll get the "Are you sure you want to leave?" modal, or if you do something that triggers another 401 (i.e. saving) the re-auth modal will come back up.

@PaulAdamDavis
Copy link
Member

Here's some minor style changes (thought it would be clearer sharing the whole block instead of line-per-line comments.

It looks like this on desktop. Mobile view hasn't changed.

screen shot 2014-12-14 at 22 06 00

// Modal login styles
.modal-body .login-form {
    display: block; // Override inherited `display: table-cell;`

    .password-wrap {
        input {
            width: 100%;
        }
    }

    @media (max-width: 900px) {
        margin: 0 auto;
        max-width: 264px;

        .password-wrap {
            width: 100%;
            margin: 0 auto 1em;
        }

        .btn {
            margin: 0;
            width: 100%;
            margin-bottom: 1em;
        }
    }

    @media (min-width: 901px) {
        display: flex;

        .password-wrap {
            flex: 1;
        }
    }
}

@sebgie
Copy link
Contributor

sebgie commented Dec 15, 2014

Tested, looks very good to me 👍! When the CSS changes from Pauls comment are in, this is good to go.

marcoow and others added 2 commits December 15, 2014 14:39
Closes #2092
- Adds styling for re-auth modal.
- Prevent transition to posts route on success.
- Clear credentials from controller.
- Handle confirmAccept action if form is submitted via 'enter'.
- Only allow re-auth as the user that was previously logged in.
@jaswilli
Copy link
Contributor Author

Updated CSS is in. Thanks again, @PaulAdamDavis

sebgie added a commit that referenced this pull request Dec 15, 2014
Allow re-authentication in editor after 401 response
@sebgie sebgie merged commit 7a0fe7c into TryGhost:master Dec 15, 2014
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.

6 participants