Implement login interface #77

jacobtylerwalls · 2022-09-22T22:09:46Z

Overview

Implement login interface.

Expose endpoints for login, logout, and other standard endpoints from dj-rest-auth
Add login and logout views at api/auth/login and api/auth/logout
Redirect users to login page when logged out
Redirect users to their originally requested page upon successful login
Avoid forcing users to login again when refreshing the page

Closes #66

Demo

% curl -X POST --data '{"email":"e","password":"p"}' -H "Content-Type:application/json" localhost:8181/api/auth/login/
{"email":["Enter a valid email address."]}

% curl -X POST --data '{"email":"a1@azavea.com","password":"p"}' -H "Content-Type:application/json" localhost:8181/api/auth/login/
{"non_field_errors":["Unable to log in with provided credentials."]}

% curl -X POST --data '{"email":"a1@azavea.com","password":"password"}' -H "Content-Type:application/json" localhost:8181/api/auth/login/

Notes

So far, I chose not to await the /login requests, so let me know if that is desired.
dj-rest-auth is the currently maintained fork of django-rest-auth, which will not be compatible with Django 4.
I opted for placeholder texts in the input boxes instead of labels, but I did add aria-labels
I figured it was harmless to retain the username field of the /login endpoint: it appears to work just fine (EDIT: well, it works using the dj-rest-auth endpoint, not the session-based django endpoint, and now only the django endpoint is exposed as of 8039f8f). Let me know if it's worth expending effort here to hide the username field.

Testing Instructions

Checklist

fixup! commits have been squashed
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
README.md updated if necessary to reflect the changes
CI passes after rebase

mstone121

Looks good! I have few questions mostly regarding the front-end.

src/app/src/components/PrivateRoute.js

src/app/src/App.js

mstone121 · 2022-09-23T17:56:56Z

src/app/src/constants.js

@@ -38,3 +38,8 @@ export const PANES = {
    MUNICIPAL_BOUNDARY_LABELS: { label: 'muni-labels', zIndex: 225 },
    USER_POLYGON: { label: 'user-polygon', zIndex: 490 },
 };
+
+export const URLS = {


Suggested change

export const URLS = {

export const API_URLS = {

We might eventually want to extract the frontend route names to a constant in which case URLS might be confusing.

mstone121 · 2022-09-23T18:01:00Z

src/django/api/views.py

+    def post(self, request, *args, **kwargs):
+        logout(request)
+
+        return Response(status=status.HTTP_204_NO_CONTENT)


Can we split this file up like we did models.py?

rajadain · 2022-09-23T18:05:09Z

Taking a look now

rajadain · 2022-09-23T18:25:00Z

src/app/src/App.js

 function App() {
    return (
        <BrowserRouter>
            <Flex>
                <Routes>
-                    <Route path='/draw' element={<Sidebar />} />
+                    <Route
+                        path='/draw'


Interestingly, /draw does redirect me, but /draw/ (which I usually use) does not.

This is concerning: apparently, this is the request axios is making, which obviously fails because there are no endpoints exposed under /draw/.

GET http://localhost:4545/draw/api/auth/login/ 404 (Not Found)

I'll troubleshoot on Monday, but let me know if anything comes to mind immediately.

Actually, c97adef seemed to do it.

rajadain · 2022-09-23T18:28:09Z

src/app/src/theme.js

+        loginError: {
+            fontSize: 'md',
+            alignItems: 'center',
+            color: 'red',


This could use some design input from @gatesgodin. It currently isn't legible against the gray background:

Also @gatesgodin if you have an opinion about using placeholder text for email and password instead of labels above the inputs I'm interested in that as well (see screenshot at top of PR).

rajadain

This is looking great! There's a couple of design changes to go through, but great job overall.

rajadain · 2022-09-23T18:54:54Z

src/app/src/pages/Login.js

+            <Button
+                w='320px'
+                variant='cta'
+                onClick={() => loginRequest(email, password)}
+            >
+                Login
+            </Button>
+            <Button variant='link'>Forgot password?</Button>


Consider wrapping these buttons in a smaller VStack since they should have less spacing between them

Suggested change

<Button

w='320px'

variant='cta'

onClick={() => loginRequest(email, password)}

>

Login

</Button>

<Button variant='link'>Forgot password?</Button>

<VStack spacing={2}>

<Button

w='320px'

variant='cta'

onClick={() => loginRequest(email, password)}

>

Login

</Button>

<Button variant='minimal'>Forgot password?</Button>

</VStack>

Where minimal is a new Button variant that matches the wireframes:

minimal: { fontWeight: 400, fontSize: 'sm', textDecoration: 'none', color: 'gray.800', },

@gatesgodin did we consider the possibility of underlining all text links in the app? My concern was that if we leave "Forgot password?" without underlining then we introduce a small inconsistency and a small accessibility/usability issue, but I'm persuadable, since "Forgot password?" is such a common web interaction.

I'm going to apply Terence's diff (thank you!), and we can always add underlining back later if desired.

@jacobtylerwalls Hey! If we change the login page background to Grey-50 and the error message to Red-600 it will meet AAA color contrast requirements.

For the links, I'm open to underline the 'Forgot password'. There aren't many links within this app, but I do agree that they should be underlined if any surface throughout the development.

For the form labels, typically if there are 2 or less fields I opt to have the label as the placeholder for a cleaner look. Although if the form field has specific formatting (like a phone number) or many fields, then I would say that it's important to show the fields. Feel free to add them in if time permits.

Great, I used red-600 and gray-50 in 4ccf042.

I'll add the underlining back in. This is the only text link so far:

src/app/src/pages/Login.js

rajadain

This looks really great! Nice job getting all the little details in.

I would recommend considering the URL redirect for logging in. Once that is in, this should be good to go!

src/app/src/pages/Login.js

rajadain · 2022-09-26T18:05:30Z

Taking a final look

rajadain

+1 tested, this looks really good! Great work on this, with all the fixup commits and being so responsive. Would recommend making sure the fixups merge correctly!

rajadain · 2022-09-26T18:12:07Z

src/django/api/views/auth.py

+class Login(LoginView):
+    permission_classes = (AllowAny,)
+
+    @method_decorator(ensure_csrf_cookie)


Is this required here? Since we already have the middleware installed?

iow-boundary-tool/src/django/iow/settings.py

Line 108 in 8ee6d54

'django.middleware.csrf.CsrfViewMiddleware',

It was required with my first attempt at axios integration, but I'll give it another look if still needed.

From the CSRF reference, it's not clear to me that the cookie is always sent with the response. Before posting the PR, I found that re-logging in at /login would sometimes fail, as the user was already in an authenticated session, and adding ensure_csrf_cookie fixed it. Later, using the correct axios integration seemed to take care of that, and plus--now you are redirected away from /login if you're logged in. So I'll take it out 👍🏻

rajadain · 2022-09-26T18:13:08Z

src/django/api/views/auth.py

+        if not request.user.is_active:
+            raise AuthenticationFailed("Unable to sign in")
+
+        return Response(status=status.HTTP_204_NO_CONTENT)


In the future we may change this to return some user details (like Role, etc) for the front-end. Fine for now.

jacobtylerwalls assigned rajadain Sep 22, 2022

jacobtylerwalls requested a review from rajadain September 22, 2022 22:22

jacobtylerwalls marked this pull request as draft September 23, 2022 13:47

jacobtylerwalls marked this pull request as ready for review September 23, 2022 14:11

mstone121 reviewed Sep 23, 2022

View reviewed changes

rajadain reviewed Sep 23, 2022

View reviewed changes

jacobtylerwalls requested review from mstone121 and rajadain September 23, 2022 22:08

rajadain requested changes Sep 25, 2022

View reviewed changes

src/app/src/pages/Login.js Outdated Show resolved Hide resolved

jacobtylerwalls mentioned this pull request Sep 26, 2022

Use spinner to avoid flashing UI components before redirecting to login #78

Closed

jacobtylerwalls requested a review from rajadain September 26, 2022 12:34

rajadain approved these changes Sep 26, 2022

View reviewed changes

jacobtylerwalls added 6 commits September 26, 2022 16:19

Style login screen

0f29fa5

Redirect users to login page when logged out

db8d4eb

Add dj_rest_auth endpoints for login, logout, etc

62f293c

Add login and logout views

a71a739

Enable logging in with Enter key

f33020c

Redirect to page originally requested upon login

59dd929

jacobtylerwalls force-pushed the jw/login branch from 87fc4af to b5e950a Compare September 26, 2022 20:30

jacobtylerwalls added 3 commits September 26, 2022 16:34

Add aria-label to email and password fields

c06d3d5

Update CHANGELOG

35e0ea3

Avoid rendering blank map component beneath login

6ab4143

jacobtylerwalls force-pushed the jw/login branch from b5e950a to 6ab4143 Compare September 26, 2022 20:34

jacobtylerwalls merged commit 1533a35 into develop Sep 26, 2022

jacobtylerwalls deleted the jw/login branch September 26, 2022 20:39

jacobtylerwalls mentioned this pull request Sep 28, 2022

Implement reset password functionality #79

Merged

4 tasks

jacobtylerwalls mentioned this pull request Oct 5, 2022

Replace leaflet map with static image in welcome page #102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement login interface #77

Implement login interface #77

jacobtylerwalls commented Sep 22, 2022 •

edited

Loading

mstone121 left a comment

mstone121 Sep 23, 2022

jacobtylerwalls Sep 23, 2022

mstone121 Sep 23, 2022

jacobtylerwalls Sep 23, 2022

rajadain commented Sep 23, 2022

rajadain Sep 23, 2022

jacobtylerwalls Sep 23, 2022

jacobtylerwalls Sep 23, 2022

rajadain Sep 23, 2022

jacobtylerwalls Sep 23, 2022 •

edited

Loading

jacobtylerwalls Sep 23, 2022

rajadain left a comment

rajadain Sep 23, 2022

jacobtylerwalls Sep 23, 2022 •

edited

Loading

jacobtylerwalls Sep 23, 2022

jacobtylerwalls Sep 23, 2022

gatesgodin Sep 26, 2022

jacobtylerwalls Sep 26, 2022

rajadain left a comment

rajadain commented Sep 26, 2022

rajadain left a comment

rajadain Sep 26, 2022

jacobtylerwalls Sep 26, 2022

jacobtylerwalls Sep 26, 2022

rajadain Sep 26, 2022

Implement login interface #77

Implement login interface #77

Conversation

jacobtylerwalls commented Sep 22, 2022 • edited Loading

Overview

Demo

Notes

Testing Instructions

Checklist

mstone121 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain left a comment

Choose a reason for hiding this comment

rajadain commented Sep 26, 2022

rajadain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls commented Sep 22, 2022 •

edited

Loading

jacobtylerwalls Sep 23, 2022 •

edited

Loading

jacobtylerwalls Sep 23, 2022 •

edited

Loading