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 an option to restrict which locales a user can edit #1004

Merged
merged 4 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void testDelete() {
String commonName = "Test Mojito";

userService.createUserWithRole(
username, password, Role.ROLE_USER, givenName, surname, commonName, false);
username, password, Role.ROLE_USER, givenName, surname, commonName, null, true, false);
User user = userRepository.findByUsername(username);
assertNotNull(user);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ private User createTestUserUsingUserService(String username, String rolename) {
}
User user =
userService.createUserWithRole(
username, password, role, givenName, surname, commonName, false);
username, password, role, givenName, surname, commonName, null, true, false);
return user;
}
}
20 changes: 20 additions & 0 deletions restclient/src/main/java/com/box/l10n/mojito/rest/entity/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ public class User {

private String commonName;

private boolean canTranslateAllLocales;

@JsonManagedReference Set<Authority> authorities = new HashSet<>();

@JsonManagedReference Set<UserLocale> userLocales = new HashSet<>();

public Long getId() {
return id;
}
Expand Down Expand Up @@ -90,4 +94,20 @@ public Set<Authority> getAuthorities() {
public void setAuthorities(Set<Authority> authorities) {
this.authorities = authorities;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public Set<UserLocale> getUserLocales() {
return userLocales;
}

public void setUserLocales(Set<UserLocale> userLocales) {
this.userLocales = userLocales;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.box.l10n.mojito.rest.entity;

import com.fasterxml.jackson.annotation.JsonBackReference;

public class UserLocale {
@JsonBackReference private User user;

private Locale locale;

public User getUser() {
return user;
}

public void setUser(User user) {
this.user = user;
}

public Locale getLocale() {
return locale;
}

public void setLocale(Locale locale) {
this.locale = locale;
}
}
4 changes: 2 additions & 2 deletions webapp/src/main/java/com/box/l10n/mojito/FlyWayConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ void tryToMigrateIfMysql8Migration(Flyway flyway, FlywayException fe) {
* since a failure to perform the query will show that the DB is not protected.
*
* <p>This is an extra check added to the settings: spring.flyway.clean-disabled=true (now default
* in Mojito) and l10n.flyway.clean=false (that is usually set manually, but can be wrongly enabled)
* and shouldn't be solely relied upon.
* in Mojito) and l10n.flyway.clean=false (that is usually set manually, but can be wrongly
* enabled) and shouldn't be solely relied upon.
*
* <p>For now this is enabled manually in the database with: CREATE TABLE
* flyway_clean_protection(enabled boolean default true); INSERT INTO flyway_clean_protection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@
name = "User.legacy",
attributeNodes = {
@NamedAttributeNode("createdByUser"),
@NamedAttributeNode(value = "userLocales", subgraph = "User.legacy.userLocales"),
@NamedAttributeNode(value = "authorities", subgraph = "User.legacy.authorities")
},
subgraphs = {
@NamedSubgraph(
name = "User.legacy.authorities",
attributeNodes = {@NamedAttributeNode("createdByUser"), @NamedAttributeNode("user")})
attributeNodes = {@NamedAttributeNode("createdByUser"), @NamedAttributeNode("user")}),
@NamedSubgraph(
name = "User.legacy.userLocales",
attributeNodes = {@NamedAttributeNode("user")}),
})
public class User extends AuditableEntity implements Serializable {

Expand Down Expand Up @@ -69,6 +73,9 @@ public class User extends AuditableEntity implements Serializable {
@JsonView(View.IdAndName.class)
String commonName;

@Column(name = "can_translate_all_locales", nullable = false)
boolean canTranslateAllLocales = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the default user role is USER I think we should default this value to false, otherwise it might be confusing in the GUI that a basic user has this enabled but still can't translate any locales

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, we should keep it as true. The default role for new users is USER, so it will likely be fairly common to accidentally create USER users instead of TRANSLATOR users. If the ADMIN then changes the role for the new user, then the new user will still be unable to translate anything, because the canTranslateAllLocales is now false. --> The ADMIN now has to change two settings instead of one.

Additionally, in your scenario, what is supposed to happen if the ADMIN downgrades a TRANSLATOR to USER? Should canTranslateAllLocales also be disabled? In this case the role selection dropdown would effectively control two inputs, which feels inconsistent and unintuitive. If the canTranslateAllLocales field is not disabled it is inconsistent with creating a new user.


So, I would rather solve this in the GUI, by disabling the checkbox for USER and displaying a short note explaining that this option won't have an effect because of the currently selected role. IMHO, this would have the least surprising behaviour for all users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for it to be handled in the GUI as you describe 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be implemented now.


/**
* Sets this flag if the user is created by a process that don't have all the information. Eg.
* pushing an asset for a branch with an owner or header base authentication. If the owner is not
Expand All @@ -80,6 +87,10 @@ public class User extends AuditableEntity implements Serializable {
@Column(name = "partially_created")
Boolean partiallyCreated = false;

@JsonManagedReference
@OneToMany(mappedBy = "user", fetch = FetchType.LAZY)
Set<UserLocale> userLocales = new HashSet<>();

@JsonManagedReference
@OneToMany(mappedBy = "user", fetch = FetchType.LAZY)
Set<Authority> authorities = new HashSet<>();
Expand Down Expand Up @@ -156,6 +167,22 @@ public void setCommonName(String commonName) {
this.commonName = commonName;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public Set<UserLocale> getUserLocales() {
return userLocales;
}

public void setUserLocales(Set<UserLocale> userLocales) {
this.userLocales = userLocales;
}

public Boolean getPartiallyCreated() {
return partiallyCreated;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package com.box.l10n.mojito.entity.security.user;

import com.box.l10n.mojito.entity.BaseEntity;
import com.box.l10n.mojito.entity.Locale;
import com.box.l10n.mojito.rest.View;
import com.fasterxml.jackson.annotation.JsonBackReference;
import com.fasterxml.jackson.annotation.JsonView;
import jakarta.persistence.Entity;
import jakarta.persistence.ForeignKey;
import jakarta.persistence.Index;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import java.io.Serializable;
import org.hibernate.annotations.BatchSize;

@Entity
@Table(
name = "user_locale",
indexes = {
@Index(
name = "UK__USER_LOCALE__USER_ID__LOCALE_ID",
columnList = "user_id, locale_id",
unique = true)
})
@BatchSize(size = 1000)
public class UserLocale extends BaseEntity implements Serializable {

@ManyToOne
@JsonBackReference
@JoinColumn(
name = "user_id",
foreignKey = @ForeignKey(name = "FK__USER_LOCALE__USER__ID"),
nullable = false)
User user;

@JsonView(View.LocaleSummary.class)
@ManyToOne
@JoinColumn(
name = "locale_id",
foreignKey = @ForeignKey(name = "FK__USER_LOCALE__LOCALE__ID"),
nullable = false)
Locale locale;

public UserLocale() {}

public UserLocale(User user, Locale locale) {
this.user = user;
this.locale = locale;
}

public User getUser() {
return user;
}

public void setUser(User user) {
this.user = user;
}

public Locale getLocale() {
return locale;
}

public void setLocale(Locale locale) {
this.locale = locale;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ public <I, O> PollableFuture<O> scheduleJobWithCustomTimeout(
* @param expectedSubTaskNumber set on the pollable task
* @param triggerStartDate date at which the job should be started
* @param uniqueId optional id used to generate the job keyname. If not provided the pollable task
* id is used. Pollable id keeps changing, unique id can be used for recurring jobs (eg. update
* stats of repository xyz)
* id is used. Pollable id keeps changing, unique id can be used for recurring jobs (eg.
* update stats of repository xyz)
* @param inlineInput to inline the input in quartz data or save it in the blobstorage
* @param <I>
* @param <O>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.box.l10n.mojito.react;

import com.box.l10n.mojito.entity.security.user.Authority;
import com.box.l10n.mojito.entity.security.user.UserLocale;
import com.box.l10n.mojito.json.ObjectMapper;
import com.box.l10n.mojito.mustache.MustacheBaseContext;
import com.box.l10n.mojito.mustache.MustacheTemplateEngine;
Expand All @@ -15,6 +16,7 @@
import java.nio.charset.StandardCharsets;
import java.util.IllformedLocaleException;
import java.util.Locale;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -134,6 +136,12 @@ ReactUser getReactUser() {
reactUser.setGivenName(currentAuditor.getGivenName());
reactUser.setSurname(currentAuditor.getSurname());
reactUser.setCommonName(currentAuditor.getCommonName());
reactUser.setCanTranslateAllLocales(currentAuditor.getCanTranslateAllLocales());
reactUser.setUserLocales(
currentAuditor.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(com.box.l10n.mojito.entity.Locale::getBcp47Tag)
.collect(Collectors.toList()));

Role role = Role.ROLE_USER;
Authority authority = authorityRepository.findByUser(currentAuditor);
Expand Down
19 changes: 19 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/react/ReactUser.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.box.l10n.mojito.react;

import com.box.l10n.mojito.security.Role;
import java.util.List;
import org.springframework.stereotype.Component;

@Component
Expand All @@ -11,6 +12,8 @@ public class ReactUser {
String surname;
String commonName;
Role role;
boolean canTranslateAllLocales;
List<String> userLocales;

public String getUsername() {
return username;
Expand Down Expand Up @@ -51,4 +54,20 @@ public Role getRole() {
public void setRole(Role role) {
this.role = role;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public List<String> getUserLocales() {
return userLocales;
}

public void setUserLocales(List<String> userLocales) {
this.userLocales = userLocales;
}
}
13 changes: 13 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/rest/security/UserWS.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
import static org.slf4j.LoggerFactory.getLogger;
import static org.springframework.data.jpa.domain.Specification.where;

import com.box.l10n.mojito.entity.Locale;
import com.box.l10n.mojito.entity.security.user.Authority;
import com.box.l10n.mojito.entity.security.user.User;
import com.box.l10n.mojito.entity.security.user.UserLocale;
import com.box.l10n.mojito.security.Role;
import com.box.l10n.mojito.service.security.user.UserRepository;
import com.box.l10n.mojito.service.security.user.UserService;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.data.domain.Page;
Expand Down Expand Up @@ -98,6 +101,11 @@ public ResponseEntity<User> createUser(@RequestBody User user) {
user.getGivenName(),
user.getSurname(),
user.getCommonName(),
user.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(Locale::getBcp47Tag)
.collect(Collectors.toSet()),
user.getCanTranslateAllLocales(),
false);

return new ResponseEntity<>(createdUser, HttpStatus.CREATED);
Expand Down Expand Up @@ -151,6 +159,11 @@ public void updateUserByUserId(@PathVariable Long userId, @RequestBody User user
user.getGivenName(),
user.getSurname(),
user.getCommonName(),
user.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(Locale::getBcp47Tag)
.collect(Collectors.toSet()),
user.getCanTranslateAllLocales(),
false);
}

Expand Down
Loading