django-saml2-auth icon indicating copy to clipboard operation
django-saml2-auth copied to clipboard

Reorganize code and use safer default configuration settings

Open kronion opened this issue 7 years ago • 5 comments

There are three primary changes in this PR:

  1. _get_metadata() now raises an exception if neither METADATA_AUTO_CONF_PATH or METADATA_LOCAL_FILE_PATH are set.
  2. Don't include function views in the lists passed to get_reverse(). This gives users the opportunity to override the views in their own code.
  3. Change a number of defaults for the configuration options. In particular:
    • DEFAULT_NEXT_URL is required, rather than defaulting to admin:index.
    • CREATE_USER is False.
    • STAFF_STATUS in NEW_USER_PROFILE is False.

The rationale behind 3 is that the current defaults are too permissive and probably don't make sense for most applications. If you're allowing users to log into your application using a third party identity provider, and a new user is created with the third party, it seems unlikely that most applications would want to immediately give that new user staff access and direct them to the admin panel. Staff access and admin panel redirection make sense if you're using an IdP to manage users in your own company/organization, but that probably isn't the norm. So I think it's acceptable to make the developer change a few additional defaults in order to auto-create users with unusual permissions.

kronion avatar Oct 19 '18 23:10 kronion

I strongly suggest moving the methods back to their original locations. Relocating methods like this makes the PR incompatible with all others.

ambsw-technology avatar Oct 29 '18 22:10 ambsw-technology

I'd be open to splitting the method reorganization into a separate PR, but I still think it should occur. At the moment, there are only six other open PRs, and only a few of those look like they'd conflict. The merge should be straightforward.

kronion avatar Oct 31 '18 18:10 kronion

I don't have strong feelings for or against reorganizing, but you can't combine relocated code with other changes in a community repo like this.

  1. Combining different issues in one PR is always bad practice. As you can see, I created both #67 and #68 so they can be accepted/discussed/rejected separately. I'm a big fan of safe defaults, but can't +1 this change because it's mixed in with others.
  2. Several (all?) of your changes are backwards incompatible. A user without a DEFAULT_NEXT_URL or explicit values for CREATE_USER and STAFF_STATUS will see their behavior change when this PR is applied. Normally, backwards incompatible changes aren't incorporated until the next major release so a lot of PRs could be accepted in the meantime.
  3. Your change to DEFAULT_NEXT_URL conflicts with my fix in #68. Maintainers can accept my #67 while they decide between our approaches. They can't accept any of your code while making this decision. If they go my direction (likely for backwards compatibility reasons), you're going to have to separate the PRs anyway. They might even apply my change now and use your change as part of a major release (though I doubt they will since their goal is to support a one-line configuration).
  4. Mixing relocated code with changes makes your PR very hard to review. It's not clear if a function is just relocated or relocated and changed. This will slow review/acceptance.
  5. Due to (2), (3), and (4) it's likely that other PRs will be accepted before yours. Someone will have to rebase your PR (i.e. you). You create extra work for yourself if your PR is not compatible with theirs.

I hope this doesn't come off as condescending -- I don't mean it that way. Packages like this need contributors like you so I want to help you make your PRs merge-friendly. Unfortunately, a lot of people get a PR rejected the reasons I describe and get frustrated because they don't understand why.

ambsw-technology avatar Nov 02 '18 14:11 ambsw-technology

Again, I'm happy to move the reorganizing bits of this PR into a separate PR. I'm in favor of the reorganization because I'd prefer views.py to be pithier and more focused in terms of what it contains.

  1. This is a good point. I'll split these changes up.
  2. Yes, these changes are definitely backwards incompatible, and I fully expect that they'll have to be incorporated into a major release. I think it's defensible to have breaking changes around safer defaults.
  3. I like your change in #68. However, something should probably change if the default for STAFF_STATUS becomesFalse. It wouldn't make a lot of sense to default to bringing a newly created non-staff user to the admin panel. Is one-line configuration actually a stated goal of the project? To be clear, I'm certainly in favor of a simple configuration process, and I acknowledge I'm making it more complicated. Maybe it would make more sense to keep DEFAULT_NEXT_URL optional but change its default value to something else?
  4. 👍
  5. Understood. That's always going to be an issue for any PR that reorganizes code. I expect I'll be able to handle a rebase if necessary, but it's likely to be an easy rebase for anyone to manage, especially after splitting those changes into a separate PR.

No worries, I appreciate the feedback. I want to make the PR better to maximize the chance of acceptance, and of course to benefit the community. I had to fork this project in order to make it useable for a production application, which isn't good for anybody.

kronion avatar Nov 02 '18 16:11 kronion

Yeah. I had to fix the reverse code because I'm not using admin so I'm with you that it's not a great default (today, let alone with your proposed defaults). I would eliminate the reverse entirely, always use DEFAULT_NEXT_URL, and use a default like /. It'll be rare that a site won't have a view at /.

My login redirect logic actually adds a next=. If I had an admin site and they were prompted to login by clicking that link, they'd end up there anyway (even if DEFAULT_NEXT_URL is /).

ambsw-technology avatar Nov 02 '18 16:11 ambsw-technology