sagemathinc / cocalc

CoCalc: Collaborative Calculation in the Cloud
https://CoCalc.com
Other
1.17k stars 216 forks source link

auth/oauth1: support reading profile #5812

Open haraldschilly opened 2 years ago

haraldschilly commented 2 years ago

hub's auth.ts has a case for OAuth2, where if (userinfoURL != null) { ... } defines a .userProfile method. This overwrites https://github.com/passport-next/passport-oauth2/blob/master/lib/strategy.js#L288 … and well, this code by itself might or might not work 100%, but it's at least a vital step.

However, there is nothing like that for OAuth1.

The goal of this ticket is to refactor this a bit and add something like userinfoURLOAuth1 to code a similar case.

… and well, after reading the code, I wonder how the profile information from defining this userProfile callback even gets into the login_opts object to be used by cocalc. So, this ticket is a bit vague and needs some investigation.

belonesox commented 2 years ago

Actually, I implemented OAuth1 support, https://github.com/belonesox/cocalc/tree/belonesox-oauth1

and tested on my mediawiki with OAuth extension, even drop some instruction for the case. http://wiki.4intra.net/Blog:StasFomin/Connect_Cocalc_by_OAuth_with_MediaWiki_(MediaWiki4IntraNet)

But I am not ready to merge, I need to test all kind of OAuth authorizations… and I now dont understand architecture well.


For example, let discuss table passport_settings.

Is there is not possible how have, two or more strategy of one type with different parameters.

For example two oauth1 or oauth2 providers?

Because unique constraint on database forbid it.

Is it bug or feature?

haraldschilly commented 2 years ago

hello @belonesox … in https://github.com/sagemathinc/cocalc/pull/5790 I started to clean up all of this, and adding a new column for that database table, to pull apart the settings for cocalc and the ones for passport. I've also improved the typescript definitions for the fields in the jsonb objects, to make this clearer. To your question, yes sure, it is possible. Only the "strategy" column is a unique string, but you can call it as you like. I would use a short lowercase name, that resembles whatever this is pointing to. The conf.type entry defines which passport strategy constructor to use, i.e. in auth.ts there is extra_strategy_constructor, which gets this type as an argument. e.g.

strategy     |  conf 
-------------+---------------------------------
test1        | {"type" : "oauth1", ...}
acme         | {"type" : "oauth1", ...}
dataschool   | {"type" : "saml", ...}
...