[6.x] Credential managers

[JamesFreeman]

• [6.x] Refactor Credential Managers to use Data Providers #112
• [6.x] Create an Abstract Oauth2CredentialManagers Store #107
• [6.x] Add the ability to have a Model as a storage provider  #101

In this PR, I've refactored Credential managers, to all use one BaseCredentialManager, there was too much duplicated code. This will also make it easier for users to create their own managers. I thought this would be a good time to refactor to Data providers.

New Credential Managers

Authenticated user manager

• This will get the authenticated users xero data from a column (this can be changed in config) on the model.

Model manager

It's possible users want to be able point at their own models, for instance they might have a settings model where they want to store this data.

They'll be able to achieve this by calling the following code in their AppServiceProvider:

app(ActiveXeroModel::class)->setActiveModel(Settings::first());

It would also be possible to change the active model in their middleware. For instance, in my app, we have an "admin" which creates our invoices, but we also allow our clients customers to pay off their invoices via our web routes. (see #105 for more details)

[JamesFreeman]

@hailwood This one's now ready for your review. 🎉

[hailwood]

Hey @JamesFreeman,

Nice work here!

With app(ActiveXeroModel::class)->setActiveModel(Settings::first());, I wonder if we should make it more like how Laravel's first party packages let you configure custom models.

I.e. (feel free to come up with a better name)

ModelStore::useCredentialStorageModel(Settings::first());
ModelStore::getCredentialStorageModel();

Prior Art
Cashier

Cashier::useSubscriptionModel(Subscription::class);
Cashier::useSubscriptionItemModel(SubscriptionItem::class);

Sanctum

Sanctum::usePersonalAccessTokenModel(PersonalAccessToken::class);

Passport

Passport::useTokenModel(Token::class);
Passport::useRefreshTokenModel(RefreshToken::class);
Passport::useAuthCodeModel(AuthCode::class);
Passport::useClientModel(Client::class);
Passport::usePersonalAccessClientModel(PersonalAccessClient::class);

Additional Suggestion
We could look at configuring the model attribute the same way as Fortify does as well for more flexibility?

ModelStore::resolveCredentialsUsing(function(ModelStore $modelStore) {
    if($modelStore->model instanceof User) {
        return $modelStore->model->currentTeam->xero;
    } elseif($modelStore->model instanceof Setting) {
        return $modelStore->model->xero_credentials;
    }
    throw new Exception('Unknown Xero model store model: ' . get_class($modelStore->model));
});

// also support just setting the attribute (which would be the default)
ModelStore::resolveCredentialsUsing('xero_credentials');

What are your thoughts on these?

[JamesFreeman]

Really good feedback, I've now refactored the code to use a base Xero class - I think this is a good first step because I have some more plans to allow users to do easier testing in there apps:

Something like this:

Xero::assertAuthenticated('...');

I've gone with the following methods:

public static function useModelStorage(Model $model): void
public static function useAttributeOnModelStore(string $attribute): void
public static function getModelStorage(): Model
public static function getModelAttribute(): string

Just a note, I'll be on holiday from the 16th until the 30th, so if I'm mia, you'll know why :)

[JamesFreeman]

Hi @hailwood - I'm back from holiday now - do you have any thoughts on my changes?

[hailwood]

Hi @JamesFreeman,

I'll have some time to look at these this weekend 👍🏻

Cheers

[hailwood]

Looking good,
I really like how you've done the data providers 👌

Only two changes requested here - declaring dependencies on the abstract class, and a new custom exception (if you think that's a good idea) and then we're good to go!

[hailwood]

We should also declare oauthProvider and session as properties on the BaseCredentialManager too.

[JamesFreeman]

Thanks for the feedback, I've made those changes now. I've done an overall change to how the constructors work on the base credential manager, I resolve from the container instead of dependency injection - which has made things alot simpler in the tests and domain code.

I did have one small issue which I believe I've resolved correctly but would be good to get your feedback on. I needed to call the abstracts constructor from AuthenticatedUserStore but I couldn't do that via parent::__construct(); because it called it's inherited constructor from ModelStore - I've got around that by just statically calling BaseCredentialManager::__construct(); which appears to work. Wasn't sure if there was a way of doing this nicely?

[hailwood]

Hey @JamesFreeman,

I've added a couple of changes here.
I reckon on the AuthenticatedUserStore we just call the parent constructor anyway, but let me know if I'm missing something :)

[hailwood]

Rather than specifying the same message in multiple places, let's just make these the default values for the $message parameter across each of the exceptions.

[JamesFreeman]

I've made most of those changes now - I've left the Exception one as I want do something a bit more and I think we might be at a good point to split that out into a separate PR.

[hailwood]

Wicked, I'm happy then.
Shall we squash and merge it in?

[JamesFreeman]

Yeah, happy if you are.

[hailwood]

Very nice work, thanks James.