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

Previous owner can sometimes add themselves again after runaway #979

Closed
NikkiLacrima opened this issue Aug 12, 2023 · 10 comments
Closed

Previous owner can sometimes add themselves again after runaway #979

NikkiLacrima opened this issue Aug 12, 2023 · 10 comments

Comments

@NikkiLacrima
Copy link
Contributor

NikkiLacrima commented Aug 12, 2023

What version of OpenCollar are you using?
8.2.3

What behavior did you expect?
(Previous) Owner should not be able to use Access menu after runaway

What behavior did you see instead?
Owner can add themselves in + Owner just after runaway if owner has an open menu on the collar before runaway, and the runaway is done from commandline

What steps does someone need to take to reproduce the problem?

  1. Owner opens main menu and keeps it open, or goes into Access submenu
  2. Wearer does prefix runaway from command line, answering Yes to confirmation dialog
  3. After runaway is complete the owner uses the previously opened menu to add himself as owner again

This does not work if wearer uses the collar menu to runaway.

This also works when runaway is done from menu and PR #963 is applied

My understanding of what is happening:

The reason is that the open access menu, started from oc_core, is not invalidated by runaway sequence or the runaway confirmation dialog, started from oc_api. So the previous owner has owner authority and an active menu and can add himself again.

If wearer runs away using the main and access menu then the owners open dialog is invalidated, due to the g_iMenuStride not initialized that makes it impossible for more than one active menu on the same script.

PR #963 fixes the g_iMenuStride issue by removing that code and letting oc_dialog keep track of open dialogs, and this allows the owners access menu on the collar to stay valid even if the wearer uses the menu to run away.

@SilkieSabra
Copy link
Contributor

So what you're saying is that PR #963 will actually add to the bug where an owner with an already open menu can still use owner level access until the menu times out; by making it possible to do this from the menu as well as the commandline?

@NikkiLacrima
Copy link
Contributor Author

Yes,
doesn't change the underlying issue, but exposes it more, the same happens if we fix the error with the uninitialized g_iMenuStride variable, in oc_core line 373 declaring
integer g_iMenuStride = 3;

@SilkieSabra
Copy link
Contributor

SilkieSabra commented Aug 12, 2023

I know it has been the case as long as I can remember that blocking someone or changing access did not prevent them from using their access if they had a menu still open. I associated it with a similar issue for group moderators where ejecting or banning someone from a group did not stop them continuing to chat as long as the group chat was still open. I don't know if that is still the case, haven't tested it in a couple years now. Is there a reasonable way to fix this since the current pr doesn't? We could decide to merge anyway for the benefits of increased efficiency and then look at this problem in a separate pull.

@NikkiLacrima
Copy link
Contributor Author

I am pondering ways to fix the root issue and yes that will be a separate pull. Some way to force all open menu's to be reauthenticated.

Still in favor of merging #963.

@tayaphidoux
Copy link
Contributor

tayaphidoux commented Aug 12, 2023

runaway could force a menu clean of any users except the wearer, this becomes even more possible if the menulist is stored in linkset, other wise you just need to act on the runaway command.

i have also made alot of important changes to the ao which this is based from, ones that would definitely benefit a stand alone template, which is what this script is supposed to be an example of how to do a stand alone that can work as an addon.

@Yosty7B3
Copy link
Collaborator

It's not a solution for right now, but in 9.x we can use the linksetdata to check auth right before calling dialog.
If done within oc_dialog, then only 1 script needs to be changed and will make all dialog calls more secure.

@NikkiLacrima
Copy link
Contributor Author

NikkiLacrima commented Aug 13, 2023

Its worth considering for sure but authority should be checked before even calling into oc_dialog, since oc_dialog does not have any idea of what autority should be, some dialogs are intended only for owners, some are for recipients with out any rights.

The problem is when authority state changes after dialog has been called and before it has been responded to. I am working on checking the changes to "auth_owner" in oc_dialog and invalidate any open dialogs for owners that are removed from the owner list. PR #980

@Medea-Destiny
Copy link
Collaborator

This is an inherent problem in providing auth as a pre-calculated value in the dialog protocol. Auth is calculated before a dialog is requested, and the oc_dialog script returns the response to a dialog with the auth value that had been stored when the dialog was generated. As a short term fix we could send a linked message with AUTH_STALE whenever any auth lists are changed, and then have oc_dialog run through g_lMenus to alert each user that menus have been cancelled due to auth change and simply delete g_lMenus when AUTH_STALE is received.

Longer term, as @Yosty7B3 says, 9.x offers the possibility of locally authing, and this means that the receiving script can auth even after it has received a dialog response.

@tayaphidoux
Copy link
Contributor

@Medea-Destiny that is exactly what i was getting at but better explained.

@NikkiLacrima
Copy link
Contributor Author

Owner menu's inactivated on runaway by PR #980

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

No branches or pull requests

5 participants