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.

Sign up for GitHub

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

Jump to bottom

Fetch security settings from the registry instead of portal properties #22

Merged
merged 5 commits into from
Dec 14, 2014
Merged

Fetch security settings from the registry instead of portal properties #22

merged 5 commits into from
Dec 14, 2014

Conversation

jcerjak
Copy link
Member

@jcerjak jcerjak commented Aug 21, 2014

All security settings are now looked up from the plone registry instead of portal properties. This work is related to plone/Products.CMFPlone#216 .

Additionally, some of the security related tests from plone.app.controlpanel [1] were converted to unit/integration tests and moved to this package, since they were basically testing the @@new-user form with generate_user_id and generate_login_name methods, based on the security settings in the registry. So I think the various cases are better tested here, while the more general browser tests for the security panel will be implemented as robot tests in CMFPlone, where the security panel will be moved.

This PR also contains a fix for plone/Products.CMFPlone#261 , discovered when adding additional tests.

[1] https://github.com/plone/plone.app.controlpanel/blob/master/plone/app/controlpanel/tests/security.txt

@jcerjak
Copy link
Member Author

jcerjak commented Aug 21, 2014

To make the existing doctests pass, I needed to do some changes, which I'm not sure about:

  1. Before the login name field on the login form changed from 'Login Name' to 'Email' if the use_email_as_login was enabled. Now it is always labeled 'Login Name'. Is this the intended behavior?

    -> see changes in duplicate_email.rst and email_login.rst: jcerjak@fd8a298#diff-6a734c193d260cbd33e5bd975e81c299L29

  2. Before when you registered, the message "Welcome! You have been registered"
    was displayed, now there is no message displayed.

    -> see changes in flexible_user_registration.rst: jcerjak@fd8a298#diff-8b730820f6f81c99db089a973a16c58aL139

  3. Before when you registered, there was an option to login immediatelly
    ('Click the button to log in immediately.'). Now there is no such option
    displayed.

    -> see changes in email_login.rst: jcerjak@fd8a298#diff-84da8edf0b0acee13e834f4274d01694L49

@jcerjak jcerjak mentioned this pull request Aug 25, 2014
Merged
@tisto
Copy link
Member

tisto commented Sep 1, 2014

@jcerjak Is this pull request needed in order to make the security panel stuff work?

@gforcada
Copy link
Member

gforcada commented Sep 1, 2014

@jcerjak Just a small note on this: I'm working on moving tests to plone.app.testing, so the less you have to change them the less trouble I will have merging it later on :)

@jcerjak
Copy link
Member Author

jcerjak commented Sep 1, 2014

@gforcada , noted, I won't touch them anymore :)

@tisto , merging this should fix some of the failing tests, but there are some broken tests that I haven't run when working on the security panel (e.g. Products/CMFPlone/tests/csrf.txt). I'll investigate.

@jcerjak
Merge branch 'master' into plip10359-z3cform
d6947e4 
Conflicts:
	plone/app/users/browser/register.py
@jcerjak
minor docstrings update in register.py
923c3bf 
Property "validate_email" was replaced by the
"enable_user_pwd_choice" setting in the registry. #216
@tisto tisto merged commit 923c3bf into plone:master Dec 14, 2014
@jcerjak jcerjak mentioned this pull request Dec 14, 2014
Closed
@jcerjak
Copy link
Member Author

jcerjak commented Dec 20, 2014

Regarding 1.) from #22 (comment) :

'Email' should be displayed instead of 'Login Name', if use_email_as_login is enabled. The problem is off course that the setting in login_form.cpt is read from site properties instead of the registry.

@tisto , is there a recommended way to fetch the settings from the registry inside a skin template? Or should I just wait for the login forms to be converted to z3cform as well? Not sure what's the status of plone.login though.

@lentinj
Copy link
Member

lentinj commented Dec 20, 2014

Waiting for plone.login would seem the sensible move, from what I've seen of it.

@jcerjak
Copy link
Member Author

jcerjak commented Dec 20, 2014

I've checked 2.) and 3.) from #22 (comment) , here we have two issues:

  • "Welcome! You have been registered" and "Click the button to log in immediately." are displayed on the registration success page (Products/CMFPlone/skins/plone_login/registered.pt), but that page is never shown when using the normal registration form in an overlay.
  • If you register by going manually to the @@register form, then the success page is displayed, but not correctly. We have a similar problem as with 1.): context.validate_email is used instead of enable_user_pwd_choice from the registry.

Since login upgrade is not listed as a blocker for Plone 5 (see plone/Products.CMFPlone#184), we should probably fix this instead of waiting for plone.login?

jcerjak added a commit that referenced this pull request Jan 26, 2015
@jcerjak
fix doctests, security settings are now properly read from the registry
5f96023 
This reverts some of the changes done in fd8a298

See also #22
@jcerjak jcerjak mentioned this pull request Jan 27, 2015
Merged
tisto added a commit to plone/buildout.coredev that referenced this pull request Mar 2, 2015
@tisto
[fc] Repository: plone.app.users
7455947 
Branch: refs/heads/master
Date: 2015-01-26T19:54:27+01:00
Author: Jure Cerjak (jcerjak) <[email protected]>
Commit: plone/plone.app.users@5f96023

fix doctests, security settings are now properly read from the registry

This reverts some of the changes done in fd8a29812598df320637a03588cf0c7599d2a9e7

See also plone/plone.app.users#22

Files changed:
M plone/app/users/tests/duplicate_email.rst
M plone/app/users/tests/email_login.rst
M plone/app/users/tests/flexible_user_registration.rst

diff --git a/plone/app/users/tests/duplicate_email.rst b/plone/app/users/tests/duplicate_email.rst
index da0e362..91b2ded 100644
--- a/plone/app/users/tests/duplicate_email.rst
+++ b/plone/app/users/tests/duplicate_email.rst
@@ -32,7 +32,7 @@ Login as user two:
     >>> browser.open('http://nohost/plone/')
     >>> browser.getLink('Log in').click()
 
-    >>> browser.getControl('Login Name').value = '[email protected]'
+    >>> browser.getControl('E-mail').value = '[email protected]'
     >>> browser.getControl('Password').value = 'secret'
     >>> browser.getControl('Log in').click()
     >>> 'Login failed' in browser.contents
diff --git a/plone/app/users/tests/email_login.rst b/plone/app/users/tests/email_login.rst
index 7766ad3..4b7c649 100644
--- a/plone/app/users/tests/email_login.rst
+++ b/plone/app/users/tests/email_login.rst
@@ -67,10 +67,9 @@ Fill out the form, using an odd email address that should not give problems.
     >>> 'Failed to create your account' in browser.contents
     False
 
-    We can now login.
-    >>> browser.getLink('Log in').click()
-    >>> browser.getControl('Login Name').value = '[email protected]'
-    >>> browser.getControl('Password').value = 'secret'
+    We can login immediately.
+    >>> 'Click the button to log in immediately.' in browser.contents
+    True
     >>> browser.getControl('Log in').click()
     >>> 'You are now logged in' in browser.contents
     True
@@ -81,9 +80,10 @@ Fill out the form, using an odd email address that should not give problems.
     True
     >>> browser.getLink(url='http://nohost/plone/logout').click()
 
-    We login as manager.
+    We login as manager. The login form now has a different label for
+    the login name.
     >>> browser.open('http://nohost/plone/login_form')
-    >>> browser.getControl('Login Name').value = SITE_OWNER_NAME
+    >>> browser.getControl('E-mail').value = SITE_OWNER_NAME
     >>> browser.getControl('Password').value = SITE_OWNER_PASSWORD
     >>> browser.getControl('Log in').click()
 
diff --git a/plone/app/users/tests/flexible_user_registration.rst b/plone/app/users/tests/flexible_user_registration.rst
index ae58d4a..affef03 100644
--- a/plone/app/users/tests/flexible_user_registration.rst
+++ b/plone/app/users/tests/flexible_user_registration.rst
@@ -147,6 +147,8 @@ Check render register form in 'Use Email As Login' mode.
     >>> browser.getControl('Password').value = 'testpassword'
     >>> browser.getControl('Confirm password').value = 'testpassword'
     >>> browser.getControl('Register').click()
+    >>> browser.contents
+    '...Welcome!...You have been registered...'
 
 Revert email mode.
 


Repository: plone.app.users
Branch: refs/heads/master
Date: 2015-02-27T07:46:59+01:00
Author: Timo Stollenwerk () <[email protected]>
Commit: plone/plone.app.users@0229de2

Merge branch 'master' into plip10359-security-controlpanel

Files changed:
M MANIFEST.in

diff --git a/MANIFEST.in b/MANIFEST.in
index 233d37d..6acb23e 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -1,4 +1,3 @@
-include *.txt
 include *.rst
 
 recursive-include docs *


Repository: plone.app.users
Branch: refs/heads/master
Date: 2015-03-02T18:10:17+01:00
Author: Timo Stollenwerk (tisto) <[email protected]>
Commit: plone/plone.app.users@6473850

Merge pull request #31 from plone/plip10359-security-controlpanel

Plip 10359 - Security Control Panel migration

Files changed:
M plone/app/users/tests/duplicate_email.rst
M plone/app/users/tests/email_login.rst
M plone/app/users/tests/flexible_user_registration.rst

diff --git a/plone/app/users/tests/duplicate_email.rst b/plone/app/users/tests/duplicate_email.rst
index da0e362..91b2ded 100644
--- a/plone/app/users/tests/duplicate_email.rst
+++ b/plone/app/users/tests/duplicate_email.rst
@@ -32,7 +32,7 @@ Login as user two:
     >>> browser.open('http://nohost/plone/')
     >>> browser.getLink('Log in').click()
 
-    >>> browser.getControl('Login Name').value = '[email protected]'
+    >>> browser.getControl('E-mail').value = '[email protected]'
     >>> browser.getControl('Password').value = 'secret'
     >>> browser.getControl('Log in').click()
     >>> 'Login failed' in browser.contents
diff --git a/plone/app/users/tests/email_login.rst b/plone/app/users/tests/email_login.rst
index 7766ad3..4b7c649 100644
--- a/plone/app/users/tests/email_login.rst
+++ b/plone/app/users/tests/email_login.rst
@@ -67,10 +67,9 @@ Fill out the form, using an odd email address that should not give problems.
     >>> 'Failed to create your account' in browser.contents
     False
 
-    We can now login.
-    >>> browser.getLink('Log in').click()
-    >>> browser.getControl('Login Name').value = '[email protected]'
-    >>> browser.getControl('Password').value = 'secret'
+    We can login immediately.
+    >>> 'Click the button to log in immediately.' in browser.contents
+    True
     >>> browser.getControl('Log in').click()
     >>> 'You are now logged in' in browser.contents
     True
@@ -81,9 +80,10 @@ Fill out the form, using an odd email address that should not give problems.
     True
     >>> browser.getLink(url='http://nohost/plone/logout').click()
 
-    We login as manager.
+    We login as manager. The login form now has a different label for
+    the login name.
     >>> browser.open('http://nohost/plone/login_form')
-    >>> browser.getControl('Login Name').value = SITE_OWNER_NAME
+    >>> browser.getControl('E-mail').value = SITE_OWNER_NAME
     >>> browser.getControl('Password').value = SITE_OWNER_PASSWORD
     >>> browser.getControl('Log in').click()
 
diff --git a/plone/app/users/tests/flexible_user_registration.rst b/plone/app/users/tests/flexible_user_registration.rst
index ae58d4a..affef03 100644
--- a/plone/app/users/tests/flexible_user_registration.rst
+++ b/plone/app/users/tests/flexible_user_registration.rst
@@ -147,6 +147,8 @@ Check render register form in 'Use Email As Login' mode.
     >>> browser.getControl('Password').value = 'testpassword'
     >>> browser.getControl('Confirm password').value = 'testpassword'
     >>> browser.getControl('Register').click()
+    >>> browser.contents
+    '...Welcome!...You have been registered...'
 
 Revert email mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tisto tisto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jcerjak @tisto @gforcada @lentinj