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

Accept terms of Service at signup #8193

Merged
merged 23 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a44f782
Adapt create org route in backend to accept terms of service version
frcroth Nov 13, 2024
b62721f
Merge branch 'master' into accept-tos-at-sign-up
frcroth Nov 13, 2024
94716a7
fix inconsistency
frcroth Nov 13, 2024
5607fcc
Fix formatting
frcroth Nov 13, 2024
05eed1c
start to implement frontend
knollengewaechs Nov 14, 2024
ae61dc6
delete old tos modal, add checkboxes to forms and add style
knollengewaechs Nov 14, 2024
5340755
improve styling and remove random new test file
knollengewaechs Nov 15, 2024
ce2396f
Merge branch 'master' into accept-tos-at-sign-up
knollengewaechs Nov 15, 2024
4ea0446
Supply user access context at sign up tos acceptance
frcroth Nov 18, 2024
77e2a59
send org name as id
knollengewaechs Nov 18, 2024
b1bb0b8
add tos modal again
knollengewaechs Nov 18, 2024
a4817e4
Merge branch 'master' into accept-tos-at-sign-up
knollengewaechs Nov 18, 2024
f52c528
Fix frontend lint
frcroth Nov 18, 2024
2602e5b
Add changelog
frcroth Nov 20, 2024
e5d5f19
Revert "send org name as id"
frcroth Nov 20, 2024
db456d0
Merge branch 'master' into accept-tos-at-sign-up
frcroth Nov 20, 2024
377d289
Require terms of service at sign up if enabled
frcroth Nov 20, 2024
8d618c4
invert logic for rendering tos checkbox
knollengewaechs Nov 21, 2024
6c819e3
Add explanation on tos failure because of non-acceptance
frcroth Nov 25, 2024
1d2ecb2
Revert changes to application.conf
frcroth Nov 25, 2024
94be416
Merge branch 'master' into accept-tos-at-sign-up
frcroth Nov 25, 2024
d60afaa
extract TOS checkbox to component
knollengewaechs Nov 25, 2024
ca427bc
Merge branch 'master' into accept-tos-at-sign-up
frcroth Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
### Changed
- Reading image files on datastore filesystem is now done asynchronously. [#8126](https://github.com/scalableminds/webknossos/pull/8126)
- Improved error messages for starting jobs on datasets from other organizations. [#8181](https://github.com/scalableminds/webknossos/pull/8181)
- Terms of Service for Webknossos are now accepted at registration, not afterward. [#8193](https://github.com/scalableminds/webknossos/pull/8193)
- Removed bounding box size restriction for inferral jobs for super users. [#8200](https://github.com/scalableminds/webknossos/pull/8200)

### Fixed
Expand Down
18 changes: 13 additions & 5 deletions app/controllers/AuthenticationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import org.apache.commons.codec.digest.{HmacAlgorithms, HmacUtils}
import play.api.data.Form
import play.api.data.Forms.{email, _}
import play.api.data.validation.Constraints._
import play.api.i18n.Messages
import play.api.i18n.{Messages, MessagesProvider}
import play.api.libs.json._
import play.api.mvc.{Action, AnyContent, Cookie, PlayBodyParsers, Request, Result}
import security.{
Expand Down Expand Up @@ -621,6 +621,8 @@ class AuthenticationController @Inject()(
dataStoreToken <- bearerTokenAuthenticatorService.createAndInitDataStoreTokenForUser(user)
_ <- organizationService
.createOrganizationDirectory(organization._id, dataStoreToken) ?~> "organization.folderCreation.failed"
_ <- Fox.runOptional(signUpData.acceptedTermsOfService)(version =>
acceptTermsOfServiceForUser(user, version))
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling for Terms of Service acceptance

The ToS acceptance is handled after user creation, but failures in acceptTermsOfServiceForUser might not be properly propagated. Consider:

  1. Validating the ToS version before acceptance
  2. Handling ToS acceptance failures explicitly
- _ <- Fox.runOptional(signUpData.acceptedTermsOfService)(version =>
-   acceptTermsOfServiceForUser(user, version))
+ _ <- Fox.runOptional(signUpData.acceptedTermsOfService) { version =>
+   for {
+     _ <- organizationService.validateTermsOfServiceVersion(version) ?~> "Invalid Terms of Service version"
+     _ <- acceptTermsOfServiceForUser(user, version) ?~> "Failed to accept Terms of Service"
+   } yield ()
+ }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

@frcroth I would prefer if the sign up request gets rejected in case acceptedTermsOfService is not set to true or empty but the application conf says that TOS is enabled.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented now.

} yield {
Mailer ! Send(defaultMails
.newOrganizationMail(organization.name, email, request.headers.get("Host").getOrElse("")))
Expand All @@ -637,6 +639,9 @@ class AuthenticationController @Inject()(
)
}

private def acceptTermsOfServiceForUser(user: User, termsOfServiceVersion: Int)(implicit m: MessagesProvider) =
organizationService.acceptTermsOfService(user._organization, termsOfServiceVersion)(DBAccessContext(Some(user)), m)

case class CreateUserInOrganizationParameters(firstName: String,
lastName: String,
email: String,
Expand Down Expand Up @@ -730,7 +735,8 @@ trait AuthForms {
firstName: String,
lastName: String,
password: String,
inviteToken: Option[String])
inviteToken: Option[String],
acceptedTermsOfService: Option[Int])

def signUpForm(implicit messages: Messages): Form[SignUpData] =
Form(
Expand All @@ -745,8 +751,9 @@ trait AuthForms {
"firstName" -> nonEmptyText,
"lastName" -> nonEmptyText,
"inviteToken" -> optional(nonEmptyText),
)((organization, organizationName, email, password, firstName, lastName, inviteToken) =>
SignUpData(organization, organizationName, email, firstName, lastName, password._1, inviteToken))(
"acceptedTermsOfService" -> optional(number)
)((organization, organizationName, email, password, firstName, lastName, inviteToken, acceptTos) =>
SignUpData(organization, organizationName, email, firstName, lastName, password._1, inviteToken, acceptTos))(
signUpData =>
Some(
(signUpData.organization,
Expand All @@ -755,7 +762,8 @@ trait AuthForms {
("", ""),
signUpData.firstName,
signUpData.lastName,
signUpData.inviteToken))))
signUpData.inviteToken,
signUpData.acceptedTermsOfService))))

// Sign in
case class SignInData(email: String, password: String)
Expand Down
6 changes: 1 addition & 5 deletions app/controllers/OrganizationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package controllers
import org.apache.pekko.actor.ActorSystem
import play.silhouette.api.Silhouette
import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits}
import mail.{DefaultMails, Send}

Expand Down Expand Up @@ -141,10 +140,7 @@ class OrganizationController @Inject()(
def acceptTermsOfService(version: Int): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(request.identity.isOrganizationOwner) ?~> "termsOfService.onlyOrganizationOwner"
_ <- bool2Fox(conf.WebKnossos.TermsOfService.enabled) ?~> "termsOfService.notEnabled"
requiredVersion = conf.WebKnossos.TermsOfService.version
_ <- bool2Fox(version == requiredVersion) ?~> Messages("termsOfService.versionMismatch", requiredVersion, version)
_ <- organizationDAO.acceptTermsOfService(request.identity._organization, version, Instant.now)
_ <- organizationService.acceptTermsOfService(request.identity._organization, version)
} yield Ok
}

Expand Down
14 changes: 12 additions & 2 deletions app/models/organization/OrganizationService.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package models.organization

import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits, TextUtils}
import com.scalableminds.webknossos.datastore.rpc.RPC
import com.typesafe.scalalogging.LazyLogging
Expand All @@ -10,6 +11,7 @@ import models.dataset.{DataStore, DataStoreDAO}
import models.folder.{Folder, FolderDAO, FolderService}
import models.team.{PricingPlan, Team, TeamDAO}
import models.user.{Invite, MultiUserDAO, User, UserDAO, UserService}
import play.api.i18n.{Messages, MessagesProvider}
import play.api.libs.json.{JsArray, JsObject, Json}
import utils.{ObjectId, WkConf}

Expand All @@ -24,8 +26,7 @@ class OrganizationService @Inject()(organizationDAO: OrganizationDAO,
folderService: FolderService,
userService: UserService,
rpc: RPC,
conf: WkConf,
)(implicit ec: ExecutionContext)
conf: WkConf)(implicit ec: ExecutionContext)
extends FoxImplicits
with LazyLogging {

Expand Down Expand Up @@ -165,4 +166,13 @@ class OrganizationService @Inject()(organizationDAO: OrganizationDAO,
def newUserMailRecipient(organization: Organization)(implicit ctx: DBAccessContext): Fox[String] =
fallbackOnOwnerEmail(organization.newUserMailingList, organization)

def acceptTermsOfService(organizationId: String, version: Int)(implicit ctx: DBAccessContext,
m: MessagesProvider): Fox[Unit] =
for {
_ <- bool2Fox(conf.WebKnossos.TermsOfService.enabled) ?~> "termsOfService.notEnabled"
requiredVersion = conf.WebKnossos.TermsOfService.version
_ <- bool2Fox(version == requiredVersion) ?~> Messages("termsOfService.versionMismatch", requiredVersion, version)
_ <- organizationDAO.acceptTermsOfService(organizationId, version, Instant.now)
} yield ()

}
4 changes: 2 additions & 2 deletions conf/application.conf
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ webKnossos {
Please add the information of the operator to comply with GDPR.
"""
termsOfService {
enabled = false
enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

undo me

knollengewaechs marked this conversation as resolved.
Show resolved Hide resolved
# The URL will be embedded into an iFrame
url = "https://webknossos.org/terms-of-service"
acceptanceDeadline = "2023-01-01T00:00:00Z"
Expand Down Expand Up @@ -146,7 +146,7 @@ features {
discussionBoard = "https://forum.image.sc/tag/webknossos"
discussionBoardRequiresAdmin = false
hideNavbarLogin = false
isWkorgInstance = false
isWkorgInstance = true
Copy link
Contributor

Choose a reason for hiding this comment

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

undo me too

recommendWkorgInstance = true
taskReopenAllowedInSeconds = 30
allowDeleteDatasets = true
Expand Down
77 changes: 55 additions & 22 deletions frontend/javascripts/admin/auth/registration_form_generic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import Store from "oxalis/throttled_store";
import messages from "messages";
import { setHasOrganizationsAction } from "oxalis/model/actions/ui_actions";
import { setActiveOrganizationAction } from "oxalis/model/actions/organization_actions";
import { useFetch } from "libs/react_helpers";
import { getTermsOfService } from "admin/api/terms_of_service";

const FormItem = Form.Item;
const { Password } = Input;
Expand All @@ -26,6 +28,8 @@ type Props = {
function RegistrationFormGeneric(props: Props) {
const [form] = Form.useForm();

const terms = useFetch(getTermsOfService, null, []);

const onFinish = async (formValues: Record<string, any>) => {
await Request.sendJSONReceiveJSON(
props.organizationIdToCreate != null
Expand Down Expand Up @@ -274,28 +278,57 @@ function RegistrationFormGeneric(props: Props) {
</FormItem>
</Col>
</Row>
{props.hidePrivacyStatement ? null : (
<FormItem
name="privacy_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.privacy_check_required"])),
},
]}
>
<Checkbox>
I agree to storage and processing of my personal data as described in the{" "}
<a target="_blank" href="/privacy" rel="noopener noreferrer">
privacy statement
</a>
.
</Checkbox>
</FormItem>
)}
<div className="registration-form-checkboxes">
{props.hidePrivacyStatement ? null : (
<FormItem
name="privacy_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.privacy_check_required"])),
},
]}
>
<Checkbox>
I agree to storage and processing of my personal data as described in the{" "}
<a target="_blank" href="/privacy" rel="noopener noreferrer">
privacy statement
</a>
.
</Checkbox>
</FormItem>
)}
{terms != null && !terms.enabled ? null : (
knollengewaechs marked this conversation as resolved.
Show resolved Hide resolved
<FormItem
name="tos_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.tos_check_required"])),
},
]}
>
<Checkbox disabled={terms == null}>
I agree to the{" "}
{terms == null ? (
"terms of service"
) : (
<a target="_blank" href={terms.url} rel="noopener noreferrer">
terms of service
</a>
)}
.
</Checkbox>
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add loading state for terms of service checkbox

The checkbox is disabled when terms are loading, but there's no visual indication to the user.

Consider adding a loading state:

-<Checkbox disabled={terms == null}>
+<Checkbox disabled={terms == null}>
+  {terms == null ? (
+    <span>
+      <LoadingOutlined spin /> Loading terms of service...
+    </span>
+  ) : (
     I agree to the{" "}
     <a target="_blank" href={terms.url} rel="noopener noreferrer">
       terms of service
     </a>
     .
+  )}
 </Checkbox>

Committable suggestion skipped: line range outside the PR's diff.

</FormItem>
)}
</div>

<FormItem>
<Button
size="large"
Expand Down
79 changes: 54 additions & 25 deletions frontend/javascripts/admin/auth/registration_form_wkorg.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import Request from "libs/request";
import Store from "oxalis/throttled_store";
import messages from "messages";
import { setActiveOrganizationAction } from "oxalis/model/actions/organization_actions";
import { useFetch } from "libs/react_helpers";
import { getTermsOfService } from "admin/api/terms_of_service";

const FormItem = Form.Item;
const { Password } = Input;
Expand All @@ -30,6 +32,7 @@ function generateOrganizationId() {
function RegistrationFormWKOrg(props: Props) {
const [form] = Form.useForm();
const organizationId = useRef(generateOrganizationId());
const terms = useFetch(getTermsOfService, null, []);

async function onFinish(formValues: Record<string, any>) {
await Request.sendJSONReceiveJSON("/api/auth/createOrganizationWithAdmin", {
Expand All @@ -43,6 +46,7 @@ function RegistrationFormWKOrg(props: Props) {
},
organization: organizationId.current,
organizationName: `${formValues.firstName.trim()} ${formValues.lastName.trim()} Lab`,
acceptedTermsOfService: terms?.version,
},
});
const [user, organization] = await loginUser({
Expand Down Expand Up @@ -155,32 +159,57 @@ function RegistrationFormWKOrg(props: Props) {
placeholder="Password"
/>
</FormItem>
<div className="registration-form-checkboxes">
<FormItem
name="privacy_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.privacy_check_required"])),
},
]}
>
<Checkbox>
I agree to storage and processing of my personal data as described in the{" "}
<a target="_blank" href="/privacy" rel="noopener noreferrer">
privacy statement
</a>
.
</Checkbox>
</FormItem>

<FormItem
name="privacy_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.privacy_check_required"])),
},
]}
>
<Checkbox>
I agree to storage and processing of my personal data as described in the{" "}
<a target="_blank" href="/privacy" rel="noopener noreferrer">
privacy statement
</a>
.
</Checkbox>
</FormItem>
<FormItem
style={{
marginBottom: 10,
}}
>
{terms != null && !terms.enabled ? null : (
knollengewaechs marked this conversation as resolved.
Show resolved Hide resolved
<FormItem
name="tos_check"
valuePropName="checked"
rules={[
{
validator: (_, value) =>
value
? Promise.resolve()
: Promise.reject(new Error(messages["auth.tos_check_required"])),
},
]}
>
<Checkbox disabled={terms == null}>
I agree to the{" "}
{terms == null ? (
"terms of service"
) : (
<a target="_blank" href={terms.url} rel="noopener noreferrer">
terms of service
</a>
)}
.
</Checkbox>
</FormItem>
)}
</div>

<FormItem>
<Button
size="large"
type="primary"
Expand Down
2 changes: 2 additions & 0 deletions frontend/javascripts/messages.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,8 @@ instead. Only enable this option if you understand its effect. All layers will n
"auth.registration_org_input": "Please select an organization!",
"auth.privacy_check_required":
"Unfortunately, we cannot provide the service without your consent to the processing of your data.",
"auth.tos_check_required":
"Unfortunately, we cannot provide the service without your consent to our terms of service.",
"auth.reset_logout": "You will be logged out, after successfully changing your password.",
"auth.reset_old_password": "Please input your old password!",
"auth.reset_new_password": "Please input your new password!",
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import DisableGenericDnd from "components/disable_generic_dnd";
import { Imprint, Privacy } from "components/legal";
import AsyncRedirect from "components/redirect";
import SecuredRoute from "components/secured_route";
import { CheckTermsOfServices } from "components/terms_of_services_check";
import DashboardView, { urlTokenToTabKeyMap } from "dashboard/dashboard_view";
import DatasetSettingsView from "dashboard/dataset/dataset_settings_view";
import PublicationDetailView from "dashboard/publication_details_view";
Expand Down Expand Up @@ -69,6 +68,7 @@ import loadable from "libs/lazy_loader";
import type { EmptyObject } from "types/globals";
import { DatasetURLImport } from "admin/dataset/dataset_url_import";
import AiModelListView from "admin/voxelytics/ai_model_list_view";
import { CheckTermsOfServices } from "components/terms_of_services_check";

const { Content } = Layout;

Expand Down
10 changes: 10 additions & 0 deletions frontend/stylesheets/main.less
Original file line number Diff line number Diff line change
Expand Up @@ -679,3 +679,13 @@ button.narrow {
.max-z-index {
z-index: 10000000000;
}

.registration-form-checkboxes {
> div:first-of-type {
margin-bottom: 0px;
}

> div:last-of-type {
margin-bottom: 15px;
}
}