ExtremeFiretop / MerlinAutoUpdate-Router

Merlin(A)uto(U)pdate is a Merlin router script which allows you to remotely identify a stable firmware update for an ASUS Merlin router, and automatically download and update via an unattended method directly from the router.
https://www.snbforums.com/threads/merlinau-v1-2-7-the-ultimate-firmware-auto-updater-amtm-addon.91326/
GNU General Public License v3.0
15 stars 1 forks source link

Detect Access Restrictions #292

Closed ExtremeFiretop closed 1 month ago

ExtremeFiretop commented 1 month ago

Detect Access Restrictions to address reported issue below from @SolidRhino

https://github.com/ExtremeFiretop/MerlinAutoUpdate-Router/issues/290

ExtremeFiretop commented 1 month ago

@Martinski4GitHub

Thoughts on this solution? Food for thought.

Martinski4GitHub commented 1 month ago

@Martinski4GitHub

Thoughts on this solution? Food for thought.

It seems that there's a misunderstanding WRT the "Enable Access Restrictions" feature in the webGUI. When this option is enabled, it does not necessarily mean that all access to the webGUI has indeed been restricted.

For example, the user may have selected to restrict access only to the SSH port, in which case the webGUI login is not affected at all. Or, the user may have configured the option to have restricted access from all LAN devices except for a select few personal devices (via their reserved DHCP IP addresses). These are just 2 examples, and there are probably other use cases.

The point is that, in and of itself, having the "Enable Access Restrictions" option enabled is not necessarily a barrier to a successful webGUI login.

IMO, what we can do instead is improve on the message displayed when the login failed by listing the most common sources of failure so that the users can go ahead and double-check for themselves and take any actions as needed.

See PR #293 which I just submitted to improve on the message.

My 2 cents.

ExtremeFiretop commented 1 month ago

@Martinski4GitHub Thoughts on this solution? Food for thought.

It seems that there's a misunderstanding WRT the "Enable Access Restrictions" feature in the webGUI. When this option is enabled, it does not necessarily mean that all access to the webGUI has indeed been restricted.

For example, the user may have selected to restrict access only to the SSH port, in which case the webGUI login is not affected at all. Or, the user may have configured the option to have restricted access from all LAN devices except for a select few personal devices (via their reserved DHCP IP addresses). These are just 2 examples, and there are probably other use cases.

The point is that, in and of itself, having the "Enable Access Restrictions" option enabled is not necessarily a barrier to a successful webGUI login.

IMO, what we can do instead is improve on the message displayed when the login failed by listing the most common sources of failure so that the users can go ahead and double-check for themselves and take any actions as needed.

See PR #293 which I just submitted to improve on the message.

My 2 cents.

No misunderstanding, I fully understand how it works. But if it's enabled, then I'm not going to ask a user to allow the IP of the router, that is way more complicated. It's simpler to just ask it be disabled.

Martinski4GitHub commented 1 month ago

@Martinski4GitHub Thoughts on this solution? Food for thought.

It seems that there's a misunderstanding WRT the "Enable Access Restrictions" feature in the webGUI. When this option is enabled, it does not necessarily mean that all access to the webGUI has indeed been restricted. For example, the user may have selected to restrict access only to the SSH port, in which case the webGUI login is not affected at all. Or, the user may have configured the option to have restricted access from all LAN devices except for a select few personal devices (via their reserved DHCP IP addresses). These are just 2 examples, and there are probably other use cases. The point is that, in and of itself, having the "Enable Access Restrictions" option enabled is not necessarily a barrier to a successful webGUI login. IMO, what we can do instead is improve on the message displayed when the login failed by listing the most common sources of failure so that the users can go ahead and double-check for themselves and take any actions as needed. See PR #293 which I just submitted to improve on the message. My 2 cents.

No misunderstanding, I fully understand how it works. But if it's enabled, then I'm not going to ask a user to allow the IP of the router, that is way more complicated. It's simpler to just ask it be disabled.

There are absolutely valid configurations for the "Enable Access Restrictions" option where access to the webGUI is not restricted at all, or it may be restricted except for very specific devices that the user wants to use exclusively for router access. Why would you want to force users to disable their valid configuration if/when it's not affecting the router login process? I don't see the point of being heavy-handed & shortsighted about this.

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

ExtremeFiretop commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

ExtremeFiretop commented 1 month ago

There are absolutely valid configurations for the "Enable Access Restrictions" option where access to the webGUI is not restricted at all, or it may be restricted except for very specific devices that the user wants to use exclusively for router access. Why would you want to force users to disable their valid configuration if/when it's not affecting the router login process? I don't see the point of being heavy-handed & shortsighted about this.

Check commit: https://github.com/ExtremeFiretop/MerlinAutoUpdate-Router/pull/292/commits/77872b30ebbfafd75f9dd72e9a1d124080422ed4 To address your valid configurations..

Your PR is messaging only, which is not enough for a feature like this I feel. If we want to avoid the questions, instructions alone aren't enough

ExtremeFiretop commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Check @SolidRhino's response to confirm:

I have added the router to the allowed list. Then it works.

Martinski4GitHub commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Ah, yes, you're right. I completely forgot about that. In this case, the user can add the router IP address. That would be a simpler request than forcing users to completely disable an otherwise valid configuration, IMO.

ExtremeFiretop commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Ah, yes, you're right. I completely forgot about that. In this case, the user can add the router IP address. That would be a simpler request than forcing users to completely disable an otherwise valid configuration, IMO.

I just thought of the perfect hybrid solution. What do you think of this...

What if we check if the rulelist contains the routers IP if access restrictions is on?

If it's there great! Otherwise. We display the advanced messaging. But we don't block like I currently do, we just display the messaging to disable or add the IP.

Thoughts on that?

Martinski4GitHub commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Ah, yes, you're right. I completely forgot about that. In this case, the user can add the router IP address. That would be a simpler request than forcing users to completely disable an otherwise valid configuration, IMO.

I just thought of the perfect hybrid solution. What do you think of this...

What if we check if the rulelist contains the routers IP if access restrictions is on?

If it's there great! Otherwise. We display the advanced messaging. But we don't block like I currently do, we just display the messaging to disable or add the IP.

Yes, that looks like a much better solution. The key point for MerlinAU is that the script must have access to the webGUI so if the "Enable Access Restrictions" option is enabled and the access list includes the router IP address then we're good to go and we don't prompt the user to do anything else about the option. Otherwise, a message is displayed instructing them to add the router IP address.

ExtremeFiretop commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Ah, yes, you're right. I completely forgot about that. In this case, the user can add the router IP address. That would be a simpler request than forcing users to completely disable an otherwise valid configuration, IMO.

I just thought of the perfect hybrid solution. What do you think of this... What if we check if the rulelist contains the routers IP if access restrictions is on? If it's there great! Otherwise. We display the advanced messaging. But we don't block like I currently do, we just display the messaging to disable or add the IP.

Yes, that looks like a much better solution. The key point for MerlinAU is that the script must have access to the webGUI so if the "Enable Access Restrictions" option is enabled and the access list includes the router IP address then we're good to go and we don't prompt the user to do anything else about the option. Otherwise, a message is displayed instructing them to add the router IP address.

I'm about to head to bed... my fathers been needing help getting around due to cataract surgery and I need to be up tomorrow morning to bring him to the eye clinic.

But I'll work on that in the morning now that we have a game plan. The messaging adjustments you did are good but too generic and I feel can be ignored/missed by a user.

But maybe if we do this check and give them a big red splash warning to add the IP (or disable) when setting the password if it's on, they might actually do it.

Martinski4GitHub commented 1 month ago

BTW, it's the IP address of the network client that must be allowed to have access, not the IP of the router.

No. In the case of MerlinAU, the router IS the client. Therefore, for it to work with it enabled, they would need the routers interface IP, aka, the clients to be added for it to work.

Ah, yes, you're right. I completely forgot about that. In this case, the user can add the router IP address. That would be a simpler request than forcing users to completely disable an otherwise valid configuration, IMO.

I just thought of the perfect hybrid solution. What do you think of this... What if we check if the rulelist contains the routers IP if access restrictions is on? If it's there great! Otherwise. We display the advanced messaging. But we don't block like I currently do, we just display the messaging to disable or add the IP.

Yes, that looks like a much better solution. The key point for MerlinAU is that the script must have access to the webGUI so if the "Enable Access Restrictions" option is enabled and the access list includes the router IP address then we're good to go and we don't prompt the user to do anything else about the option. Otherwise, a message is displayed instructing them to add the router IP address.

I'm about to head to bed... my fathers been needing help getting around due to cataract surgery and I need to be up tomorrow morning to bring him to the eye clinic.

I understand. I hope your Father is doing well in his recovery. Both my Mother and my Mother-in-law have had that surgery (separate times) and my wife & I took them to the clinic the day of the surgery and then to the post-op appointments. My Father will likely need the same surgery within the next 6-12 months since his cataracts seem to be getting worse.

But I'll work on that in the morning now that we have a game plan. The messaging adjustments you did are good but too generic and I feel can be ignored/missed by a user.

But maybe if we do this check and give them a big red splash warning to add the IP (or disable) when setting the password if it's on, they might actually do it.

Sounds good. Have a good night & talk to you tomorrow evening.

ExtremeFiretop commented 1 month ago

I understand. I hope your Father is doing well in his recovery. Both my Mother and my Mother-in-law have had that surgery (separate times) and my wife & I took them to the clinic the day of the surgery and then to the post-op appointments. My Father will likely need the same surgery within the next 6-12 months since his cataracts seem to be getting worse.

Thanks for the wishes. It's been tiring. I have work in the day hours, my side gig needing a rack server upgrade, and my father needing help getting around to his appointments, hospital, etc. Then there's always MerlinAU and issues being opened 😉

Love my parents, and will always help, but it can be draining on time and energy.

But I'll work on that in the morning now that we have a game plan. The messaging adjustments you did are good but too generic and I feel can be ignored/missed by a user. But maybe if we do this check and give them a big red splash warning to add the IP (or disable) when setting the password if it's on, they might actually do it.

Sounds good. Have a good night & talk to you tomorrow evening.

Sounds good, thanks buddy. I merged yours in, I'll adjust mine to try to address this idea when I have a second. Sleep well and have a goodnight

ExtremeFiretop commented 1 month ago

@Martinski4GitHub

Ready for review, tested and worked for me :)

ExtremeFiretop commented 1 month ago

It's looking good to go. I'll verify a couple of use cases after merging.

If you find a use case that's valid that this doesn't correctly flag let me know or open a PR!

So I tested with access restrictions disabled. With it on, but the router not added. with the router added but without access (SSH only). And the router added with access to the WebUI.

ExtremeFiretop commented 1 month ago

It's looking good to go. I'll verify a couple of use cases after merging.

And just as an FYI it can be tested without dropping Internet access, but it will boot you out of any logins you currently have.