stormpath / Turnstile

An authentication framework for Swift.
Apache License 2.0
165 stars 29 forks source link

Make OAuth2 open so other modules can subclass it #11

Closed harlanhaskins closed 7 years ago

harlanhaskins commented 7 years ago

I'm making a custom OpenID Connect auth endpoint that's not unlike Google's authorization, but with custom login URLs (for Computer Science House).

I need to be able to subclass OAuth2 outside the TurnstileWeb module.

harlanhaskins commented 7 years ago

The Facebook test case is failing because there's no client ID set in the environment variable. Not sure how you want to proceed, given that. 🙃

edjiang commented 7 years ago

Conversation on Slack:

Edward Jiang (Stormpath) [7:09 PM]  
hey @harlan!

[7:09]  
just saw your PR

Harlan Haskins [7:09 PM]  
:smile:

Edward Jiang (Stormpath) [7:09 PM]  
I haven’t thought too much about subclassing just yet & the scenarios where things could break if I made all of the public classes open instead

Harlan Haskins [7:10 PM]  
I can definitely understand that

Edward Jiang (Stormpath) [7:10 PM]  
haha Swift 3 defaults

[7:10]  
make it more difficult for people consuming APIs

[7:10]  
but I’m curious what you’re trying to do? from what I read you were trying to make an OpenID connect server

Harlan Haskins [7:10 PM]  
Yep, but I think it promotes thoughtful discussion

Edward Jiang (Stormpath) [7:10 PM]  
and the OAuth class is more for being an OAuth client (edited)

Harlan Haskins [7:10 PM]  
I’m trying to interface with a Keycloak server that we just deployed

[7:10]  
From a Vapor web service

Edward Jiang (Stormpath) [7:10 PM]  
ah kk

[7:11]  
so you’re trying to be an OpenID Connect client

Harlan Haskins [7:11 PM]  
Yep!

Edward Jiang (Stormpath) [7:11 PM]  
this is functionality that I’d like to see in Turnstile in the near future

Harlan Haskins [7:11 PM]  
Me too :sweat_smile:

Edward Jiang (Stormpath) [7:11 PM]  
so — instead of subclassing (for now, let me think about it a bit overnight)

[7:11]  
would love to invite you to contribute and perhaps help write that functionality

Harlan Haskins [7:12 PM]  
I don’t know anything about OpenID Connect beyond what I’ve just implemented locally, unfortunately. But I could always learn!

Edward Jiang (Stormpath) [7:12 PM]  
=] that’s the spirit!

[7:13]  
but the other way

[7:13]  
you could do things w/o subclassing

[7:13]  
is just embedding the OAuth class inside another class

Harlan Haskins [7:13 PM]  
Yep, that would work fine

Edward Jiang (Stormpath) [7:14 PM]  
ok

[7:14]  
otherwise, if we make it open

[7:14]  
I think we should do a general find -> replace of all `public` with `open`

Harlan Haskins [7:15 PM]  
Well, for classes

Edward Jiang (Stormpath) [7:15 PM]  
yeah

Harlan Haskins [7:16 PM]  
Yep, so if we choose to allow subclassing, I’ll expand my PR

Edward Jiang (Stormpath) [7:19 PM]  
sounds great! either way, would love to see OpenID connect, so if you end up building it in your fork, I’d love to pull it back upstream

Harlan Haskins [7:20 PM]  
Awesome! I just saw how well it’s integrated into Vapor and I couldn’t pass it up!
edjiang commented 7 years ago

I think I'm going to make the Facebook test disable itself if the environment variables aren't set