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

GUACAMOLE-1020: Implement extension to enable additional restrictions #830

Merged

Conversation

necouchman
Copy link
Contributor

I've taken a run at implementing a decorating extension that allows users, groups, connections, and connection groups, to be restricted beyond the defaults provided by the base Guacamole implementation:

  • Restrict the times at which users can log in based on a "Day of the Week" schedule. This is implemented using both an "Allow at certain times" field, which, if present, will restrict the user to logins only during those times, and a "Deny at certain times" field, which, if present, will block the user from logging in during the specified times. This is implemented at both the individual user level, as well as a setting that can be applied to a group and will impact all the members of that group.
    Screenshot_2023-04-11_09-33-34
    Screenshot_2023-04-11_10-46-48

  • Restrict the hosts from which users can log in, based on hostname, IP address, or CIDR notation. I've attempted to implement both IPv4 and IPv6 restrictions. Hostnames will be reverse-queried to resolve to IPs, and then they are checked against the user's login IP, if it's available.
    Screenshot_2023-04-11_10-50-45

  • Restrict the times at which connections and/or connection groups (of the Balancing variety) can be accessed, in the same "Day of the Week" schedule.

  • Restrict the hosts from which connections and/or connection groups (of the Balancing variety) can be accessed, using hostname, IP address, and/or CIDR range.
    Screenshot_2023-04-11_11-06-47

@necouchman necouchman force-pushed the working/login-restrictions branch 2 times, most recently from 2f93e27 to d76cd50 Compare April 11, 2023 15:27
@necouchman necouchman force-pushed the working/login-restrictions branch 2 times, most recently from f5ea89b to c3cc3cc Compare April 12, 2023 22:25
@jmuehlner
Copy link
Contributor

jmuehlner commented Apr 12, 2023

Had you considered allowing either the weekday or time parts of the restrictions to be left out, rather than requiring both? It looks like most of the code would already be pretty close to supporting that.

I think it could be pretty handy - for example:

An admin might prefer to add rule a that a user can access a connection from 9:00 to 17:00 every day, and also add a couple of rules that the user cannot access the connection on Saturday or Sunday.

As opposed to right now it looks like they'd have to add 5 rules, one for each day of the week, and if they wanted to change the hours, they'd have to change all 5 of the rules.

If this is hard to implement, I'm fine with leaving it as a future enhancement,

@necouchman necouchman force-pushed the working/login-restrictions branch 2 times, most recently from d14b316 to 0b16370 Compare April 13, 2023 02:25
@necouchman
Copy link
Contributor Author

necouchman commented Apr 13, 2023

Had you considered allowing either the weekday or time parts of the restrictions to be left out, rather than requiring both? It looks like most of the code would already be pretty close to supporting that.

I had thought about it, but not quite so thorougly.

An admin might prefer to add rule a that a user can access a connection from 9:00 to 17:00 every day, and also add a couple of rules that the user cannot access the connection on Saturday or Sunday.

This should be pretty easy to do - I could add a RegEx/parsing rule that looks for an * to create a rule for every day. I could also look for values like WD for Week Day and WE for Week End. It should be pretty easy to map these through to the selection box on the web side so that you get Monday - Sunday, and then three more options: Every Day, Week Days, and Week Ends.

As opposed to right now it looks like they'd have to add 5 rules, one for each day of the week, and if they wanted to change the hours, they'd have to change all 5 of the rules.

Yep, that could be quite cumbersome.

If this is hard to implement, I'm fine with leaving it as a future enhancement,

Nah, I'll take a run at it, I think it should be pretty easy. Thanks for the suggestion!

@necouchman
Copy link
Contributor Author

@jmuehlner I've taken a run at implementing what I think you were getting at with the multi-day options.
Screenshot_2023-04-13_13-20-14

I also tweaked it so that 1) date is always stored in UTC in the database, and 2) the front-end form sticks with the user's timezone for the field itself, then translates to UTC when storing in the backend.

@necouchman necouchman force-pushed the working/login-restrictions branch 2 times, most recently from 796f556 to e11cf80 Compare April 13, 2023 18:12
@necouchman necouchman force-pushed the working/login-restrictions branch 2 times, most recently from ea84c94 to 0158509 Compare September 9, 2024 18:24
@jmuehlner
Copy link
Contributor

LGTM, but it looks like there's an outstanding change request by @mike-jumper

@necouchman
Copy link
Contributor Author

LGTM, but it looks like there's an outstanding change request by @mike-jumper

I'm not seeing any unresolved issues, but I'll scroll through it again and make sure, and wait for Mike's approval.

@jmuehlner
Copy link
Contributor

LGTM, but it looks like there's an outstanding change request by @mike-jumper

I'm not seeing any unresolved issues, but I'll scroll through it again and make sure, and wait for Mike's approval.

Ah, looks like it was just the comment:

Initial partial review - I'm about 50% done reading through the full set of changes.

@mike-jumper
Copy link
Contributor

LGTM, but it looks like there's an outstanding change request by @mike-jumper

I'm not seeing any unresolved issues, but I'll scroll through it again and make sure, and wait for Mike's approval.

Ah, looks like it was just the comment:

Initial partial review - I'm about 50% done reading through the full set of changes.

Heh - nice. :)

I'll read through things now.

@jmuehlner
Copy link
Contributor

LGTM, but it looks like there's an outstanding change request by @mike-jumper

I'm not seeing any unresolved issues, but I'll scroll through it again and make sure, and wait for Mike's approval.

Ah, looks like it was just the comment:

Initial partial review - I'm about 50% done reading through the full set of changes.

Heh - nice. :)

I'll read through things now.

@mike-jumper any objections if I merge this? Did you stil have more you wanted to read through?

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

Going to double-check the restriction logic, but everything else looks good.

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

One minor change needed to ensure we don't issue too many queries. Otherwise, LGTM!

Comment on lines +154 to +156
// Check and see if the logged in user has admin privileges -
// either system-level or for that particular object.
boolean hasAdmin = isAdmin || adminIdentifiers.contains(object.getIdentifier());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I like this.

@mike-jumper mike-jumper merged commit 5d44ae4 into apache:staging/1.6.0 Oct 2, 2024
1 check passed
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.

5 participants