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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

erral
Copy link
Sponsor 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
Sponsor 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
Sponsor 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
Sponsor 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
Sponsor 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
Sponsor Member Author

erral commented Mar 1, 2024

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

@stevepiercy
Copy link
Contributor

@erral what output? The preview looks good: https://deploy-preview-1757--plone-restapi.netlify.app/endpoints/login.html

@erral
Copy link
Sponsor Member Author

erral commented Mar 1, 2024

@stevepiercy the last output, this one:

HTTP/1.1 200 OK
Content-Type: application/json

{
    "options": []
}

That should not be empty :/

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.

Question for @erral

response = self.api_session.get(
f"{self.portal.absolute_url()}/@login",
)
save_request_and_response_for_docs("external_authentication_links", response)
Copy link
Contributor

Choose a reason for hiding this comment

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

With my rename of the docs file, should this be changed as well?

Suggested change
save_request_and_response_for_docs("external_authentication_links", response)
save_request_and_response_for_docs("login", response)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

No, this is for the test-generated request and response file. I would leave this as it is, because we already have login.req file in the http-examples folder

@stevepiercy
Copy link
Contributor

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?

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

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.

docs/source/endpoints/login.md Outdated Show resolved Hide resolved
docs/source/endpoints/login.md Outdated Show resolved Hide resolved
def get_providers():
"""
return a list of login providers, with its id, title, plugin and url
"""
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe this should be defined in plone.base instead of plone.restapi, so that an addon doesn't need to depend on plone.restapi just to register this adapter and provide information about what providers it supports?

On the other hand, that would make it harder to backport support for this feature to older versions of Plone.

I don't feel strongly about it but in general, plone.restapi's purpose is to provide API access to features that exist at a lower level and might also be accessed via other interfaces.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

the whole idea of the endpoint is to avoid duplicated @login endpoints such the ones we have in pas.plugins.authomatic and pas.plugins.oidc to support volto-authomatic

I agree that perhaps this should go in plone.base somewhere and integrate it with Plone's standard login page.

Perhaps we can open an issue in Plone and work on that later on?

@erral
Copy link
Sponsor 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
Sponsor 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
Sponsor Member Author

erral commented Mar 12, 2024

@jenkins-plone-org please run jobs

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.

None yet

4 participants