iplweb / django-password-policies-iplweb

Django unicode-aware password policies.
Other
7 stars 21 forks source link

added admin:logout to PasswordChangeMiddleware and enabled userlinks in django-password-policies views #15

Closed madEng84 closed 3 years ago

madEng84 commented 3 years ago

Screenshot 2021-07-09 at 20 18 29

mpasternak commented 3 years ago

Hi,

sorry if I'm being ignorant, but I don't exactly understand the goal of this update.

Is it to add logout/password change link to admin views?

If yes, then I'm thinking I'd be a little against it. The reason is, that you can modify all you need overriding admin templates. There are some admin replacements, like django-grappelli which would need extra testing. And so on.

Could you please explain to me, what exactly does this change do?

madEng84 commented 3 years ago

in django admin you can see those links only if the variable has_permission is True.

        {% block usertools %}
        {% if has_permission %}
        <div id="user-tools">
            {% block welcome-msg %}
                {% translate 'Welcome,' %}
                <strong>{% firstof user.get_short_name user.get_username %}</strong>.
            {% endblock %}
            {% block userlinks %}
                {% if site_url %}
                    <a href="{{ site_url }}">{% translate 'View site' %}</a> /
                {% endif %}
                {% if user.is_active and user.is_staff %}
                    {% url 'django-admindocs-docroot' as docsroot %}
                    {% if docsroot %}
                        <a href="{{ docsroot }}">{% translate 'Documentation' %}</a> /
                    {% endif %}
                {% endif %}
                {% if user.has_usable_password %}
                <a href="{% url 'admin:password_change' %}">{% translate 'Change password' %}</a> /
                {% endif %}
                <a href="{% url 'admin:logout' %}">{% translate 'Log out' %}</a>
            {% endblock %}
        </div>
        {% endif %}
        {% endblock %}

Here you can see the definition of has_permission

def has_permission(self, request):
    """
    Return True if the given HttpRequest has permission to view
    *at least one* page in the admin site.
    """
    return request.user.is_active and request.user.is_staff

that variable is passed in every django admin templates thanks to AdminSite.each_context method So I have modified the context of every view to pass the has_permission variable, elsewhere the {% block userlinks %} is couldn't be available without overriding django-admin template

I think it is appropriate to at least support the official django admin

grappelli is however supported because this modification only adds a variable to the template

mpasternak commented 3 years ago

So - please correct me if I'm wrong - what you want to implement is to give the ability for the "standard" django-password-policies views to be rendered with admin template and have the links visible. Did I understand this change properly?

madEng84 commented 3 years ago

Yes @mpasternak I confirm

madEng84 commented 3 years ago

I'm closing this PR There is a better way to inject admin context in django-password-policies views

Thank you

mpasternak commented 3 years ago

From what I've seen, the best philosophy of 3rd party packages - like this one - is to be really, really vanilla (unless you're specifically writing a package replacing admin or a theme package or something like that). And just as you wrote, it's best to override contexts from one's own application than to give multiple config options in such packages. Thanks for understanding!