Introduced 'Remember me' functionality #470

vansika · 2018-12-17T10:10:35Z

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other

Problem

Some users note that they get logged out of ListenBrainz quite often.

Solution

Introduced a 'Remember me' checkbox and linked it with 'Sign in with Musicbrainz' button. Created a form to send checkbox data to server. If the checkbox is checked then the user will be logged in for specified duration even if the browser is accidentally closed.
Have cleared session before logging out to clear the 'Remember me' token that hinders signing out.

alastair

Hello!
Thank you for your contribution to ListenBrainz! When you create a pull request, please also add a link to a ticket if one exists (https://tickets.metabrainz.org/projects/LB/issues/LB-398?filter=allopenissues)

I'd like to make a few suggestions to this change. You'll note that in the ticket we added a number of items that should be checked before completing this task. It would be great if you could test the following items and add a comment to the ticket explaining what you learned:

How long is the default flask session time currently?
What is the behaviour of flask if we have a remember-me cookie set but we change the application secret key? Does this cause a login, or a logout?

As a next step, we can implement flask's remember_me option. There are a few items in the ticket that we didn't clarify, which we should have. Sorry about that. Keep in mind these things:

Use a 1 year timeout, like CatQuest suggested. It would be a good idea to make this explicit in case it changes in flask_login in the future
Yes, we need to use alternate tokens, as described in the documentation, this means we will need to make a database change and make some changes to the user model
We will need to disable session cookies on API requests
We don't need a checkbox letting the user select to use 'remember me', instead we should always use this setting. This will keep the login process simple.

As a side note, this is a good place to suggest that if you're unsure about what changes to make, you're absolutely welcome to add a comment to an open ticket asking questions, or telling us what you plan to do. This way we can give feedback before you start.

Please have a look at the initial questions that I mentioned above and see if you can answer them. Once this is done we can continue with the next steps to implement this functionality.

Thanks again for your contributions!

vansika · 2018-12-19T10:25:41Z

Hello @alastair
Thank you for the feedback. I will keep in mind to add appropriate links.

I have added comments to the ticket. Please have a look:)

Wouldn't it be good if we keep the remember me checkbox? Some users might wish to be remembered and some might not :)
The default remember me time is one year, though we can add it explicitly (through config.py) to be able to change it in future.
I will start working on the alternate tokens, and then other requirements after feedback from you.

Thank you.

vansika · 2018-12-23T13:44:19Z

Hello @alastair

I implemented alternate id, making changes in database and other files. Everything is working fine on localhost, when I change alternate id I get logged out otherwise the session is restored according to remember me cookie.
The logs seem fine yet there are errors while passing tests. The get_id method of user class is returning alt_id which is a string (have checked that) yet the tests say that it returns an integer.
Any help would be great.
Thankyou.

paramsingh

I think we can have a better name for the alt_id column, maybe something that describes how we're using it in the login process.

paramsingh · 2018-12-24T06:29:45Z

admin/sql/create_tables.sql

@@ -12,6 +12,7 @@ CREATE TABLE "user" (
 );
 ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_id_key UNIQUE (musicbrainz_id);
 ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_row_id_key UNIQUE (musicbrainz_row_id);
+ALTER TABLE "user" ADD COLUMN alt_id VARCHAR;


@vansika, the bug seems to be here, you're declaring the alt_id column as varchar, but then comparing it with integers later.

Also, this column should be inside the CREATE TABLE statement instead of being a seperate ALTER statement.

Please note that the automated tests on travis do a fresh build of the code you push, while your local environment may be different because of experimentation etc.

@paramsingh, get_id( ) passes alt_id to load_user( ) which is a string, but the redirects in tests are passing user_id to load_user( ), which is the cause of error because the check function (filter_by_alt_id( )) inside load_user( ) can return user by checking against alt_id (string) and not user_id (integer).
I will try to debug. Thank you

paramsingh · 2018-12-24T06:30:21Z

listenbrainz/config.py.sample

@@ -117,3 +117,6 @@ SPOTIFY_CALLBACK_URL = 'http://localhost/profile/connect-spotify/callback'
 # We do not use this feature at all, so it is safe to set to False.
 # See https://stackoverflow.com/a/33790196 for more details.
 SQLALCHEMY_TRACK_MODIFICATIONS = False
+
+# expiration of 'Remember me' cookie
+days = 365


This name can be more descriptive. SESSION_REMEMBER_ME_DURATION maybe.

paramsingh · 2018-12-24T06:30:53Z

listenbrainz/db/user.py

+             WHERE alt_id = :alt_id
+        """.format(columns=','.join(USER_GET_COLUMNS))), {"alt_id": alt_id})
+        row = result.fetchone()
+        print (row)


This print can probably be removed.

paramsingh · 2018-12-24T06:31:30Z

listenbrainz/webserver/views/login.py

@@ -29,7 +30,7 @@ def musicbrainz_post():
    if provider.validate_post_login():
        user = provider.get_user()
        db_user.update_last_login(user.musicbrainz_id)
-        login_user(user)
+        login_user(user, remember=True, duration=datetime.timedelta(days=config.days))


please use current_app.config['days'] here instead of importing config explicitly.

paramsingh · 2018-12-24T06:32:07Z

listenbrainz/config.py.sample

@@ -98,7 +98,7 @@ MAX_CONTENT_LENGTH = 16 * 1024 * 1024  # 16MB
 UPLOAD_FOLDER = "/tmp/lastfm-backup-upload"


-API_URL = 'https://api.listenbrainz.org'
+API_URL = 'http://localhost'


this change isn't needed, in my opinion

paramsingh · 2018-12-24T06:34:58Z

Also, we try to do descriptive commit messages in comparison to Update <file>. See this guide for examples: https://juffalow.com/other/write-good-git-commit-message

vansika · 2019-01-06T06:01:05Z

@alastair
I have implemented alternate tokens and the other requirements of the PR except disabling session cookies on API requests. API auth doesn't seem to involve flask-login, how exactly do i disable it.

paramsingh · 2019-01-08T19:08:11Z

admin/sql/create_tables.sql

@@ -12,6 +12,7 @@ CREATE TABLE "user" (
 );
 ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_id_key UNIQUE (musicbrainz_id);
 ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_row_id_key UNIQUE (musicbrainz_row_id);
+ALTER TABLE "user" ADD COLUMN user_login_id VARCHAR;


this column should be added inside the create table function call instead of creating the table and then altering it.

Also, it should be NOT NULL, I guess.

Also, If the only thing we're filling this column with is UUIDs, we should use the postgres uuid data type instead. (https://www.postgresql.org/docs/9.6/datatype-uuid.html)

This would make it more efficient. And you could set a default value for all users as well.

"uuid-ossp" extension is not being created due to some missing files. I will try to fix it and then would use UUID and postgres functions to generate user_login_id.

It seems that docker entry point need environment variables,defining them results in other authentication problems. Could you help find a way?

I've fixed the problem in #479. Thanks for the bug report! As soon as it gets merged, you can rebase this PR and everything should work fine. In the meanwhile, you can just install the extension in your "listenbrainz" db and continue.

merged now. =)

paramsingh · 2019-01-08T19:09:07Z

admin/sql/updates/2019-01-04-add-user-login-id.sql

+BEGIN;
+
+-- Add column for alternative user id for login purpose to user table
+ALTER TABLE "user" ADD COLUMN user_login_id VARCHAR;


we will need this script to fill out the column for existing users also, because each user must have a login ID, no matter what.

paramsingh · 2019-01-08T19:09:38Z

listenbrainz/db/user.py

+    """Get user with a specified login ID.
+
+    Args:
+        id (int): login ID of a user.


this should be updated because login ID of the user isn't an integer.

alastair · 2019-01-22T10:07:21Z

I have implemented alternate tokens and the other requirements of the PR except disabling session cookies on API requests. API auth doesn't seem to involve flask-login, how exactly do i disable it.

From the documentation, https://flask-login.readthedocs.io/en/latest/#disabling-session-cookie-for-apis, this step only appears to be necessary if we use header_loader or request_loader for authentication in the API. We don't use these, instead we do authentication manually using _validate_auth_header, so it looks like this step is unnecessary.

vansika · 2019-01-22T11:43:20Z

@alastair
Okay, thank you. Is there any thing else I should implement?

paramsingh

💯

vansika · 2019-02-03T16:41:59Z

😊

mayhem · 2018-12-17T10:40:10Z

listenbrainz/webserver/views/login.py

 @login_forbidden
 def musicbrainz():
+    if request.method == 'POST':
+        if 'check' in request.form:


You could re-write this to
session['check'] in request.form
That would be more pythonic. :)

mayhem · 2018-12-17T10:40:24Z

listenbrainz/webserver/views/login.py

    session.clear()
+    logout_user()


I'm curious, why this change?

Session variables need to be cleared before logging out so that remember me cookie is deleted. Otherwise, user won't be able to log out.

mayhem · 2019-02-08T17:54:11Z

listenbrainz/webserver/views/test/test_index.py

        self.assertIn('Your Listens', data)
        mock_user_get.assert_called_with(user['user_login_id'])

+        self.assertIn('My Listens', data)


We've changed "Your Listens" to "My listens", so the "Your Listens" are no longer needed.

UUID values

alastair · 2019-02-12T19:13:09Z

admin/sql/updates/2019-01-04-add-user-login-id.sql

+BEGIN;
+
+-- Add column for alternative user id for login purpose to user table
+ALTER TABLE "user" ADD COLUMN user_login_id UUID NOT NULL DEFAULT uuid_generate_v4();


I just noticed that there's no index on this field, there should be if we use it for lookups

alastair self-assigned this Dec 17, 2018

alastair reviewed Dec 18, 2018

View reviewed changes

paramsingh reviewed Dec 24, 2018

View reviewed changes

vansika force-pushed the master branch 3 times, most recently from 1c03d66 to c985f70 Compare January 5, 2019 04:49

paramsingh reviewed Jan 8, 2019

View reviewed changes

paramsingh mentioned this pull request Jan 17, 2019

Add uuid extension to the correct database #479

Merged

5 tasks

vansika force-pushed the master branch from c9fea70 to a1f0323 Compare January 19, 2019 09:54

paramsingh approved these changes Jan 31, 2019

View reviewed changes

mayhem reviewed Feb 8, 2019

View reviewed changes

vansika added 4 commits February 9, 2019 00:29

use alternate token for login

98f3a15

add value for session length to Config.py.sample

ca7a976

uses function to generate

eb2955e

UUID values

resolved merge conflicts

9a7f9a7

vansika force-pushed the master branch from 5c52e3e to 9a7f9a7 Compare February 8, 2019 19:20

paramsingh merged commit f313b0d into metabrainz:master Feb 11, 2019

alastair reviewed Feb 12, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced 'Remember me' functionality #470

Introduced 'Remember me' functionality #470

vansika commented Dec 17, 2018

alastair left a comment

vansika commented Dec 19, 2018

vansika commented Dec 23, 2018 •

edited

paramsingh left a comment

paramsingh Dec 24, 2018

paramsingh Dec 24, 2018

vansika Dec 28, 2018

paramsingh Dec 24, 2018

paramsingh Dec 24, 2018

paramsingh Dec 24, 2018

paramsingh Dec 24, 2018

paramsingh commented Dec 24, 2018

vansika commented Jan 6, 2019

paramsingh Jan 8, 2019

paramsingh Jan 8, 2019

paramsingh Jan 8, 2019

vansika Jan 14, 2019

vansika Jan 16, 2019

paramsingh Jan 17, 2019

paramsingh Jan 17, 2019

paramsingh Jan 8, 2019

paramsingh Jan 8, 2019

alastair commented Jan 22, 2019

vansika commented Jan 22, 2019

paramsingh left a comment

vansika commented Feb 3, 2019

mayhem Dec 17, 2018

mayhem Dec 17, 2018

vansika Feb 8, 2019

mayhem Feb 8, 2019

vansika Feb 8, 2019

alastair Feb 12, 2019

Introduced 'Remember me' functionality #470

Introduced 'Remember me' functionality #470

Conversation

vansika commented Dec 17, 2018

Problem

Solution

alastair left a comment

Choose a reason for hiding this comment

vansika commented Dec 19, 2018

vansika commented Dec 23, 2018 • edited

paramsingh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramsingh commented Dec 24, 2018

vansika commented Jan 6, 2019

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

alastair commented Jan 22, 2019

vansika commented Jan 22, 2019

paramsingh left a comment

Choose a reason for hiding this comment

vansika commented Feb 3, 2019

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

vansika commented Dec 23, 2018 •

edited