-
Notifications
You must be signed in to change notification settings - Fork 54
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
Tachyon oauth #335
Tachyon oauth #335
Conversation
🎉 I will review it for sure (don't have perms to assign myself as reviewer) |
42afe49
to
898129a
Compare
issuer: base, | ||
authorization_endpoint: base <> ~p"/oauth/authorize", | ||
token_endpoint: base <> ~p"/oauth/token", | ||
token_endpoint_auth_methods_supported: ["none", "client_secret_post"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.rfc-editor.org/rfc/rfc6749.html#section-2.3.1
The authorization server MUST support the HTTP Basic authentication scheme for authenticating clients that were issued a client password.
I interpret that as client_secret_basic
effectively being required on this list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added support for basic auth in 19bc2c2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to not be resolved, I've tested with curl and I get missing client_id
when passing client_id via Basic auth:
curl --user generic_lobby: -d grant_type=authorization_code -s -X POST http://localhost:4000/oauth/token
13f0d02
to
f08f739
Compare
d973582
to
aff1262
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work with the authorization screen when user is not logged in? Will it first ask to login, then forward directly to auth screen?
lib/teiserver_web/templates/o_auth/authorize/authorize.html.heex
Outdated
Show resolved
Hide resolved
3693542
to
3514c24
Compare
Yes exactly. It'll preserve any potential querystring (so state stuff is there). |
3514c24
to
de255e3
Compare
Thanks for the review @p2004a , I believe I've addressed all your comments. Ping me on discord if you want me to restart the test server on my (tiny) vps, I've turned it off to save a bit of resources. |
de255e3
to
002fd82
Compare
Setup schema, a query on primary key and a test. No UI or write ops yet.
002fd82
to
3f0dbd9
Compare
Don't put a `?` symbol if there's no querystring
Screen for the user to grant access to the given client and then redirect with a freshly generated authorization code.
3f0dbd9
to
5a9385f
Compare
7b2a579
to
5ddcc77
Compare
I've done a bunch of testing today, the biggest problem is lack of support for mandatory HTTP Basic authentication scheme for all but client credentials grant, that's a blocker. A bunch of other things I've noticed that I don't think are hard blockers:
One more question: how are old codes/tokens etc cleaned up from db? |
One more question more on design side: I'm not sure I understand the relationship between autohost and application, why they are tied, can you elaborate on that design choice? |
I don't think an autohost is tied to an application. The schema only specify an id and a name (and some timestamps). About cleaning up old codes and tokens, I'll create an scheduled task to handle that later. For point 3: I wasn't aware you could change the scopes when refreshing a token. The scope param is ignored when refreshing, you get the same scope as the original token, regardless of the request. 4: I'll improve that. I left them deliberately vague "for security", but it's more a nuisance than anything. |
Ah,
So from https://datatracker.ietf.org/doc/html/rfc6749#section-6
and later
so effectively, you could theoretically reduce the scope of access token returned by endpoint, by specifying a subset of specified scopes. Quite weird, but ok. |
669c46d
to
bb132bd
Compare
Alright, I think I've got everything now, the last 6 commits should do the job. I've added some basic check if you provide a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Thank you!
@L-e-x-o-n Please PTAL and merge if all looks good from elixir side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all really well done.
I just have a few questions, but I think it's ready to be merged.
owner_id: user_id, | ||
application_id: attrs.id, | ||
scopes: attrs.scopes, | ||
expires_at: Timex.add(now, Timex.Duration.from_minutes(5)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@p2004a do you have any opinion on this? I'm personally neutral, it's easy enough to change in the code, but does require a recompilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion there is no need. The code is supposed to be used immediately by a program, not in any way by humans, so 5m is very generous and I don't think we would need to change it in a hurry. Shortening it also doesn't improve anything from security side.
value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), | ||
application_id: application.id, | ||
scopes: application.scopes, | ||
expires_at: Timex.add(now, Timex.Duration.from_minutes(30)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could be, also the refresh token, but IMHO we don't need to add it already in this PR. I think it is unlikely to change too and can't think about a good reason to have it different across instances.. maaaybe in dev setup for testing clients code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say ship it. If we need to tweak that for dev code, that's easy, and this PR is already massive enough.
Worst case, ping me and I can setup a teiserver on my vps.
bb132bd
to
95ed769
Compare
Because it's more obnoxious than anything to just get "invalid_request" when something goes wrong without any indication about the problem.
As per https://www.rfc-editor.org/rfc/rfc6749.html#section-2.3 > The client MUST NOT use more than one authentication method > in each request.
This is incomplete but until teiserver supports more than one scope it is not really possible to test that. It also doesn't really make sense.
95ed769
to
a479a14
Compare
This implements the two oauth flows required for running tachyon.
There's also at the time of writing, a demo server running at https://tachyon.geekingfrog.com:4567/login
There's currently only one OAuth application registered, with the hardcoded scope
tachyon.lobby
, one redirect uri:http://127.0.0.1/oauth2callback
and the client id isgeneric_lobby
.The lobby flow
This is using the
authorization_code
flow.getting an authorization code
First, the client need to generate a PKCE verifier and challenge.
For example:
Then, the client can request an authorization code with a
GET
request to the endpoint:https://tachyon.geekingfrog.com:4567/oauth/authorize?response_type=code&code_challenge=BGLMtLONQ_f6-Z6ikTk8ofWo-cWM3UUeT93LIEG33-M&code_challenge_method=S256&client_id=generic_lobby&redirect_uri=http%3A%2F%2F127.0.0.1%2Foauth2callback
You get redirected to a login screen.
The demo server has one user with email/password:
[email protected]
and password:tachyonmelon
You then should see a screen to grant access:
Clicking on "Let's go!" redirects to:
127.0.0.1/oauth2callback?code=<code>
exchanging the authorization code for an access token
You can then issue a
POST
request toand you get back
the autohost flow
I configured one client_id/client_secret pair that can be used to retrieve an auth token:
and this gives the same type of response.
Testing
There are automated tests under 2 location, one for the context and one for the controllers:
Out of (OAuth) Scope for this PR