More extensible view for assertion consumer service

[sheilatron]

The assertion consumer service is now more extensible as a class-based view with hooks that can be overridden by subclass implementations.

[MasterRoshan]

I came across this PR, while trying to figure out how to implement some things. It took me a while to figure this out, but you can modify the RelayState/Redirect url by setting the next url parameter at login. For example: href="{% url 'saml2_login' %}?next={{request.path}}" would result in a redirect to page the user was on before the login.

This is just something that fit my use case that I thought I would share.

[sheilatron]

Oh! Thank you for that tip, @MasterRoshan!

[sheilatron]

I came across this PR, while trying to figure out how to implement some things. It took me a while to figure this out, but you can modify the RelayState/Redirect url by setting the next url parameter at login. For example: href="{% url 'saml2_login' %}?next={{request.path}}" would result in a redirect to page the user was on before the login.

This is just something that fit my use case that I thought I would share.

It looks like to make this suggestion work for my use case @MasterRoshan , I would have to define a 'next' query string in the ACS URL defined in the metadata. I prefer not to disclose that URL path in my public metadata, so having this as a subclass method prevents that unnecessary information disclosure. For my purposes, it's better for only authenticated users to know about that next URL.

[sheilatron]

I came across this PR, while trying to figure out how to implement some things. It took me a while to figure this out, but you can modify the RelayState/Redirect url by setting the next url parameter at login. For example: href="{% url 'saml2_login' %}?next={{request.path}}" would result in a redirect to page the user was on before the login.
This is just something that fit my use case that I thought I would share.

It looks like to make this suggestion work for my use case @MasterRoshan , I would have to define a 'next' query string in the ACS URL defined in the metadata. I prefer not to disclose that URL path in my public metadata, so having this as a subclass method prevents that unnecessary information disclosure. For my purposes, it's better for only authenticated users to know about that next URL.

Or, maybe I am mistaken. When I tested the 'next' query string param with my ACS URL, it had no effect. Setting the settings.ACS_DEFAULT_REDIRECT_URL also didn't do the job; I still need the subclass method 'custom_redirect' to provide the expected redirect after IdP-initiated SAML authentication.

[peppelinux]

Hi @sheilatron
I think that it's time to merge this PR, I think that this follows the code restyle that @mhindery is working on in: #183

Can you resolve the conflicts, that's would be merged in v0.18.2 if you agree, thank you

[mhindery]

This would indeed be nice, I was working on a similar move to class-based-views in a branch to make a PR after my current PR's went through, I only started on the loginview https://github.com/mhindery/djangosaml2/blob/views/djangosaml2/views.py#L84-L281

I would initially keep the function-based view entry in there, when then simply act as a proxy to the new class-based view. As an example see the last part of the selected text in my link above. This way, users can upgrade to the new package while keeping their current implementation functional (which calls these function-based views with custom arguments). They can then change to a class-based view customized implementation when possible.

[peppelinux]

This would indeed be nice, I was working on a similar move to class-based-views in a branch to make a PR after my current PR's went through, I only started on the loginview https://github.com/mhindery/djangosaml2/blob/views/djangosaml2/views.py#L84-L281

I would initially keep the function-based view entry in there, when then simply act as a proxy to the new class-based view. As an example see the last part of the selected text in my link above. This way, users can upgrade to the new package while keeping their current implementation functional (which calls these function-based views with custom arguments). They can then change to a class-based view customized implementation when possible.

Please do not start the refactor of the code that could collide with this PR, we're waiting for @sheilatron reply, then I'd prefer to merge her PR and then move to your.

I know that you are a lover of Django class Views, as far I see in djangosaml2idp :)

[sheilatron]

Thanks for the followup on this @peppelinux ! I am glad to see the activity in this project, and would be happy to resolve these merge conflicts.

I noticed that the master branch includes removal of Python 2 compatibility code, such as this:

try:
    from django.utils.six import text_type, binary_type, PY3
except ImportError:
    import sys
    PY3 = sys.version_info[0] == 3
    text_type = str
    binary_type = bytes

Because that would break backward compatibility, perhaps the version number could be bumped a little more? Moving from 0.18.0 to 0.18.2 seems like a small version bump for that big of a change.

@mhindery wrote

I would initially keep the function-based view entry in there, when then simply act as a proxy to the new class-based view. As an example see the last part of the selected text in my link above. This way, users can upgrade to the new package while keeping their current implementation functional (which calls these function-based views with custom arguments). They can then change to a class-based view customized implementation when possible.

I could see the benefit, in case of implementations that wrap the old function-based ACS view. I wonder if that is a common practice.

[peppelinux]

Thanks for the followup on this @peppelinux ! I am glad to see the activity in this project, and would be happy to resolve these merge conflicts.

I noticed that the master branch includes removal of Python 2 compatibility code, such as this:

try:
    from django.utils.six import text_type, binary_type, PY3
except ImportError:
    import sys
    PY3 = sys.version_info[0] == 3
    text_type = str
    binary_type = bytes

Because that would break backward compatibility, perhaps the version number could be bumped a little more? Moving from 0.18.0 to 0.18.2 seems like a small version bump for that big of a change.

Ok Sheila, I can thin kthat @knaperek could also agree, so 0.18.2 will be 0.19.0 e the following 0.19.1.

Before start I'd like to have also @OskarPersson indications and also advance my excuses if this morning I woke up with these releases ... :)

[peppelinux]

@sheilatron at last but not least, please consider in your last commits the things we already merged from other contributions in ACS view. I know that you're a good analyst but take a look and your time to carry all the things and do not miss nothing of already done as good

[sheilatron]

@sheilatron at last but not least, please consider in your last commits the things we already merged from other contributions in ACS view. I know that you're a good analyst but take a look and your time to carry all the things and do not miss nothing of already done as good

I will review again, because I have not had time to fully test these changes today. I can see there are errors in the CI job, so I know some corrections are needed.

Today is very busy but I will try and find a little more time to work on this when I can.

[mhindery]

I've added some remarks. Hope to get this merged!

[mhindery]

You can add the csr_exempt in a more coincise way like this:

from django.utils.decorators import method_decorator

@method_decorator(csrf_exempt, name='dispatch')
class AssertionConsumerServiceView(View):
    ...

That way you don't need override the dispatch solely to add the csrf decorator. Only the dispatch method is enough, there's no need to set this on the post method additionally.

[mhindery]

Can you use fail_acs_response here as well?

[mhindery]

Could the build_relay_state, custom_redirect and the is_safe_url be a single method? Which takes the (request, user, session_info) and possibly whatever else is useful (e.g. whether or not to perform the is_safe_check, this might want to be customized entirely), an returns a 'definitive' url to redirect to. And then the default implementation does what is currently done. That way it's a bit clearer due there being a single method to determine the redirect, which users can override, instead of the several pieces which all do a part of it.

[mhindery]

What kind of customization do you see being applied here? What behaviour will this impact?

[mhindery]

Regarding the backwards compatibility: due to the current implementation with function-based views somewhat lacking in customizability, we have these kind of constructs currently in use:

from djangosaml2.views import assertion_consumer_service as djangosaml2_acs
from djangosaml2.views import login as djangosaml2_login

@method_decorator(csrf_exempt, name='dispatch')
class ACSWrapperView(View):
    """ Wrapper for SSO login endpoint. Show a useful error page.
    """
    def post(self, request, *args, **kwargs):
        try:
            response = djangosaml2_acs(request)
            if response.status_code == 403:  # SSO error = issue in their config
                return render(request, 'error_page.html', {'message': "Something went wrong with your login attempt. Please contact your property administrator or IT team."})
            if <some custom logic here>:
                ...
                raise PermissionDenied
            return response
        except PermissionDenied:
            return render(request, 'error_page.html', {'message': 'lalala'})


class SSOLoginView(View):
    def get(self, request, *args, **kwargs):
        return djangosaml2_login(request, post_binding_form_template='sso_post_form.html')

So if we keep the old functions in the package (which just translate into their CBV new version), this allows people to update to the new package, and everything will keep working. They can then migrate view per view to the new structure. This will be a lot smaller treshold than having to do everything at once, which risks people stop upgrading to avoid breaking everything, and that way they also miss out on other fixes in new versions. There could be a deprecation warning, something indicating that the next major version will not have these function-based proxies anymore to give an incentive to migrate.

[peppelinux]

Regarding the backwards compatibility: due to the current implementation with function-based views somewhat lacking in customizability, we have these kind of constructs currently in use:

It seem pretty good, I'd ask to @sheilatron to make a revision and to you to put this commits directly to the sheila's repository. This way sheila could benefits of your contributions and then everything would be merged in the mailstream. Your commits will be authored as well.

@knaperek I hope that you'll agree on this code restyle completely based on Class View, not so many developers loves it but it's not so bad and certainly more extendible and customizable over time. Yes... with a simple function the reading is more linear and in case of customization a developer can use his custom function declaring it in the urls, but Class View are good if you agree and as Monthy Python said "And now for something completely different"!

[mhindery]

This can be merged without anything from my side, I was working on another view, so I'll just add it at a later stage.

[sheilatron]

Sorry I was busy for the last two deals dealing with urgent work. I hope to get a little time for this over the weekend.

I will need to investigate why the unit tests are failing for some versions of Python, or whether these are random failures due to flaky tests. I can try and reproduce this locally. Any suggestions welcome!

[mhindery]

There was a problem with the tests in that they use hardcoded strings of xml responses to do equality assertions. And Python 3.8 has different ordering of xml attributes, which means the hardcoded strings don't compare as equal anymore. I've made an PR to fix that which got merged, in the meantime another PR was merged which added a test which now also fails on Python 3.8 for this reason. This PR fixes that.

So I'd suggest merging the master branch in your feature branch, and if it hasn't been merged at the time you're working on this, also the branch from the PR above. That has running and succeeding tests. I suppose also if the tests that are failing are not yours, it shouldn't be blocking merging your PR, so I wouldn't lose too much time on that :)

[peppelinux]

@sheilatron unit test beign fixed with this:
9a1d06d

please do not stress, we're here for fun and superior knowledge :) There's no hurry, really, take your time and do beautiful things, even here

[mhindery]

Hi @sheilatron did you have time to look into this, or do you plan to in the short term? I'm asking as I would also like to have these converted to class-based views (I started on the loginview already some time ago) and I'm holding of on creating a PR to not conflict with this one. But if you don't plan to work on this in the near future (which is no problem at all) I would go ahead and continue my refactoring efforts. I would leave the acs view alone and work on the others, but it will mean you'll have some merge conflicts when the master branch gets updated with my PR's as soon as you go back to this.

[sheilatron]

Hi @sheilatron did you have time to look into this, or do you plan to in the short term? I'm asking as I would also like to have these converted to class-based views (I started on the loginview already some time ago) and I'm holding of on creating a PR to not conflict with this one. But if you don't plan to work on this in the near future (which is no problem at all) I would go ahead and continue my refactoring efforts. I would leave the acs view alone and work on the others, but it will mean you'll have some merge conflicts when the master branch gets updated with my PR's as soon as you go back to this.

Looking at it now...

[peppelinux]

Hi Sheila, unit testss fails with
NameError: name 'is_safe_url' is not defined
mind to pull the latest master branch
good work

[peppelinux]

great, is it ready to be merged?
I would ask a rebase but is not mandatory :)

[sheilatron]

Thanks for the patience on this. I am glad this PR is getting some attention, but this has been a busy time for me.

I don't usually do much rebasing but can give it a quick try.

[sheilatron]

great, is it ready to be merged?
I would ask a rebase but is not mandatory :)

There were some conflicts when attempting a plain rebase, because the merge commits containing conflict resolution were missing. That was after fetching so the local repo had all commits from the master branch.

So I added the -p option and that seemed to work:

$ git rebase -p origin/master
warning: git rebase --preserve-merges is deprecated. Use --rebase-merges instead.
Successfully rebased and updated refs/heads/ACS_customizable.

When I pushed, not much action happened.

 $ git push -f
 Everything up-to-date

Maybe that's because all the merge commits were preserved. Sorry if this is not as much squashed as it could be. I may not have more time for this one today, but pls let me know if you have suggestions.

[peppelinux]

one of my favourite way to rebase the latter commits is

git log # see the commit uid where to want to back
git reset 31d3132ec18726090c5d0f4997f3f8d68e5b5f16 # this uid is an example
git diff  # here all your changes uncommitted
git commit -am 'here my new rebased commit'
git push -f

[sheilatron]

one of my favourite way to rebase the latter commits is

Thank you @peppelinux I will give that a try when I get a few minutes later today.

[sheilatron]

This has been rebased onto master and all tests are passing @peppelinux and @mhindery. I updated the CHANGES file to specify inclusion in the 0.19 release.

[peppelinux]

Thank you Sheila, @knaperek have confirmed that asap we'll have a brand new releases with all these contributions. We're a great team ;)