add new @login endpoint to return available external login options #1757
Conversation
|
@erral thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
✅ Deploy Preview for plone-restapi canceled.
|
|
@jenkins-plone-org please run jobs |
There was a problem hiding this comment.
This needs tests and documentation, similar to other endpoints. https://6.docs.plone.org/plone.restapi/docs/source/endpoints/index.html
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Yes, I am preparing that. |
Co-authored-by: Steve Piercy <[email protected]>
|
@jenkins-plone-org please run jobs |
Thanks! ❤️ |
|
I am investigating why the output for the documentation is empty. |
|
It's not an issue with the docs, because the docs only pull from this file: Can you show what the content of that file should be? |
There was a problem hiding this comment.
I like this idea in general.
Things I'm wondering:
- What about a site that wants to let the user choose between login from external providers and Plone's standard login?
- What if the integrator wants more control over the order of different login options?
So I'm thinking that maybe there should be a registry setting with a list of login providers to show, and this endpoint could use the info from the registry setting plus the details from the IExternalLoginProviders adapters. But -- this can probably be added later, so I'm not necessarily asking you to do it as a condition for adding this endpoint.
I have already fixed this. There was a bug in my code 😈 |
|
@jenkins-plone-org please run jobs |
There was a problem hiding this comment.
Preview of docs with update look good: https://deploy-preview-1757--plone-restapi.netlify.app/endpoints/login.html
|
@jenkins-plone-org please run jobs |
|
@jenkins-plone-org please run jobs |
|
@jenkins-plone-org please run jobs |
|
@jenkins-plone-org please run jobs |
|
@erral is there any reason not to merge this PR? |
|
@erral @stevepiercy Is there any prototype of using this API from a frontend? That would provide some confidence that we've implemented the right API. It's easier to make changes before we release something. I'm not sure if you plan to update Volto to use this, or just use it in specific projects. If Volto will use it, then we also need to coordinate with @robgietema to support it in Nick. |
|
Right know this The endpoint is consumed by volto-authomatic to list the login options. But this implementation works only with pas.plugins.authomatic. I had the requirement to use pas.plugins.authomatic and pas.plugins.oidc in the same site. Moreover I had the requirement to use 2 different oidc plugins. So first I generalized pas.plugins.oidc to support multiple oidc based plugins. Then I needed volto-authomatic to support both plugins, so I brought the So, the idea is:
|
|
@erral Thanks for the explanation and pointers. It makes sense to me. I could imagine that in the future, maybe Volto itself could check this endpoint and show the login providers on the default login page. But it's good enough to be able to use it in add-ons for now. I don't see a problem with Nick support. Nick doesn't support external auth providers yet, so it should be very easy to implement the endpoint which returns an empty list :) I'll add a review with a couple minor comments. |
src/plone/restapi/interfaces.py
Outdated
| @@ -240,3 +240,14 @@ class IBlockVisitor(Interface): | |||
|
|
|||
| def __call__(self, block): | |||
| """Return an iterable of sub-blocks found inside `block`.""" | |||
|
|
|||
|
|
|||
| class IExternalLoginProviders(Interface): | |||
There was a problem hiding this comment.
@erral Maybe we should just call this ILoginProviders? An add-on could also add a custom login form that is implemented within Plone rather than an external service, and want to include it in the list
|
|
||
| To achieve that, third party products need to register one or more adapters for the Plone site root object, providing the `plone.restapi.interfaces.IExternalLoginProviders` interface. | ||
|
|
||
| In the adapter, the add-on needs to return the list of external links and some metadata, including the `id`, `title`, and name of the `plugin`. |
There was a problem hiding this comment.
What's the difference between id and plugin? Why do we need both?
There was a problem hiding this comment.
This is on purpose to support the case of a plugin added twice. Think of 2 oidc plugins in a site. They would be different in id but same in plugin, like a way to mark the "plugin type" in plugin.
There was a problem hiding this comment.
That's what I do, for instance, in my PR on pas.plugins.oidc: https://github.com/collective/pas.plugins.oidc/blob/use-adapter/src/pas/plugins/oidc/services/login/get.py#L23-L26
This PR adds a generic
GET /@loginendpoint that would expose links to additional login providers in a Plone site.This way the endpoints added in packages like pas.plugins.authomatic and pas.plugins.oidc could be exchanged with adapter registrations and allow both products to be installed in the same Plone site for its use.
I am open to discuss whether the adapter needs to adapt just the IPloneSiteRoot object or perhaps may be a multiadapter including the request (perhaps to register the adapter for a given IThemeSpecific interface) or any other things.