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

PLIP: Replace portal_skins-based login with a browserview-based one #2092

Closed
esteele opened this Issue Jul 5, 2017 · 18 comments

Comments

5 participants
@esteele
Member

esteele commented Jul 5, 2017

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer:

Eric Steele

Seconder:

Philip Bauer

Abstract

Remove the remaining skin scripts dealing with login in CMFPlone with a more modern implementation that is pluggable, easier to override, test and extend.

Motivation

Skin scripts and templates are deprecated and need to die.

Assumptions

Everyone wants this.

Proposal & Implementation

In 2014 at the Emerald Sprint the package https://github.com/plone/plone.login was created. It is based on z3c.form and is used in various projects but was never finished. Since then some aspects of it have been reimplemented (e.g. password-reset and registration-forms) in various places but the main thing (the login) is still based on skin scripts.

The new forms and views could either live in plone.login or moved to CMFPlone. For now we have them in plone.login so it can be used as a addon.

Adding oauth-features like a integration with https://github.com/collective/pas.plugins.authomatic is out of the scope of this PLIP.

Deliverables

Risks

Might break customized login-processes that rely on the old scripts.

Participants

@pbauer pbauer changed the title from (WIP) PLIP: Replace portal_skins-based login with a browserview-based one to PLIP: Replace portal_skins-based login with a browserview-based one Jul 10, 2017

@ebrehault

This comment has been minimized.

Show comment
Hide comment
@ebrehault

ebrehault Sep 5, 2017

Member

This PLIP has been approved

Member

ebrehault commented Sep 5, 2017

This PLIP has been approved

@pbauer pbauer moved this from New (drafts) to In Process (approved) in PLIPs Sep 19, 2017

@datakurre

This comment has been minimized.

Show comment
Hide comment
@datakurre

datakurre Oct 4, 2017

Member

@esteele @pbauer Would you be able to summarize what remains still missing after your Midsummer Sprint effort in July, and how your work could be tested already? https://github.com/plone/plone.login/commits/master

Is it possible already to use plone.login as an add-on or would it require merging into CMFPlone before testing/using and polishing?

Member

datakurre commented Oct 4, 2017

@esteele @pbauer Would you be able to summarize what remains still missing after your Midsummer Sprint effort in July, and how your work could be tested already? https://github.com/plone/plone.login/commits/master

Is it possible already to use plone.login as an add-on or would it require merging into CMFPlone before testing/using and polishing?

@pbauer pbauer referenced this issue Nov 10, 2017

Closed

Fix remaining failing test in 5.2 #2195

10 of 10 tasks complete
@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Nov 10, 2017

Member

@datakurre this basically works fine in 5.1. Only for self-registration to work you also need the branch plonelogin of plone.app.users. Give it a try!

The changes in CMFPlone are basically only removing the old templates and are not strictly needed to use is as an addon.

To make the PLIP mergeable we need to fix quite a number of tests (see http://jenkins.plone.org/view/PLIPs/job/plip-2092-login/). I will work on that when time allows.

Member

pbauer commented Nov 10, 2017

@datakurre this basically works fine in 5.1. Only for self-registration to work you also need the branch plonelogin of plone.app.users. Give it a try!

The changes in CMFPlone are basically only removing the old templates and are not strictly needed to use is as an addon.

To make the PLIP mergeable we need to fix quite a number of tests (see http://jenkins.plone.org/view/PLIPs/job/plip-2092-login/). I will work on that when time allows.

@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Nov 13, 2017

Member

I fixed a couple of bugs that I found when testing it.
In collective/demo.plone.de#22 I use it in demo.plone.de.

Member

pbauer commented Nov 13, 2017

I fixed a couple of bugs that I found when testing it.
In collective/demo.plone.de#22 I use it in demo.plone.de.

@jensens jensens added this to the Plone 5.2 milestone Dec 4, 2017

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Dec 4, 2017

Member

Adding oauth-features like a integration with https://github.com/collective/pas.plugins.authomatic is out of the scope of this PLIP.

The old login supports External Logins.
I expect the new login to also support this feature. It is broadly used in larger companies.

Member

jensens commented Dec 4, 2017

Adding oauth-features like a integration with https://github.com/collective/pas.plugins.authomatic is out of the scope of this PLIP.

The old login supports External Logins.
I expect the new login to also support this feature. It is broadly used in larger companies.

@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Dec 4, 2017

Member

@jensens True. Support for the existing settings external_login_url, external_logout_url and external_login_iframe is still missing.
@esteele Can you remember if we talked about that in Finnland or did we simply miss it?

Member

pbauer commented Dec 4, 2017

@jensens True. Support for the existing settings external_login_url, external_logout_url and external_login_iframe is still missing.
@esteele Can you remember if we talked about that in Finnland or did we simply miss it?

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Dec 4, 2017

Member

external_login_iframe scares me, we may want to deprecate that ;)

Member

jensens commented Dec 4, 2017

external_login_iframe scares me, we may want to deprecate that ;)

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Dec 4, 2017

Member

Btw.: The current way providing these urls is not good. But its used and plone.login should provide feature parity.

Having a way to plugin a subform or subtemplate or whatever its is called in the shiny z3cform world would be probably better. Also we then would have better UX and the ability to support multiple login methods (like username-password and OAuth2 with google, twitter and facebook). But this can be done in a next enhancement step and its ok to have it outside this plip.

Member

jensens commented Dec 4, 2017

Btw.: The current way providing these urls is not good. But its used and plone.login should provide feature parity.

Having a way to plugin a subform or subtemplate or whatever its is called in the shiny z3cform world would be probably better. Also we then would have better UX and the ability to support multiple login methods (like username-password and OAuth2 with google, twitter and facebook). But this can be done in a next enhancement step and its ok to have it outside this plip.

@jensens jensens moved this from In Process (approved) to Under Review (implemented) in PLIPs Jan 9, 2018

@ale-rt ale-rt moved this from Under Review (implemented) to Ready in PLIPs May 8, 2018

@ale-rt ale-rt moved this from Ready to Under Review (implemented) in PLIPs May 8, 2018

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens May 14, 2018

Member

I implemented external-login in the old way. external iframe support is untested, but should work. pas.plugins.authomatic integration works, so should others.
More improvements needed, but could be done later.

Also template customization using z3c.jbot is now supported.

@agitator and I will do some field-testing of all this in customer projects.

Once found ready, it can be released as addon for Plone 5.1.

And, all code will be moved over to Products.CMFPlone in order to not introduce yet another package with some browser views. It can live side-by-side with password reset tool there, good place.

Member

jensens commented May 14, 2018

I implemented external-login in the old way. external iframe support is untested, but should work. pas.plugins.authomatic integration works, so should others.
More improvements needed, but could be done later.

Also template customization using z3c.jbot is now supported.

@agitator and I will do some field-testing of all this in customer projects.

Once found ready, it can be released as addon for Plone 5.1.

And, all code will be moved over to Products.CMFPlone in order to not introduce yet another package with some browser views. It can live side-by-side with password reset tool there, good place.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens May 14, 2018

Member

Ah, and i18n/l10n is missing. But there is a PR already (wip) plone/plone.login#46

Member

jensens commented May 14, 2018

Ah, and i18n/l10n is missing. But there is a PR already (wip) plone/plone.login#46

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 12, 2018

Member

state: plone.login is ready now. I am mergin the code now into Products.CMFPlone (target 5.2 master).

Member

jensens commented Jun 12, 2018

state: plone.login is ready now. I am mergin the code now into Products.CMFPlone (target 5.2 master).

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 12, 2018

Member

to merge before standalone.

to merge together

for jenkins PR job

https://github.com/plone/Products.CMFPlone/pull/2445
https://github.com/plone/plone.app.upgrade/pull/165
Member

jensens commented Jun 12, 2018

to merge before standalone.

to merge together

for jenkins PR job

https://github.com/plone/Products.CMFPlone/pull/2445
https://github.com/plone/plone.app.upgrade/pull/165
@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 12, 2018

Member

Note: the po-files (there are already german translations in plone.login) are not moved to plone.app.locales for now.

Member

jensens commented Jun 12, 2018

Note: the po-files (there are already german translations in plone.login) are not moved to plone.app.locales for now.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 18, 2018

Member

released https://pypi.org/project/plone.login/ for 5.1 (and probably 5.0 - untested) as rc1

Member

jensens commented Jun 18, 2018

released https://pypi.org/project/plone.login/ for 5.1 (and probably 5.0 - untested) as rc1

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 18, 2018

Member

Note: The external login of the merged plone.login does no longer provide the more or less hacky way to pass __ac cookie of wrong domain from some second Plone login site to the current site and use it as login credentials. At the Buschenschanksprint we decided its ok to remove this rarely used feature including its tests. If someone needs this, please create an addon supporting this. Better, SSO can be done easily with OAuth2/Shibboleth/SAML2 or others these days.

Member

jensens commented Jun 18, 2018

Note: The external login of the merged plone.login does no longer provide the more or less hacky way to pass __ac cookie of wrong domain from some second Plone login site to the current site and use it as login credentials. At the Buschenschanksprint we decided its ok to remove this rarely used feature including its tests. If someone needs this, please create an addon supporting this. Better, SSO can be done easily with OAuth2/Shibboleth/SAML2 or others these days.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 18, 2018

Member

Ops and sorry - for 2 commits on master (plone.app.users/plone.app.portlets) - I just forgot I merged 'em already and branches were gone. But looks good so far.

Member

jensens commented Jun 18, 2018

Ops and sorry - for 2 commits on master (plone.app.users/plone.app.portlets) - I just forgot I merged 'em already and branches were gone. But looks good so far.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Jun 19, 2018

Member

All GREEN. I merge it now.

Member

jensens commented Jun 19, 2018

All GREEN. I merge it now.

@jensens jensens moved this from Under Review (implemented) to Merged in PLIPs Jun 19, 2018

@jensens jensens closed this Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment