Skip to content
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

add new @login endpoint to return available external login options #1757

Merged
merged 33 commits into from
Jan 17, 2025

Conversation

erral
Copy link
Member

@erral erral commented Feb 28, 2024

This PR adds a generic GET /@login endpoint 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.

@erral erral requested a review from ericof February 28, 2024 19:01
@mister-roboto
Copy link

@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:

@jenkins-plone-org please run jobs

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!

Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for plone-restapi canceled.

Name Link
🔨 Latest commit f52a73d
🔍 Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/65f4b29ed1a9c2000846c1d8

@erral
Copy link
Member Author

erral commented Feb 28, 2024

@jenkins-plone-org please run jobs

@erral erral marked this pull request as ready for review February 28, 2024 19:20
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs tests and documentation, similar to other endpoints. https://6.docs.plone.org/plone.restapi/docs/source/endpoints/index.html

news/1757.feature Outdated Show resolved Hide resolved
news/1757.feature Outdated Show resolved Hide resolved
erral and others added 2 commits February 29, 2024 10:52
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@erral
Copy link
Member Author

erral commented Feb 29, 2024

This needs tests and documentation, similar to other endpoints. https://6.docs.plone.org/plone.restapi/docs/source/endpoints/index.html

Yes, I am preparing that.

news/1757.feature Outdated Show resolved Hide resolved
erral and others added 2 commits February 29, 2024 12:33
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@erral
Copy link
Member Author

erral commented Mar 1, 2024

@jenkins-plone-org please run jobs

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided it would be faster, easier, and less frustrating to make the changes directly instead of a proper review. I hope that is OK.

@erral
Copy link
Member Author

erral commented Mar 1, 2024

I decided it would be faster, easier, and less frustrating to make the changes directly instead of a proper review. I hope that is OK.

Thanks! ❤️

@erral
Copy link
Member Author

erral commented Mar 1, 2024

I am investigating why the output for the documentation is empty.

@erral
Copy link
Member Author

erral commented Mar 3, 2024

It's not an issue with the docs, because the docs only pull from this file:

https://github.com/plone/plone.restapi/blob/erral-login-options/src/plone/restapi/tests/http-examples/external_authentication_links.resp

Can you show what the content of that file should be?

I have already fixed this. There was a bug in my code 😈

@erral
Copy link
Member Author

erral commented Mar 3, 2024

@jenkins-plone-org please run jobs

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erral
Copy link
Member Author

erral commented Mar 12, 2024

@jenkins-plone-org please run jobs

@erral
Copy link
Member Author

erral commented Aug 20, 2024

@jenkins-plone-org please run jobs

@erral
Copy link
Member Author

erral commented Sep 16, 2024

@jenkins-plone-org please run jobs

@erral
Copy link
Member Author

erral commented Jan 10, 2025

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

@erral is there any reason not to merge this PR?

@davisagli
Copy link
Member

@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.

@erral
Copy link
Member Author

erral commented Jan 10, 2025

Right know this GET operation over the @login endpoint is implemented by pas.plugins.authomatic but only returns the plugins available there (see https://github.com/collective/pas.plugins.authomatic/blob/main/src/pas/plugins/authomatic/services/login.py).

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 @login endpoint implementation here, and prepare PRs for both pas.plugins.authomatic and pas.plugins.oidc to support the adapter lookup implemented here (see collective/pas.plugins.authomatic#73 and collective/pas.plugins.oidc#47).

So, the idea is:

  • plone.restapi to provide a way for other add-ons to regiser external login services, as a central point.
  • volto-authomatic is the consumers of such endpoint

@davisagli
Copy link
Member

@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 Show resolved Hide resolved
docs/source/endpoints/login.md Show resolved Hide resolved
docs/source/endpoints/login.md Outdated Show resolved Hide resolved
docs/source/endpoints/login.md Outdated Show resolved Hide resolved
@davisagli davisagli merged commit 74f3d72 into main Jan 17, 2025
21 of 25 checks passed
@davisagli davisagli deleted the erral-login-options branch January 17, 2025 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants