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.

Sign up for GitHub

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

Jump to bottom

Elevated privileges now usable #65

Closed

geisterfurz007 wants to merge 17 commits into Unihedro:master from geisterfurz007:master

Contributor

geisterfurz007 commented Mar 12, 2017 •

edited by Unihedro

Summon, Unsummon and Shutdown are now only accessbile for defined userid's.
Userid's that have access to those commands can be specified in the properties file using ADMINS as well as during runtime using commands addAdmin and removeAdmin. Changes are saved for later instances of the bot.

Note: Don't worry about the strange name there; messed up something as I pushed the commit from eclipse...

closes #37

Michelin-Männchen and others added 7 commits

March 10, 2017 22:40


          Built base for privilege system.

b8d0336


          Commit message

d2b8e99


          Base for privilege system.

1109d4d

Added privilege-check to summon, unsummon and shutdown.
Added property to BotConfig that stores admins.


          Update to check commit

c511774


          Added addAdmin command

e1c21b6

Adding will add it to the current instance of the bot as well as to the
bot.properties file. When recompiling this file will be lost. Going to
be one of the next commits!


          Added adding and removing of admins including runtime and the properties

fef73ed


          Merge pull request #1 from geisterfurz007/privilege-Issue37

774145c

Privilege issue37

Vogel612 reviewed

View reviewed changes

src/main/java/com/gmail/inverseconduit/BotConfig.java Outdated

+                  /**
+                   * List of userids with elevated privileges
+                   */
+                  private final List<Integer> admins;

Collaborator

Vogel612 Mar 12, 2017

Considering that the bot can be used in the whole SE network an Integer might not be large enough to hold the largest possible user-id... well that's probably not going to affect us in the foreseeable future anyways, soo ...

Contributor Author

geisterfurz007 Mar 12, 2017

True that! Missed that, thanks for pointing out!

Vogel612 reviewed

View reviewed changes

src/main/java/com/gmail/inverseconduit/BotConfig.java Outdated

+                      		users.add(user);
+                      	} catch (NumberFormatException e) {
+                      	    LOGGER.log(Level.CONFIG, "Skipping unparsable user ID.");
+                              LOGGER.log(Level.FINEST, "", e);

Collaborator

Vogel612 Mar 12, 2017

not quite sure what the purpose of this log-statement is, but it's not indented

Contributor Author

geisterfurz007 Mar 12, 2017

That was mostly copied from the parsing of the room numbers and with that I copied the log-statements. I am fairly new to this, so I thought they might be intended and with that they should be added to the block of reading privileged users as well.

Vogel612 reviewed

View reviewed changes

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

+              					Integer newUserId = Integer.parseInt(args[1]);
+              					if (!bc.getAdmins().contains(newUserId))
+              						bc.getAdmins().add(newUserId);
+              					else return "User already has elevated privileges!";

Collaborator

Vogel612 Mar 12, 2017

please add curlies around here. Just for safety we always do that

Vogel612 reviewed

View reviewed changes

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

+              	public static CommandHandle addAdminCommand(BotConfig bc) {
+              		return new CommandHandle.Builder("addAdmin", message -> {
+              			if (isElevatedUser(message.getUserId(), bc)) {

Collaborator

Vogel612 Mar 12, 2017

this if-condition could be inverted into a so-called "guard clause" to reduce the level of indentation in the following code:

if (!isElevatedUser(message.getUserId(), bc)) {
    return "I'm afraid I cannot let you do that";
}

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

+              	public static CommandHandle removeAdminCommand(BotConfig bc) {
+              		return new CommandHandle.Builder("removeAdmin", message -> {
+              			if (isElevatedUser(message.getUserId(), bc)) {

Collaborator

Vogel612 Mar 12, 2017

same goes for here... would it be viable to extract the privilege-check into the CommandHandle itself?

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

+              		System.out.println(uID);
+              		for (Integer adID : bc.getAdmins()) {
+              			System.out.println(adID);
+              			elevatedUser = adID.equals(uID) ? true : elevatedUser;

Collaborator

Vogel612 Mar 12, 2017

can be more succinct as:

elevatedUser |= adID.equals(uId)

but the whole exercise could become:

public static boolean isElevatedUser(Integer uID, BotConfig bc) {
    return bc.getAdmins().contains(uID);
}

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

+              		} catch (IOException e) {
+              			System.out.println("------------------------------------------------------------------------");
+              			System.out.println("PROBLEM ADDING ADMIN! INVESTIGATION STRONGLY ADVISED!");
+              			System.out.println("------------------------------------------------------------------------");

Collaborator

Vogel612 Mar 12, 2017

this is an error ... accordingly we also need to log the exception to properly investigate.

If you want to go extra fancy, you could change the text color for the console display, but ... meh

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

+              		} catch (IOException e){
+              			System.out.println("------------------------------------------------------------------------");
+              			System.out.println("PROBLEM REMOVING ADMIN! INVESTIGATION STRONGLY ADVISED!");
+              			System.out.println("------------------------------------------------------------------------");

Collaborator

Vogel612 Mar 12, 2017

same as above

src/main/java/com/gmail/inverseconduit/commands/sets/ChatCommands.java Outdated

+              				SeChatDescriptor descriptor = SeChatDescriptor.buildSeChatDescriptorFrom(message);
+              				chatInterface.leaveChat(descriptor);
+              				return "*~bye, bye*";
+              			} return "I am afraid, I cannot let you do that!!";

Collaborator

Vogel612 Mar 12, 2017

yup. the privilege check should be baked into CommandHandle. We just need to keep track of whether a command is a privileged command and then check the admin status before making the message available

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

+              	private void createRemoveCommand(){
+              		allCommands.add(AdminCommands.removeAdminCommand(BOT_CONFIG));
+              	}

Collaborator

Vogel612 Mar 12, 2017

Why has this complete file been reformatted?

geisterfurz007 and others added 4 commits

March 12, 2017 14:36


          Fixxed mistakes shown by Vogel612

df8c030


          Merge pull request #2 from geisterfurz007/privilege-Issue37

810d931

Fixxed mistakes shown by Vogel612


          Set logging level to severe for adding/removing a userid

4a7743f


          Merge pull request #3 from geisterfurz007/privilege-Issue37

728803c

Set logging level to severe for adding/removing a userid

Collaborator

Vogel612 commented Mar 15, 2017

LGTM. @Unihedro you got any changes you want to see?

Unihedro reviewed

View reviewed changes

Owner

Unihedro left a comment

also I'm not really sure if we should be using long for ids but just to be safe I guess

src/main/java/com/gmail/inverseconduit/BotConfig.java Outdated

+                  /**
+                   * List of userids with elevated privileges
+                   */
+                  private final List<Long> admins;

Owner

Unihedro Mar 16, 2017

Perhaps a Set is more appropriate since we are only using contains, add and remove?

src/main/java/com/gmail/inverseconduit/BotConfig.java Outdated

@@ @@ -103,6 +123,10 @@ public Path getJavadocsDir() { @@
                       return javadocs;
                   }
+                  public List<Long> getAdmins() {

Owner

Unihedro Mar 16, 2017

I'm not liking this, we should separate the method into isAdmin(user), removeAdmin(user) and addAdmin(user) so that we can implement automatically saving the changes (in the latter two parts) without deprecating old code.

If doing it in the BotConfig file isn't the right way I propose:

convert getAdmins into default privacy
creating and using Permissions class in the same package
create the middleman functions in it

Owner

Unihedro Mar 16, 2017

I'm aware that the commands already handle this but eventually we might want to watch for privilege escalation socket events so it better be separated for now.

src/main/java/com/gmail/inverseconduit/commands/sets/ChatCommands.java Outdated

-                  }
+              	public static CommandHandle unsummonCommand(ChatInterface chatInterface, BotConfig bc) {
+              		return new CommandHandle.Builder("unsummon", message -> {
+              			if (AdminCommands.isElevatedUser((long) message.getUserId(), bc)) {

Owner

Unihedro Mar 16, 2017

change to a guard clause:

if (!condition) // {
  terminate with instruction
// } end block scope

everything else

This way we can avoid building arrow code.

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

@@ @@ -204,4 +207,13 @@ private void createTestCommand() { @@
                       }).build();
                       allCommands.add(test);
                   }
-              }
+                  private void createAddCommand() {

Owner

Unihedro Mar 16, 2017

propose renaming to createAddAdminCommand and createRemoveAdminCommand


          Editted according to review

7b5b55a

Contributor Author

geisterfurz007 commented Mar 16, 2017

I hope, I got the permission thing correctly. I was unsure whether to use static methods or create an Object with the BotConfig as field so I went with the static approach first. If this should be changed I can do so.

Unihedro reviewed

View reviewed changes

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

+              	public static void addAdmin(Long userID, BotConfig bc) {
+              		bc.getAdmins().remove(userID);
+              	}

Owner

Unihedro Mar 16, 2017

These looks reversed.

Unihedro added the ready label


          Corrected add to add and remove to remove

ec8dd33

Contributor Author

geisterfurz007 commented Mar 16, 2017

No idea what went on there... Thanks for the hawk eye!

Owner

Unihedro commented Mar 16, 2017

👍 LGTM and the technical debt seems small enough; did not test

Owner

Unihedro commented Mar 16, 2017

closes #37

geisterfurz007 and others added 4 commits

March 17, 2017 17:54


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

b498aba


          Added missing check for admin privileges to shutdown command

91d5992


          Automatically loading of roomowners and the ability to ban/unban (#5)

f83ec68

* 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.


          Fixxed two mistakes

e1e4baf

Vogel612 added a commit that referenced this pull request


          Merge reviewed commits of PullRequest #65 manually

7976bf2

Collaborator

Vogel612 commented Mar 25, 2017 •

edited

hmm ... okay GH can't quite keep up ...

@geisterfurz007 would you please move the commits after "corrected add to add and remove to remove" into a new pull request so that they can be properly reviewed? Thanks 👍

Vogel612 closed this

Contributor Author

geisterfurz007 commented Mar 26, 2017

I am afraid that it seems like I need some assistance with that... I tried to revert the changes of the last commits, but that did not work out. Or can I just move the commits over to a new PR? I am fairly new to git and not familiar with the things to do, sorry.

Collaborator

Vogel612 commented Mar 26, 2017

@geisterfurz007 no worries.

The following "dark magical rites of git" should help you:

git checkout master
git remote add blessed git@github.com:Unihedro/JavaBot.git
git fetch blessed
git rebase blessed/master
git push --force

let me explain a little what happens here.

make sure we're on the correct branch
grab a set of the changes that happened in this repo
for that we add this repository as remote and fetch the changes
move all the stuff we don't yet have in master here up onto master
and put it remote explicitly overriding safety mechanisms

if you have any questions, feel invited to ping me 👍

Contributor Author

geisterfurz007 commented Mar 27, 2017

@Vogel612 Ok so if I understand correctly I used the master branch of this project to rever the changes on the master branch of my fork? So next thing to do would be creating a new pull request with the branch I worked on as the one to compare?

Vogel612 mentioned this pull request

Minor modifications for admins, implementation of banning system #72

Closed

Collaborator

Vogel612 commented Mar 27, 2017

@geisterfurz007 well ... not quite, but it's close enough.

Yes, usually you'd not make a pull request from master to master, but .. since you already worked on your master this is the simplest way to fix things up :)

I recommend you create topic branches from now on and sync your master with Unihedro/JavaBot:master regularly to reduce merge-conflicts.
FWIW I took the liberty to open the follow-up pull request for you

Contributor Author

geisterfurz007 commented Mar 27, 2017

@Vogel612 Well if it was close enough I will take that for now!
I had that in mind, but somewhere I lost it appearently...
Thanks for opening!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment