Minor modifications for admins, implementation of banning system #72

Vogel612 · 2017-03-27T11:48:32Z

Follow up to #65 with continuing changes to privilege system

* Automatically read roomowners and grant them privileges * Allows banning in the same way as adding admins * Banning possible, banned users' command invokations get ignored * Changed to guard clause (once again) * Rooms are now saved locally using their ID Reading the information needed is about 1000 times faster than reading them from the corresponding websites DOM. Adding different domains soon! * Added support for potential other domains. Domain is now included in filename to save the roomowners * Added check for adding privileges for line in properties.

Vogel612 · 2017-03-27T11:53:03Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+		}
+
+		Path file = Paths.get("bot.properties");
+		removePrivFromFile(file, (long) userID, "BANNED");


being banned is not really a privilege, is it?
Also I really dislike stringly typing code. It would be cleaner to either properly separate banning from administrator privileges or to collapse them both into a "modification" which would be encapsulated into an enum:

enum UserModifier { ADMIN, BANNED }

Vogel612 · 2017-03-27T11:53:29Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+	 *            file to add the ID to
+	 * @param newAdmin
+	 *            ID of new admin
+	 */


javadoc is pretty outdated at this point ...

Vogel612 · 2017-03-27T11:54:04Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+			Files.deleteIfExists(tempFile);
+
+			BufferedReader br = new BufferedReader(new FileReader(propertyFile.toFile()));
+			BufferedWriter bw = new BufferedWriter(new FileWriter(tempFile.toFile()));


why are you manually editing the properties-file instead of using the "native" Properties wrapper?

Vogel612 · 2017-03-27T11:54:33Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+			Files.move(tempFile, propertyFile);
+
+		} catch (IOException e) {
+     	Logger.getAnonymousLogger().log(Level.SEVERE, "Problem adding " + role + " " + newEntry + " to property file!", e);


nit: indentation

Vogel612 · 2017-03-27T11:54:49Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+			while((nextLine = br.readLine()) != null){
+				String line = nextLine.toUpperCase().startsWith(role) ? nextLine.replace(Long.toString(newEntry), "").
+						//Regex to clean up the line
+						replace(", ,", ",").replaceAll(", $", "").replace("=, ", "=") : nextLine;


that seems like a really bad idea

Vogel612 · 2017-03-27T11:56:06Z

src/main/java/com/gmail/inverseconduit/commands/sets/AdminCommands.java

-				Path file = Paths.get("bot.properties");
-				addAdminToFile(file, (long) newUserId);
-				return "Added userID " + newUserId + " successful!";
+				return Permissions.addAdmin(newUserId, bc);


this is a bit of a bad idea, because it intermingles application logic (adding the user to admins) with display logic (notifying the user)

So you mean save the return value of the method first and then returning it? Or throwing an exception and catch it with a new response?

no. adding an admin shouldn't return a String for the user, just a Result or something similar. The result can be examined to create a String that's intended for the user:

switch (Permissions.addAdmin(newUserId, bc)) { case ALREADY_EXISTS: return "User already has elevated privileges"; case SUCCESS: return "Added user " + newUserId + " successfully!"; case FAILURE: return "Failed to add admin. Please contact the bot owner"; }

Vogel612 · 2017-03-27T11:56:13Z

src/main/java/com/gmail/inverseconduit/commands/sets/AdminCommands.java

-				removeAdminFromFile(file, (long) remID);
-
-				return "Removed " + remID + " sucessful";
+				return Permissions.removeAdmin(remID, bc);


same as above

Vogel612 · 2017-03-27T11:57:38Z

src/main/java/com/gmail/inverseconduit/commands/sets/CoreBotCommands.java

@@ -182,6 +186,9 @@ private void createNumberCommand() {
    private void createShutdownCommand(ChatInterface chatInterface) {
        CommandHandle shutdown =
                new CommandHandle.Builder("shutdown", message -> {
+                	if (!Permissions.isAdmin((long) message.getUserId(), BOT_CONFIG)) {


static import could make this a bit nicer-looking, but that seems like personal preference ... @Unihedro what's our opinion on static imports?

Vogel612 · 2017-03-27T11:58:37Z

src/main/java/com/gmail/inverseconduit/utils/SourceCodeParser.java

+	 * @param htmlCode Code to look for matches
+	 * @return List of all Matches in the String
+	 */
+	private List<String> getAllMatches(String htmlCode) {


html and regex ... really?
http://stackoverflow.com/a/1732454/1803692

I know that answer, but in other answers, it says that regex is fine to parse a limited and known set of html. In addition to that at least the fetching of the room owners has nothing to do directly with html, but looking for a pattern that shows the id's of the roomowner. This pattern completely ignores the tags. And I know that the way of getting the room name is not the clean (lets be honest, it is horrible...). But I could not find a way of finding stuff in the DOM of a website.
At first I thought of passing message.getRoomName() to the createRoom method, but this would then ignore the rooms that the bot joins during startup.
Is there something to find a certain string pattern in HTML except for RegExes? Or should I just leave the name out of the files?

Unihedro · 2017-04-06T12:00:10Z

src/main/java/com/gmail/inverseconduit/utils/SourceCodeParser.java

+	private List<String> getAllMatches(String htmlCode) {
+		List<String> matches = new ArrayList<>();
+
+		Matcher m = Pattern.compile(searchRegex).matcher(htmlCode);


do not recompile this for every search

Unihedro · 2017-04-06T12:00:48Z

src/main/java/com/gmail/inverseconduit/utils/SourceCodeParser.java

+			br = new BufferedReader(new InputStreamReader(is));
+
+			while ( (line = br.readLine()) != null) {
+				htmlCode += line;


StringBuffer

Unihedro · 2017-04-06T12:03:27Z

src/main/java/com/gmail/inverseconduit/datatype/Room.java

+import com.gmail.inverseconduit.SESite;
+import com.gmail.inverseconduit.utils.SourceCodeParser;
+
+public class Room {


Half of this file is useless? Unnecessary encapsulation

Beeing unsure what you mean I will ask for safety: You mean that I can leave out the getters and setters because the attributes are not supposed to be changed once created? Or did I misunderstood something there?

Unihedro · 2017-04-06T12:34:04Z

src/main/java/com/gmail/inverseconduit/Permissions.java

+	 * @param newAdmin
+	 *            ID of new admin
+	 */
+	private static void addPrivToFile(Path propertyFile, Long newEntry, String role) {


rewriting the file every time is too expensive for our use case.

So better something like: Get current state, modify in program and write it back to the file all at once?

Vogel612 · 2018-02-22T21:05:27Z

This has been stale for a year now...

mvarendorff and others added 4 commits March 27, 2017 12:39

Moved routines to add and remove admins from the command to Permissions

fd66413

Added missing check for admin privileges to shutdown command

d815c7c

Fixxed two mistakes

7a7dcbd

Vogel612 commented Mar 27, 2017

View reviewed changes

Unihedro reviewed Apr 6, 2017

View reviewed changes

Vogel612 closed this Feb 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor modifications for admins, implementation of banning system #72

Minor modifications for admins, implementation of banning system #72

Vogel612 commented Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

mvarendorff Mar 27, 2017 •

edited

Loading

Vogel612 Mar 28, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

Vogel612 Mar 27, 2017

mvarendorff Mar 29, 2017 •

edited

Loading

Unihedro Apr 6, 2017

Unihedro Apr 6, 2017

Unihedro Apr 6, 2017

mvarendorff Apr 9, 2017

Unihedro Apr 6, 2017

mvarendorff Apr 9, 2017

Vogel612 commented Feb 22, 2018

Minor modifications for admins, implementation of banning system #72

Minor modifications for admins, implementation of banning system #72

Conversation

Vogel612 commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvarendorff Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvarendorff Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vogel612 commented Feb 22, 2018

mvarendorff Mar 27, 2017 •

edited

Loading

mvarendorff Mar 29, 2017 •

edited

Loading