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 feature change "?edit" variable (and "?edit_off", "?build") to custom #3069

Merged
merged 21 commits into from May 15, 2014
Merged

Add feature change "?edit" variable (and "?edit_off", "?build") to custom #3069

merged 21 commits into from May 15, 2014

Conversation

itcrab
Copy link
Contributor

@itcrab itcrab commented Apr 16, 2014

I add little changes for may changing "?edit" variable (and "?edit_off", "?build") to custom variable (eq. "?admin_on") in project started with Django-CMS. I need it's for hidden login form on sites for "hackers" and make happy our customer (change variable to company name).

…stom variable (eq. "?admin_on") in project started with Django-CMS.
@itcrab
Copy link
Contributor Author

itcrab commented Apr 16, 2014

Please wait - I found bug.

…variables "?CMS_ADMIN_TOOLBAR__EDIT_ON", "?CMS_ADMIN_TOOLBAR__EDIT_OFF", "?CMS_ADMIN_TOOLBAR__BUILD".
@itcrab
Copy link
Contributor Author

itcrab commented Apr 16, 2014

I fix bug: in toolbar live/draft mode is not changing. And take many replacements static urls "?edit", "?edit_off" and "?build" to changeable variables.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 16, 2014

Now I need small time - I want small refactoring for good code. I write when finished it.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 16, 2014

I finished refactoring.

@johnraz
Copy link
Contributor

johnraz commented Apr 16, 2014

I'm not really sure to understand completely the need for this. It sounds like an edge case to me. Could you explain a little bit more how it could be used in various use cases ?
Thanks!

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

I always "hide" Django standart admin in urls.py - name company for get admin is good like customers. It's first reason.
Second reason is hackers hidden - I don't know how hackers may hack system (many ways), but if I hide login form and getting admin permissions - I get more secure system.

@johnraz
Copy link
Contributor

johnraz commented Apr 17, 2014

If your goal is only to change the /admin/ path, it can easily be done in your project's main
urls.py by replacing:
url(r'^admin/', include(admin.site.urls)),
with
url(r'^company-name/', include(admin.site.urls)),

Am I missing something else ?

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

My goal not is change slug for Django standart admin - I know how it's do (url(r'^company-name/', include(admin.site.urls)),).
In this pull request I need change ?edit to ?company_name for show toolbar and login form.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

In my local copy based on Python 2.7.6 my pull request work's fine. Now I may change ?edit to ?company_name in settings.py project.

@johnraz
Copy link
Contributor

johnraz commented Apr 17, 2014

Sorry but it still seems to me that it is an edge case and I don't see how changing the name of a get variable would strengthen security.

Also, nobody should be putting the ?edit param manually in the url at any time so I don't see how it can make the customer happy in any way.

As on the "local copy" topic, travis tests are failing, and since they are failing, it is broken.

Can someone else comment on this ? Am I alone thinking this is an edge case ?

@mkoistinen
Copy link
Contributor

@itcrab, I'm glad you're working on this. In the days before django CMS 3.0, we could use django-admin-honeypot to address the issues you're attempting to deal with here.

I would recommend that we retain, however, the existing log-in form at ?edit, but that this form does nothing, or better yet, just log failed attempts (even if the credentials are correct), in a fashion similar to what django-admin-honeypot does. Otherwise, the hacker will quickly realise that she needs to look for another get-parameter to use.

@@ -208,13 +208,13 @@ def test_get_page_for_apphook_on_preview_or_edit(self):
with self.login_user_context(superuser):
with force_language("en"):
path = reverse('sample-settings')
request = self.get_request(path + '?edit')
request = self.get_request(path + '?%s' % get_cms_setting('CMS_TOOLBAR_URL__EDIT_ON'))
Copy link
Member

Choose a reason for hiding this comment

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

Check if get_cms_setting has been imported in apphooks.py

@mkoistinen
Copy link
Contributor

There should probably also be some checks that these new settings aren't going to clash with other, legitimate GET params, like 'page'. There may be others that the CMS uses internally for cache-busting, etc.

@johnraz
Copy link
Contributor

johnraz commented Apr 17, 2014

I will try to summarize what we just said on irc.

First of all, it seems I was wrong and this is not really that of an edge case, so my apologies on pushing this in the wrong direction.

Second, if we need/want to strengthen the login form's security by adding some login spoofing / honey pot'ing whatever the name we give it, it should probably be a "all-in-one" feature that should be easily enabled and fully documented.

The description made by @mkoistinen should be enough to get the feature implemented.

@digi604
Copy link
Contributor

digi604 commented Apr 17, 2014

ok tests failing and we would need better docs documenting the new settings

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

@mkoistinen I think that's a good idea to log all failed login attempts. But I also think that this will need constant monitoring logs periodicity or all time. My customers (and not only my) may not have time needed for this task. My pull request partially solves this problem.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

@mkoistinen Certainly use the company name for give access to toolbar I gave as an example and for hackers I prepared a surprise (give another name or company name plus salt).

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

My global idea - hidden all administrating zones on site for give more secure.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 17, 2014

@mkoistinen There should probably also be some checks that these new settings aren't going to clash with other, legitimate GET params, like 'page'. There may be others that the CMS uses internally for cache-busting, etc.

@mkoistinen How I may fast get all reserved words for this functionality?

@itcrab
Copy link
Contributor Author

itcrab commented Apr 18, 2014

I added notice to docs.

@itcrab
Copy link
Contributor Author

itcrab commented Apr 29, 2014

I finished works for this PR. My global goal is achieved (positive customer url for admin and hidden administration interface for hackers). Logging event for login form in toolbar somebody can make in another PR.

@yakky
Copy link
Member

yakky commented May 1, 2014

LGTM
Ping @FinalAngel

@yakky yakky added the frontend label May 1, 2014
@kezabelle
Copy link
Contributor

To address an earlier point made:

No clue. I know that 'page' is used by paginator. BTW, I don't see any benefit at all of changing 'edit_off' or even 'build' ever.

edit_off is harder to argue, but having a GET form which makes use of the parameters edit and build isn't by any means hard to imagine, the latter especially if the form is for filtering a list of products which might have a different build.

In an ideal world, django-CMS would ship with underscore (_edit) or specifically (cms-) prefixed parameters, for the same reason it ships it's own JavaScript namespace - namely, so that the chance of collision with outside requirements is minimised.

@mkoistinen
Copy link
Contributor

@kezabelle All good points! I vote that once we get this "magic strings" PR committed, we set the defaults to 'cms-edit', 'cms-edit-off' and 'cms-build'.

@itcrab
Copy link
Contributor Author

itcrab commented May 5, 2014

@mkoistinen I may set the defaults for:
?edit to ?cms_edit
?edit_off to ?cms_edit_off
?build to ?cms_build

Need?

@itcrab
Copy link
Contributor Author

itcrab commented May 5, 2014

@mkoistinen And developer may redefine this in him settings.py if this needed.

@digi604
Copy link
Contributor

digi604 commented May 14, 2014

could you add a note to the changelog? Besides this everything looks good for me. Changing the defaults: not at this moment... maybe for 3.1

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 Yes, wait a moment.

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 I rebase my fork branch develop (in PyCharm "Rebase my GitHub fork") and in it not have lastest changelogs:

==== 3.0.1 (2014-04-30) ===

- Renamed NamespaceAllreadyRegistered to NamespaceAlreadyRegistered in menus/exceptions.py
- Frontend editor UI fixes
- Fix in cms fix-mptt command

==== 3.0.2 ===
- Add 'as' form to render_placeholder templatetag to save the result in context

In my changelog have only 3.0.1 version changes (https://github.com/itcrab/django-cms/blob/develop/CHANGELOG.txt).

I wanted merge develop to my feature branch and do commit to changelog.
What I do not right (maybe you know)?

P.S. Sorry - it's my first work when forked repo is out of date in my fork.

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 I see in my repo master branch - in in only 3.0.1 changelogs.

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 Waiting I found howto do this.

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 I finished works.

@coveralls
Copy link

coveralls commented May 14, 2014

Coverage Status

Coverage remained the same at 94.444% when pulling 57bef42 on itcrab:feature/change-edit-variable-for-login-to-admin into 77304e2 on divio:develop.

@itcrab
Copy link
Contributor Author

itcrab commented May 14, 2014

@digi604 All tests passed!

digi604 added a commit that referenced this pull request May 15, 2014
…-login-to-admin

Add feature change "?edit" variable (and "?edit_off", "?build") to custom
@digi604 digi604 merged commit d897cf0 into django-cms:develop May 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants