- 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?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
renyuneyun commented Oct 29, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Otto-AA commented Oct 29, 2023
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): fromsolid_file_pythonimportSolidAPIimportsolid_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 parameterdefget_auth_headers(url, method): returnsession.get_auth_headers(url, method) # create SolidAPI with the authentication methodsolid_api=SolidAPI(get_auth_headers) # now this internally uses the get_auth_headers methodsolid_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. |
renyuneyun commented Oct 29, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
peter0083 commented Oct 30, 2023
thank you both @Otto-AA and @renyuneyun for the input. I'm tagging @hrchu here for a review. |
hrchu commented Oct 31, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
sorry for late reply, just come back from PyCon APAC'23. I will check it later. |
hrchu commented Nov 1, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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:
|
hrchu commented Nov 1, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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
OidcAuthto 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:
yieldto send out the URL, and the caller doesfor login_url in auth.login(idp): ....loginfunction as two parts,login()andwaitLogin, and require the caller to sequentially call them.I can modify the code accordingly, after a decision is made.