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

Phase out password logins #394

Closed
hoyes opened this issue Mar 20, 2018 · 26 comments

Comments

@hoyes
Copy link
Member

@hoyes hoyes commented Mar 20, 2018

Some musings about removing passwords from Camdram, meaning only Facebook, Google and Raven logins will be supported. An e-mail went out on 18th Februray to which we've received no replies, and the manual "Create Account" form was broken for the month of January so I think we're safe to do this.

Phase 1

  • Delete the "Create Account" page and link
  • Delete the "Change Password" UI on the Account Settings page for users which don't already have a password assigned.
  • Tweak "reauthentication" page to redirect automatically to Raven etc where sensible.
  • Add fields/logging to monitor when each login method (Raven/Facebook/Google/password) is used.
  • Ensure Raven/Google/Facebook accounts can be matched by e-mail address as well as account id.

In the interim, monitor use of password logins and maybe do some manual mailouts to people who haven't migrated.

Phase 2

  • Remove password forms
  • Remove other password management UI (change password, forgotten password etc)
  • Delete password column
@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Mar 20, 2018

At the moment we ask for the following information from all users, even if their first login is from an external account:

  • Year of graduation
  • Are you a current University student or member of staff? (options: Cambridge University, ARU, other, no)

Wondering about removing these fields so that there's no need for an additional step/form after logging in with Raven/Facebook/Google for the first time. All the other information on the login form can be automatically pulled from the APIs.

These fields have existed since the dawn of time, but as far as I know haven't really been used or analyzed much. I suspect similar (albeit slightly less accurate) information could be derived from a user's e-mail address and sign-up date.

@hoyes hoyes added the task label Mar 20, 2018
@philosophicles

This comment has been minimized.

Copy link
Member

@philosophicles philosophicles commented Mar 21, 2018

Personally I also can't see any reason to keep those two user facts - you've got my vote.

I also can't remember ever using them. They don't tell us very much really; people can remain active in Cambridge drama beyond graduation, they can sometimes retain their Raven etc access by starting a higher degree etc, their "current student/staff" status becomes outdated...

I think there's a GitHub issue somewhere else about processing bouncebacks when a cam.ac.uk email address is no longer valid, and marking user records accordingly. That sounds like a much more useful attribute than these things, to me.

hoyes added a commit that referenced this issue Mar 25, 2018
hoyes added a commit that referenced this issue Mar 25, 2018
hoyes added a commit that referenced this issue Mar 26, 2018
@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jun 14, 2018

From a GDPR perspective it would probably be best to drop the two fields you mention @hoyes. I can't see us using them for anything so it would be difficult to justify storing them, plus the overhead of needing to keep them reasonably up-to-date.

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jun 14, 2018

There's already a load of v1-related DB removals pending a release in master so I'll add those in.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jun 14, 2018

I've just noticed that http://old.camdram.net is still up. Is this being served by a different database?

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jun 14, 2018

!!! @alexbrett ?

99% sure it's safe to disable the vhost for old.camdram.net - it's just the room booking API that needs to stay alive.

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jun 14, 2018

I've just taken old.camdram.net offline (again). Not sure what the story is there but can restore it easily enough if someone/something is still relying on it.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jun 14, 2018

Out of interest, was a conscious choice made to not support Twitter? To me Facebook, Twitter and Google are the big three, +Raven to make lives easier for UoC people. Not sure if there's a similar SSO system in place at ARU?

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jun 14, 2018

Not deliberate...I just picked three. It would be trivial to add anything listed here: https://github.com/hwi/HWIOAuthBundle so feel free to add.

Relatedly, I briefly pondered adding Windows Live too in relation to removing password logins. We check for an existing account with the same e-mail address when you log in using one of these, and last time I checked there were a handful of active accounts with Outlook/Hotmail e-mail addresses that might get orphaned once passwords are deleted.

I guess I was just trying to constrain the number of options ultimately. The "Create an account" link is now gone in master though so that frees up some space...

IIRC, ARU SSO is only possible using Shiboleth, which I figured wasn't worth the effort for the same reasons you described in #323 .

hoyes added a commit that referenced this issue Jun 16, 2018
@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jun 16, 2018

I added Twitter OAuth above. We already had API keys and and image so was straightforward.

Release 20180616 broadly completes Phase 1. We just need to figure out out when it's safe to delete all the password stuff.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jun 17, 2018

Seems to me like we need a way or working out who still uses password login, tracking/auditing this, contacting those people to tell them to link their account to an OAuth provider, and then finally dropping the relevant DB fields once everyone has done this (maybe at the same time as #395).

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 9, 2018

Some edges cases to maybe consider here after re-reading #270:

  • I've made it so that if you log in with OAuth we skip the e-mail confirmation step and mark is_email_verified as true straight away. The idea being that Facebook/Google etc have already validated your e-mail address... we should probably verify this. Exact information is a bit scarce but StackOverflow seems to say that Facebook disallows OAuth until the e-mail is verified but Google allows it, but returns an email_verified flag which we can copy across.
  • For Raven we currently assume that all new users have an associated cam.ac.uk e-mail address. I think this is safe (without a verification step) for current students/staff, but for "Raven for life" this isn't the case so we should an e-mail field on the first-time registration page. I'm pretty sure Raven passes back a flag telling you whether it's a "Raven for life" account or not.
  • Further to the above, we could likely detect the "Raven for life" flag on each login and unverify/remove the e-mail address if it changes.
hoyes added a commit that referenced this issue Jul 9, 2018
Also make registered/login fields nullable and include time
hoyes added a commit that referenced this issue Jul 9, 2018
hoyes added a commit that referenced this issue Jul 9, 2018
@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 9, 2018

So, it turns out I broke logging login dates in Februrary 😞 while doing #184. I've fixed that above, and also change the field from date to datetime.

I've also just added a couple of custom Mailout filters (that we can delete once this process is complete) that filter "active users without an active external login", the logic being:

  • Last login or registered field within last 2 years (so we'll still capture people who signed up since Februrary and only miss people who signed up > 2 years ago but didn't log in between 2 years ago and February).
  • Zero linked external_users

I separated out "@cam.ac.uk" and "other domains" into separate filters, the thinking being it might be nice to send slightly different content (the former being more biased towards Raven). I initially considered skipping cam.ac.uk altogether as we check for an e-mail match on your first Raven login, but figured that it would be better to educate people about using Raven instead of a password prior to removing the option.

Might be useful doing #387 in the near future so we can capture any bounces... And we should probably do a release soon so that we don't lose too much more login info.

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 9, 2018

Also, for reference, out of 806 "active users", there are 139 users with a cam.ac.uk but have only ever logged in using a password, 267 other users without an OAuth alternative, and 400 with an external_user in place. So the success rate is currently 50%...

Raven/Google logins have only really been useful since #184 was done in January, so there are probably a bunch of people (including cam.ac.uk users who graduated last year) who haven't maybe only logged in once or twice a year ago.

hoyes added a commit that referenced this issue Jul 9, 2018
…deprecations #394

Made password login form always redirect to account settings
@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jul 9, 2018

Raven via mod_ucam_webauth does set a AARequiredPtags flag for current/non-current users. Not sure if we're using that Apache module or a PHP-native implementation.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jul 9, 2018

The stats are pretty interesting. What classifies an active user versus an inactive one? I think it would be a pretty good idea to do a release soon so that we get that login notification banner up for people.

hoyes added a commit that referenced this issue Jul 10, 2018
hoyes added a commit that referenced this issue Jul 10, 2018
Make registration form handle a missing e-mail address
hoyes added a commit that referenced this issue Jul 10, 2018
@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 10, 2018

It's a custom PHP-native implementation, which was originally based off https://github.com/misd-service-development/raven-bundle but has diverged a lot... Found the relevant bit of the spec though and on https://development.camdram.net the e-mail field is now blank if you try to register using Raven for life.

"Active user" above = "logged in or registered at some point in the last 2 years", which I guess is quite broad.

Will probably do a release tomorrow, but after checking over the above changes on https://development.camdram.net wih a fine tooth comb...

@GKFX

This comment has been minimized.

Copy link
Member

@GKFX GKFX commented Jul 25, 2018

Just wondering what the motivation for this is? It’ll break the login system on the development servers and lock out at least some users. What are the intended benefits?

@philosophicles

This comment has been minimized.

Copy link
Member

@philosophicles philosophicles commented Jul 25, 2018

I think we might need to implement some special-case approach for the development server and local dev environments, perhaps? As @GKFX says above and I think I said somewhere else, the stock admin/user1/user2 accounts won't be able to work without passwords.

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 25, 2018

How about:

  • Enable Facebook, Google, Twitter logins on the dev site (with separate API keys to www)
  • Create a console command that allows upgrading an account to a Camdram admin (which was mentioned of #399 anyway). E.g. php app/console camdram:admin:create foo@bar.com. This would allow creating an admin on a local checkout.
  • Host a simple app on a separate subdomain (say devauth.camdram.net) which does an OAuth login on the dev site, then allows you to change the admin rights to that account, either via the console command above or a backdoor to the camdram_dev DB. By doing this bit out of repo it removes the risk of accidentally enabling such a feature on www, through a misconfiguration or code regression.
@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jul 25, 2018

Just wondering what the motivation for this is?

For anyone reading this: this question was discussed in private and a consensus was reached.

Host a simple app on a separate subdomain

Sounds like a good idea but might be a tad over-kill maybe? We all have shell access and could run the console command on Tiberius ourselves. Alternatively we could modify the Buddy pipeline so that it adds relevant records to the dev DB that allow the GitHub development team to login as administrators?

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Jul 25, 2018

Yea you're right... Open-to-all admin access isn't really a necessity. So just need create API keys on development and create the console command.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Jul 25, 2018

Yeah. Bearing in mind anyone other than the GitHub development team won't have push access to the repo (only merges from a personal fork) they'll have had to have test it locally first before hand.

@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Dec 8, 2018

I've just run

UPDATE `acts_users` SET `pass` = NULL WHERE `pass` IS NOT NULL AND (last_password_login_at < last_login_at OR last_password_login_at IS NULL)

on the DB. i.e.: remove passwords for anyone who's most recent login attempt didn't use the password, or otherwise hasn't used their password at all since the last_password_login_at field was added in July.

There are 10 passwords remaining.

@CHTJonas

This comment has been minimized.

Copy link
Member

@CHTJonas CHTJonas commented Dec 9, 2018

SELECT `acts_users`.* FROM `acts_users`
LEFT JOIN `acts_external_users` ON `acts_users`.`id` = `acts_external_users`.`id`
WHERE `acts_users`.`pass` IS NOT NULL
AND `acts_external_users`.`id` IS NULL

Looks like 7 of those 10 haven't set up some form of external login flow.

hoyes added a commit that referenced this issue Dec 15, 2018
hoyes added a commit that referenced this issue Dec 15, 2018
hoyes added a commit that referenced this issue Oct 15, 2019
hoyes added a commit that referenced this issue Oct 15, 2019
hoyes added a commit that referenced this issue Oct 24, 2019
@hoyes

This comment has been minimized.

Copy link
Member Author

@hoyes hoyes commented Oct 24, 2019

Task completed via #557

@hoyes hoyes closed this Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.