As far as I understand this module if multiple users are matched by ip_login_check() there is no way to specify which uid should be selected.

I can imagine an ideal solution to introduce weights to IP matching rules and a possibility to reorder users on admin/settings/ip_login.

But we can also use a simple rule: the more complex a pattern is the higher the weight. Specifying the pattern complexity could be as simple as ordering by pattern length:

$partial_matches = db_query("SELECT uid, ip_match FROM {ip_login_user} WHERE ip_match LIKE ('%s') ORDER by LENGTH(ip_match) DESC", '%' . $addr[0] . '.%');

Comments

jim kirkpatrick’s picture

jim kirkpatrick commented
Priority: Normal » Minor

I've wondered about overlaps before and thought, and then wondered if it's likely, and if it matters. I mean, how many people will have a IP Login-enabled site and also two users with an overlapping IP range?

You're right though on both counts - a weight would be a good idea, as would the rule of thumb that says bigger rules (and therefore less specific matching) are lower priority than short ones. It's a bit hacky though...

I think this is related to #1252994: ip_match should be a multiple value field, possible changes to schema for better performance in that a small schema change could be on the cards. But I also think this is a rare corner case and would rather keep things simple...

Downgrading for now, will think about it more when I have time - if you have any other ideas, please let us know. Thanks mtcs.

jim kirkpatrick’s picture

jim kirkpatrick commented

I've committed a similar change -- mine was ORDER BY ASC, not DESC since shorter IP ranges should be prioritised over long ones. It's a start but will need something more concrete eventually...

mtcs’s picture

mtcs commented

Thanks for considering this.

Having overlapping subnets is indeed reality at large organizations where nested subnets are very common (e.g. department / division / county, etc. ...). IP addresses from a local subnet (e.g. department) have other permissions than those from a broader subnet (e.g. division) or from a different local subnet (other departments). In these situations IPs from a local subnet will also match the IP rule describing a broader address range so rule weights are needed.

I've used DESC because it assumes that a more specific range mask is a longer string (more specific means less IP addresses in range, e.g. 192.* vs. 192.168.* - I want the second mask to fire first).

It's clearly a hack I know.

Introducing weights on the admin interface would be way better. It's not a quick work though.
If you consider using ip2long() (see my other patch: #1271976: Subnet matching), it could also be possible to calculate a better weight automatically.

Since the subnet patch will not work with this simple calculated weight - and I need both of them -, I will continue work on this issue.

jim kirkpatrick’s picture

jim kirkpatrick commented

I'll look at your subnet matching patch later this week, thanks.

I think there are 3 broad - but related - issues the module has presently:

  1. Inability to use standard network notation, as per your [ #1271976] ticket.
  2. Schema issues: Ordering/prioritisation of user accounts (this ticket) and storage of long rules: #1252994: ip_match should be a multiple value field, possible changes to schema for better performance
  3. No support for IPv6

I don't want to do any changes to the schema or code unless we can deal with 1 and 2 cleanly, and ideally prepare for IPv6 stuff which will only get more pressing as time goes on. I note the inet_ntop() function (and its counterpart inet_pton()) look very handy indeed...

jim kirkpatrick’s picture

jim kirkpatrick commented
Status: Active » Postponed

Postponing major changes like this to the forthcoming 2.1 branch, want to get 2.0 out the way and bug-free first.

jvieille’s picture

jvieille commented
Category: Feature request » Bug report
Priority: Minor » Critical
Issue summary: View changes

Allowing multiple matches is a security breach as this openly allows a user to connect to somebody else account.

I would suggest preventing the same IP range to be used by more than one account.

jim kirkpatrick’s picture

jim kirkpatrick commented
Category: Bug report » Feature request
Priority: Critical » Normal

@jvieille, don't agree. By design this module allows someone to log in by IP and without password, and therefore it could easily be seen as "someone else's acount".

So not a security breach, more "administrator beware".

This is a feature request, happy to see patches for this.

amateescu’s picture

amateescu commented
Status: Postponed » Closed (outdated)

Closing old issues.