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 initial Terms of Service support. #186

Merged
merged 2 commits into from
May 10, 2018
Merged

Add initial Terms of Service support. #186

merged 2 commits into from
May 10, 2018

Conversation

lukegb
Copy link
Collaborator

@lukegb lukegb commented Apr 29, 2018

  • New TermsOfService that represents a ToS, with a name, date, URL and
    an "active" flag.
  • New middleware like the existing one for enforcing active emails that
    requires users which haven't agreed to all "active" ToSes to agree.
  • Sync support for syncing ToSes to groups, for export to Discourse and
    Ore. At the moment, this only happens when a user logs in again.
  • Migration to create the initial SpongePowered 2018-03-10 ToS.
  • Updated registration form to add checkboxes to agree to the ToSes
    at registration time. These are all mandatory.

@coveralls
Copy link

coveralls commented Apr 29, 2018

Coverage Status

Coverage decreased (-2.7%) to 96.364% when pulling 81c8eab on feature/tos into 066080f on master.

- New TermsOfService that represents a ToS, with a name, date, URL and
  an "active" flag.
- New middleware like the existing one for enforcing active emails that
  requires users which haven't agreed to all "active" ToSes to agree.
- Sync support for syncing ToSes to groups, for export to Discourse and
  Ore. At the moment, this only happens when a user logs in again.
- Migration to create the initial SpongePowered 2018-03-10 ToS.
- Updated registration form to add checkboxes to agree to the ToSes
  at registration time. These are all mandatory.
{% block title %}{% trans "Terms of Service" %}{% endblock %}
{% block main %}
<div class="container top-level">
<div class="row">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This formatting is all over the place.

@lukegb lukegb requested a review from felixoi April 30, 2018 00:27
Copy link
Contributor

@phase phase left a comment

Choose a reason for hiding this comment

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

What is the current user flow for when the TOS is updated? Are we making users auto agree to the updated terms? We can send a notification to users when the TOS is updated with a link to the Docs.

@@ -43,6 +43,20 @@ class ForgotTokenGenerator(django.contrib.auth.tokens.PasswordResetTokenGenerato


def _log_user_in(request, user, skip_twofa=False):
# Resync groups with the TOS acceptances.
# XXX(lukegb): this is a hack, don't do this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this syncing the groups in memory with the groups in the DB? Do you want to put this into a separate function and have these sync periodically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Specifically this syncs the group membership in the database with the ToS acceptances. These are separate because going back and retroactively changing the group membership to use a through table is tricky, and acceptances store a timestamp value as well, whereas groups do not.

This should really be a trigger when acceptances change rather than on login, but I haven't bothered to implement that yet.

This probably has consequences for the following flow, though:

  1. User logs in, and hasn't accepted all the ToSes yet
  2. User clicks through the ToS acknowledgement
  3. User continues to Discourse/Ore

...because the user won't be added to the ToS group for the things they've just acked.

@lukegb
Copy link
Collaborator Author

lukegb commented Apr 30, 2018

The user flow for when ToSes are updated is currently that we will force them to click through the ToS again on login (or any action on SpongeAuth). This is slightly different to the wording in the ToS, which permits us to update the ToS, but I don't see it happening particularly often, so I'm not too worried.

@felixoi
Copy link
Member

felixoi commented May 9, 2018

  1. If we are doing this using groups how we are going to check on Ore for example if the latest tos is acceped? Hard to do if there are multiply roles for multiply ToS'

  2. I checked this out locally and I had one bug. When you login trough Ore, accept the TOS and click submit and you return to Ore afterwards you are not logged in. If you click Login again afterwards you are instantly logged in. Also the ACP shows me my user isn't in the group who accepted the tos.

  3. Adding a new TOS using the ACP ends up in an error:

null value in column "group_id" violates not-null constraint
DETAIL:  Failing row contains (3, test, 2018-05-09, http://test.com, t, null).
  1. I'm not sure about this.. but I think the groups for this have to be public since they aren't included in the sso payload otherwise.

@lukegb
Copy link
Collaborator Author

lukegb commented May 10, 2018

  1. Ore needs to know what groups it cares about. Or it can just expire people's session's periodically and then they'll be forced to accept the latest ToS anyway when they roundtrip through Auth.

  2. Ack, please file a bug. Will handle separately.

  3. I'll remove support for adding ToS through the ACP. They should be added using migrations in this repo for now.

  4. Yeah, they do. The ones autogenerated don't at the moment, but that is fixible in the ACP.

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

Successfully merging this pull request may close these issues.

None yet

4 participants