Open renyuneyun opened 11 months ago
Hi, congrats for getting it to work together! :ok_hand:
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.
to not include flask and solid-oidc-client as dependencies
That makes sense. The README and code documentation should be updated accordingly to prompt the developer for that.
So the usage would be (pseudocode):
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 login()
function (or the OidcAuth
constructor):
def login(self, idp, external_callback=False):
... # prepare
if external_callback:
return login_url, self.on_login_finished # Return the log-in URL, and a callback for storing the session after log-in finished
... # Start flask server and listen to callback
def on_login_finished(self, session):
... # store log-in finished state
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:
OidcAuth.py
so users can choose not install flask
and solid-oidc-client
and also let test cases pass.> auth = OidcAuth(callback_server=False)
> auth.login(idp)
Plz login via URL: .....
Paste callback url here: ...
> solid_api = SolidAPI(auth)
Further thoughts about supporting OIDC login:
solid-file-python
should allow users to create their own authentication class for their specific use cases, as suggested by @Otto-AA.
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.