Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bugfix render_login_html_to_string (refs: #53) #54

Conversation

mjholtkamp
Copy link
Contributor

Hello and thanks for djangosaml2idp!

While reviewing this software for use in our company, I found some bugs and I decided to make a couple of PRs for this. I'll try to keep the PRs small and simple, so they are easy to review :)

There was also an issue reported for this already (#53), I believe it can be closed by merging this PR.

If there are comments, please let me know and I'll see what I can do :)

""" Render the html response for the login action. Can be using a custom html template if set on the view. """
default_login_template_name = 'djangosaml2idp/#.html'
custom_login_template_name = getattr(self, 'login_html_template')
custom_login_template_name = getattr(self, 'login_html_template', None)
if custom_login_template_name:
Copy link
Contributor Author

@mjholtkamp mjholtkamp Feb 3, 2020

Choose a reason for hiding this comment

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

The only other call to render_login_html_to_string is from create_html_response and it only passes two kwargs (context and request), no args. So I think this is a leftover of a refactor.

In the code template_name is not being used, so I just removed it and handled the case where there is no login_html_template set.

@mhindery
Copy link
Contributor

mhindery commented Feb 3, 2020

Thanks for the contributions @mjholtkamp! I'll get through them as soon as possible. There are indeed some issues that crept in during refactoring in the latest release, as you noted in one of the MR's. We ourselves are running v0.5.0 in production and have been for several months, should you want to to try a more tested version.

@mhindery mhindery merged commit 6f754be into OTA-Insight:master Feb 3, 2020
@mjholtkamp mjholtkamp deleted the feature/bugfix-render_login_html_to_string branch February 4, 2020 08:26
@mjholtkamp
Copy link
Contributor Author

Thanks for the quick reply @mhindery and thanks for the tip of 0.5.0. In the documentation it is being said that this project is production-ready, but I was a bit surprised to find these bugs (no offence meant, I mean that it's likely they would be found if used in production).

That confused me a little bit. I don't know if you have a 'stable' channel or something, but maybe mention somehow in the documentation that the 0.5.0 is considered stable and the 0.6.x is being worked on (or whatever the status is)? How do you feel about that?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants