Move to ManagedAuthenticator

[angusmcleod]

This PR follows up on the discussion here. It does two things:

1. Turns the OAuth2BasicAuthenticator class into an extension of Auth::ManagedAuthenticator instead of Auth::OAuth2Authenticator. Mostly this involves moving the after_authenticate result and user handling to the standardised Auth::ManagedAuthenticator after_authenticate method.

2. Migrates the existing storage of associated user details from PluginStore to UserAssoicatedAccount.

The existing tests are all passing and existing implementations should not be affected.

[davidtaylorhq]

This is looking good - thanks @angusmcleod!

I've added a few small comments. The main thing is that we would like to keep database migrations as pure SQL, rather than using ActiveRecord.

[davidtaylorhq]

We like to keep migrations clear of any specific ruby code - that way they won't break if anything changes with PluginStoreRow or UserAssociatedAccount in future. It should be possible to do the migration in pure SQL, like

https://github.com/discourse/discourse-steam-login/blob/master/db/migrate/20190510055357_migrate_steam_auth_data.rb

You can even do the JSON parsing using something like

plugin_store_row.value::json->'user_id'

For now, let's just migrate the data to UserAssociatedAccount, and leave the old data in PluginStoreRows. Then later we can add a post deploy migration to delete the old data (needs to be post deploy so that authentication doesn't break for the few minutes during deployment).

[angusmcleod]

Ah cool. I've made this change 👍

[davidtaylorhq]

I'm curious why this was required. Do you have an authentication system which was returning "true" as a string, rather than a native boolean?

[angusmcleod]

tbh I can't recall, but yes, that was what this would have been guarding against. We could take it out, but do you feel it poses a risk leaving it in?

[angusmcleod]

I've taken out the cast 👍

[davidtaylorhq]

This should not be required. The auth object should be an instance of OmniAuth::AuthHash, which already supports indifferent access, and has also access via . syntax

[2] pry(main)> hash = OmniAuth::AuthHash.new({info: {name: "test"}})
=> {"info"=>{"name"=>"test"}}
[3] pry(main)> hash[:info]
=> {"name"=>"test"}
[4] pry(main)> hash["info"]
=> {"name"=>"test"}
[5] pry(main)> hash.info
=> {"name"=>"test"}

I suspect the problem here is that the tests are using normal hashes, rather than OmniAuth::AuthHash instances

[angusmcleod]

Yes, right on the money. I've moved the with_indifferent_access to the auth hashes in the tests

[davidtaylorhq]

Nitpick: the brackets are not required if there are no arguments. Ditto in a few other places

[angusmcleod]

Fair. I've removed these 👍

[davidtaylorhq]

As mentioned above, this should be changed to

Suggested change

	{ 'provider' => 'oauth2_basic',
	OmniAuth::AuthHash.new { 'provider' => 'oauth2_basic',

Then the tests will match production behaviour

[angusmcleod]

Yup also fair. I've made that change.

[davidtaylorhq]

Whitespace

[angusmcleod]

Removed!

[angusmcleod]

@davidtaylorhq Thanks for the detailed review! I've followed up on each piece of feedback. Let me know if there's anything else you'd like me to change.