IA-only auth flow #526

mekarpeles · 2017-08-02T01:59:41Z

Note: Before accepting, we need to CR and checkin code into olsystem

Todo:

some naming needs to change.
test cases required

Test flows:
Account Creation

Attempt to create an account on OpenLibrary.org w/ the email of an existing Archive.org user (e.g. foo@archive.org)
Attempt to create an account on OpenLibrary.org w/ the email of an existing Open Library username
Attempt to create an account on OpenLibrary.org w/ with an email that doesn't exist on IA or OL (e.g. foo+test1@archive.org)

Login

Attempt to login w/ valid OL credentials (the following should all fail):

try w/ an OL account which hasn't been activated
try w/ an OL account which is blocked
try w/ an OL account which is linked
try w/ an OL account which is not linked

Attempt to login w/ an email that doesn't exist on IA or OL (this should fail -- no such account)
Attempt to login w/ a valid IA account...

which isn't verified/activated (should fail)
which is already linked to an OL account (should succeed)
which is unlinked & where no OL account exists with matching email (should succeed)
which is unlinked & shares the same email as an OL account which is also unlinked (should succeed)
which is unlinked & shares the same email as an OL account linked to another account (this should fail!)

bfalling · 2017-08-02T07:50:27Z

openlibrary/accounts/model.py

+        # `ol_account` shares the same email as our IA account and
+        # thus can and should be safely linked to our IA account, or
+        # (c) no `ol_account` which is linked or can be linked has
+        # been found and therefore, asuming


Misspell. Should be "assuming".

bfalling · 2017-08-02T07:56:41Z

openlibrary/accounts/model.py

+                except ValueError as e:
+                    return {'error': 'max_retries_exceeded', 'msg': str(e)}
+
+                if not ol_account.itemname:


Under which conditions would ol_account.itemname be true-y? We've already determined that there was no ol_account going in to this branch, and OpenLibraryAccount.create() will return an error dictionary if it fails, won't it? (Or is there a case where it will return None?) Cuz if what I say is correct, then seems like if not ol_account.itemname will always be true, since it's always dealing with a newly created, therefore unlinked ol_account.

You're right, there shouldn't be a case possible, I added this because a function had been removed between branches causing it to appear as if the account did not have an itemname set after login (when really the function to fetch the itemname was indeed the problem)

bfalling · 2017-08-02T08:06:33Z

openlibrary/accounts/model.py

            if not ol_account.verified:
-                return {'error': 'account_not_verified'}
+                if lending.config_ia_auth_only:


I think you can slightly simplify the logic by consolidating this if with the occurrence of if lending.config_ia_auth_only just above. If you go that route, you'll need to move around the if not ol_account.verified logic.

I think this is about the same level of complexity in either case as ol_account.verified would then have to be checked in 2 locations instead of lending.config_ia_auth_only

bfalling · 2017-08-02T08:07:32Z

openlibrary/accounts/model.py

+                    # call to safely activate them.
+                    ol_account.activate()
+                else:
+                    return {'error': 'ia_account_not_verified'}


At this point, wouldn't it be the OL account which is not verified (for dev usage)? (Of course, this logic may shift if you follow the previous suggestion.)

Yes, you're right

bfalling · 2017-08-02T08:09:59Z

openlibrary/accounts/model.py

+                    # call to safely activate them.
+                    ol_account.activate()
+                else:
+                    return {'error': 'ia_account_not_verified'}
            if ol_account.blocked:


Maybe add comment. This is for the new OL account.

i.e. We already checked for blocked accounts up above, but you might comment that this is for the newly fetched OL account.

bfalling · 2017-08-02T08:13:23Z

openlibrary/accounts/model.py

+                # When enabled, this will prevent users from logging in
+                # with their Open Library credentials and instead require
+                # Internet Archive (Archive.org) credentials.
+                return {'error': 'ia_login_only'}


Why wait this long to shut out OL logins when in production? In this current code, if the IA login fails, then the system still makes a db call to OL and an HTTP call to IA and then realizes "Oh, hey, I'm in production. I should return an error." Seems these should be moved up.

Removed this flow entirely to simplify path of execution

bfalling · 2017-08-02T08:16:29Z

openlibrary/accounts/model.py

@@ -681,13 +736,15 @@ def audit_accounts(email, password, test=False):
            ol_account.link(audit['link'])
            stats.increment('ol.account.xauth.auto-linked')


Seems like this whole branch starting with # Links the accounts if they can be and are not already is not needed for the production case. Haven't we already hit every case up above?

Yes, this was an attempt to fix the problem where user logged in and it appeared as if they were not linked (when really the function to check link status had a regression). Resolved

bfalling · 2017-08-02T08:20:45Z

openlibrary/templates/account/not_verified.html

@@ -23,7 +23,7 @@
 <p>If you can't find the email, just hit this button to send a fresh one:</p>

 <form method="POST" class="olform">
-    <input type="hidden" name="username" value="$username"/>
+    <input type="hidden" name="email" value="$email"/>
    <input type="hidden" name="password" value="$password"/>
    <input type="hidden" name="action" value="resend_verification_email"/>



Please make sure all these forms are in the test plan.

bfalling · 2017-08-04T03:28:06Z

LGTM

LeadSongDog · 2018-02-01T21:08:02Z

@mekarpeles Did this change not disable the user's ability to set preferences, such as interface language? I can't even find the menu anymore.

mekarpeles requested a review from bfalling August 2, 2017 01:59

bfalling requested changes Aug 2, 2017

View reviewed changes

mekarpeles force-pushed the ia-auth-only branch from da056e0 to 866cfca Compare August 3, 2017 22:19

bfalling approved these changes Aug 4, 2017

View reviewed changes

mekarpeles added 7 commits August 6, 2017 22:48

[WIP] IA-only auth flow

0a7fb98

Simpified the IA login flow, removed deprecated modal methods

e6add47

Fixing account linking and email exists cases

996a4ab

Fixing account creation verification to check emails against IA

d3dc097

Ignoring find_ia_account in account test

47f6c75

Updating the account creation to mirror archive.org

fce65ca

repairing tests

fce7901

mekarpeles force-pushed the ia-auth-only branch from 72b5f1a to fce7901 Compare August 6, 2017 22:48

removing unused error msgs

e7f9676

mekarpeles merged commit 413ee7e into master Aug 7, 2017

mekarpeles deleted the ia-auth-only branch April 22, 2019 01:49

mekarpeles changed the title ~~[WIP] IA-only auth flow~~ IA-only auth flow Jan 23, 2021

mekarpeles mentioned this pull request Jan 23, 2021

Should be a way to "resend" welcome/activation email #608

Closed

scottbarnes mentioned this pull request Aug 10, 2024

Delete unused admin, loan, and login templates and endpoints #9722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IA-only auth flow #526

IA-only auth flow #526

mekarpeles commented Aug 2, 2017

bfalling Aug 2, 2017

bfalling Aug 2, 2017

mekarpeles Aug 2, 2017

bfalling Aug 2, 2017

mekarpeles Aug 2, 2017

bfalling Aug 2, 2017

mekarpeles Aug 2, 2017

bfalling Aug 2, 2017

bfalling Aug 2, 2017

bfalling Aug 2, 2017

mekarpeles Aug 3, 2017 •

edited

Loading

bfalling Aug 2, 2017

mekarpeles Aug 3, 2017

bfalling Aug 2, 2017

bfalling commented Aug 4, 2017

LeadSongDog commented Feb 1, 2018

		@@ -681,13 +736,15 @@ def audit_accounts(email, password, test=False):
		ol_account.link(audit['link'])
		stats.increment('ol.account.xauth.auto-linked')

IA-only auth flow #526

IA-only auth flow #526

Conversation

mekarpeles commented Aug 2, 2017

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

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

mekarpeles Aug 3, 2017 • 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

bfalling commented Aug 4, 2017

LeadSongDog commented Feb 1, 2018

mekarpeles Aug 3, 2017 •

edited

Loading