-
Notifications
You must be signed in to change notification settings - Fork 12
[WIP] feat: Support OIDC login using solid-oidc-client #38
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
base: master
Are you sure you want to change the base?
Conversation
Hi, congrats for getting it to work together! 👌 Regarding the dependency / library question: I think it would make sense to not include flask and solid-oidc-client as dependencies, but rather let the library user create the auth class (or function) and let them pass it as a parameter to the SolidAPI constructor. This would be similar to how I implemented it for the solid-file-client in javascript, where the fetch method is passed to the constructor. So the usage would be (pseudocode): from solid_file_python import SolidAPI
import solid_oidc_client
# do all the login stuff (including opening the url, the oauth redirect, etc)
solid_oidc_client.login(...)
session = solid_oidc_client...
# create fetch/object to pass as parameter
def get_auth_headers(url, method):
return session.get_auth_headers(url, method)
# create SolidAPI with the authentication method
solid_api = SolidAPI(get_auth_headers)
# now this internally uses the get_auth_headers method
solid_api.fetch(...) The benefits of the separation would be:
The disadvantages:
An open question would be, how it should be passed in. As a method that creates headers, a class instance, etc. |
Hi @Otto-AA , thanks for the comments.
That makes sense. The README and code documentation should be updated accordingly to prompt the developer for that.
The current usage is similar to what you described, just in a different organization. See the updated main body for the code. One possibility in being more flexible is to extend the parameters for the
Essentially, this provides a branch when caller wants to handle it separately. Otherwise, use the original/built-in mechanism. |
thank you both @Otto-AA and @renyuneyun for the input. I'm tagging @hrchu here for a review. |
sorry for late reply, just come back from PyCon APAC'23. I will check it later. |
Nice work! I agree that this PR is not flexable enough for all scenarios, but at least we can address a few scenarios like let Python developers access Solid Pod with this PR. Here are some points I can think of to make it even better:
|
Further thoughts about supporting OIDC login:
|
This PR aims to support Solid-OIDC authentication, using the solid-oidc-client library as mentioned in #33.
Essentially, it provides a new class
OidcAuth
to handle that. The rest of the library should be used in the same way as before.Discussion
Usage
Dependency
The dependency is not added, as I'm not entirely sure which place should the modification be placed. I tested against NSS and CSS. The added dependencies are:
Login URL handling
At the moment, it will print out a URL in the terminal, and the user should separately visit that URL and perform auth on the browser. The library will receive the callback automatically, and perform further steps.
This is not flexible enough for library users. In particular, the URL (
login_url
) should be emitted to the caller of the login function, and the caller should determine how this URL is used, e.g. printing to console (for CLI app), or automatically redirecting to the URL (for browser app).I can think of two ways, and am not sure which is preferred:
yield
to send out the URL, and the caller doesfor login_url in auth.login(idp): ...
.login
function as two parts,login()
andwaitLogin
, and require the caller to sequentially call them.I can modify the code accordingly, after a decision is made.