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

Remove unnecessary Model conformance on Authenticatable #35

Merged
merged 5 commits into from May 7, 2018

Conversation

3 participants
@jseibert
Copy link
Contributor

jseibert commented Apr 12, 2018

Vapor's approach to authentication is designed to be flexible, and the ability to authenticate arbitrary classes on each Request via try req.authenticated(T.self) is powerful.

However, there is no logical or technical reason that Authenticatable classes must be Model's. In fact, it's easy to imagine cases (e.g. God roles) where they shouldn't be.

This makes that possible.

@tanner0101 tanner0101 self-assigned this Apr 16, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Apr 16, 2018

@jseibert it might be even more flexible to not require Model on any of the protocols and instead require it in the extensions and middleware that actually rely on Fluent.

@jseibert

This comment has been minimized.

Copy link
Contributor Author

jseibert commented Apr 17, 2018

@tanner0101 even better! That makes total sense.

@jseibert jseibert force-pushed the jseibert:conformance branch from 6e3ce3c to 3bf38b2 May 7, 2018

@jseibert

This comment has been minimized.

Copy link
Contributor Author

jseibert commented May 7, 2018

@tanner0101 Ok, I just got time to update this. I removed the Model conformance on as many of the protocols as possible. Some, like Session and Token, require it implicitly, so we can't strip it from them.

@jseibert

This comment has been minimized.

Copy link
Contributor Author

jseibert commented May 7, 2018

Interesting, it builds on macOS but not Linux. Looking into it...

jseibert added some commits May 7, 2018

@jseibert

This comment has been minimized.

Copy link
Contributor Author

jseibert commented May 7, 2018

@tanner0101 OK, compilation finally works. This is probably the furthest it can be abstracted without significant rewrites to TokenAuthenticatable.

Test are now failing for an unrelated reason, so I'll put the ball back in your court :)

@tanner0101
Copy link
Member

tanner0101 left a comment

merging this in and i'll take a look at fixing up the tests. thanks!

@tanner0101 tanner0101 merged commit ad36cf5 into vapor:master May 7, 2018

1 check failed

ci/circleci: linux Your tests failed on CircleCI
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 7, 2018

Hey @jseibert, you just merged a pull request, have a coin!

You now have 1 coins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment