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

Session meta procedure to forcibly disconnect a session #314

Closed
gammazero opened this issue Jun 19, 2018 · 26 comments
Closed

Session meta procedure to forcibly disconnect a session #314

gammazero opened this issue Jun 19, 2018 · 26 comments

Comments

@gammazero
Copy link
Contributor

Since there have been requests for such a feature I wanted to propose this as a part of the WAMP specification. This is already supported by Crossbar: https://crossbar.io/docs/Session-Metaevents-and-Procedures/#killing-a-session, and seems like something that should have been proposed. I could not find mention of such a feature in any issue, so I am opening an issue.

The proposal is for a session meta procedure, wamp.session.kill. It takes one positional argument, session|ID, two optional keyword arguments, reason and message. The reason argument is the URI used as the GOODBYE.Reason. The message argument is put into Goodbye.Details under the key "message". If the session does not exist, an error is returned.

@ecorm
Copy link
Contributor

ecorm commented Jun 19, 2018

There should also be a wamp.session.kill_all meta procedure for closing all of the sessions associated with an authid, as I proposed in crossbario/crossbar#1340. Two use cases in our application motivated the need for this:

  • The application admin removing a user account at runtime.
  • The application admin suspending (deactivating) a user account at runtime.

We are currently resorting to our application backend keeping track of all frontend sessions and associating each of the active sessions with the authid. Our backend must then call Crossbar's wamp.session.kill meta procedure in a loop for each active session associated with the authid.

@gammazero
Copy link
Contributor Author

I am currently planning on implementing both of these as stated in gammazero/nexus#112. So, it sounds like these are both reasonable extensions of the WAMP session meta api. If there is consensus on this, I will draft the spec documentation of these.

@oberstet
Copy link
Member

thinking about, since the main WAMP session related information is:

  • session_id
  • authid
  • authrole
  • realm

we could go for 4 procedures:

  • wamp.session.kill: kill a single specific session given a WAMP session ID
  • wamp.session.kill_by_authid: kill all currently connected sessions with given WAMP authid
  • wamp.session.kill_by_authrole: kill all currently connected sessions with given WAMP authrole
  • wamp.session.kill_by_realm: kill all currently connected sessions for the given WAMP realm

@gammazero
Copy link
Contributor Author

I have given it some thought, and this seems like a solid plan. It is simple, and is addresses all the identified use cases. It has my vote!

@ecorm
Copy link
Contributor

ecorm commented Jun 19, 2018

we could go for 4 procedures: wamp.session.kill, wamp.session.kill_by_authid, wamp.session.kill_by_authrole, wamp.session.kill_by_realm

From an API perspective, would it be a better idea to fold these four separate RPCs into a single one that takes a dictionary as a parameter? This dictionary would be something like:

{
    "sessions": [1934839483943, 304034303],
    "authids": ["alice", "bob"],
    "authroles": [],
    "realms": ["Mordor"]
}

Then a single wamp.session.kill meta-procedure can kill all sessions that are covered by the dictionary. Implementations would likely want to kill applicable sessions in the order of realms, authroles, authids, and finally sessions.

For backward compatibility, the wamp.session.kill meta-procedure could handle a single session id in the place of the proposed dictionary.

@oberstet
Copy link
Member

Collapsing all 4 into 1 procedure removes the ability to control authorization based on simple URI matching in a router, and rgd semantics: these needed to be "OR" (as providing a session ID already determines everything else). But with "OR" semantics, tbh I don't see that big of an advantage: I can kick 2 specific session IDs, all session of 2 authids, and all session of 1 realm in one call instead of 5 - where the latter can be pipelined (I can fire off all 5 calls at once, and then wait on the full set to resolve - sure, couple of more bytes). Mmmh. This seems like a very specific situation. I wouldn't collapse them ..

@oberstet
Copy link
Member

there is also a 2nd issue with collapsing IMO: if the procedure is under the current URI wamp.session.kill, it'll make the procedure polymorphic in the parameters, as we have to support the existing argument types expected. if we add the new procedure under new names, this isn't a problem obviously.

@johannwagner
Copy link

Just my two cents:

All router implementations, including nexus, currently handle the authrole as string or list of string, which also makes the one endpoint way too complex. I think, this is currently not included in the standard, but widely used.
Additionally, there is no possiblity to difference between and and or, which @oberstet mentioned earlier.

Best Regards,
Johann

// cc: @martin31821

@KSDaemon
Copy link
Collaborator

And another two cents:
At first glance having an only one RPC with different params looks clearer, but in this case there are a number of problems already mentioned above. So i vote for a different URIs for different cases.
Just one remark: if we have 3 RPS called wamp.session.kill_by_*, why not to name last one wamp.session.kill_by_sessionid instead of wamp.session.kill? What do you think?

@johannwagner
Copy link

@KSDaemon There is an existing API from crossbar.io, so we could change that, but we don't have to.

@KSDaemon
Copy link
Collaborator

Ough! I didn't take that into account. Thanks, @johannwagner!

@oberstet
Copy link
Member

Yeah, if we would start from scratch, kill_by_sessionid would be more systematic naming, but since we have an existing endpoint to support, I think it's not worth the hassles that would result if we'd break that existing API. Of course we could add an alias - but then .. is it worth the potential confusion? Keeping the existing kill seems "ok" from the slight naming inconsistency, while not breaking anything ..

@johannwagner
Copy link

What would be the next steps to get this thing rolling?

@oberstet
Copy link
Member

Ideally a PR against this repo, adding a spec text proposal describing the new procedures ..

@gammazero
Copy link
Contributor Author

I am perfectly happy with wamp.session.kill, even without any existing implementation, since the implicit default identifier for a session is the session ID.

Also, I will be happy to draft the spec text and submit a PR in the next couple days, unless someone cannot wait that long (I know how exciting it must be).

@gammazero
Copy link
Contributor Author

Actually, I do have one question about wamp.session.kill_by_realm: If a client is only authenticated/authorized within a single realm, can it have authority to terminate sessions in other realms? Would this be better changed to wamp.session.kill_all, meaning kill all sessions (including this one) in the current realm?

@KSDaemon
Copy link
Collaborator

Good point! Agree with @gammazero.

@oberstet
Copy link
Member

oberstet commented Jun 21, 2018

@gammazero yeah, you are right: it should be wamp.session.kill_all() - it should not take a realm parameter, since the meta API is exposed on a specific realm (and cannot access arbitrary other realms).

killing sessions in an arbitrary realm running on a router is not possible with the WAMP meta API, which is designed to be attached to or expose exactly one realm ..

@oberstet
Copy link
Member

rgd kill_all: what happens with the caller? I mean: does the caller even get a result - as the calling session will be killed as well. Or do we require the router to first return with success, and then kill that last session (the caller of kill_all)?

@KSDaemon
Copy link
Collaborator

KSDaemon commented Jun 21, 2018

May be we can even add details attribute to kill_all: exclude_me:boolean|default false ?
If kill_all is going to kill even an initiator session, i think we can return success and then close session.

@gammazero
Copy link
Contributor Author

gammazero commented Jun 21, 2018

kill_all should kill all sessions except the initiator. The initiator can then then close itself, or perform some additional (implementation specific) actions that it needs to do in isolation. No need for an additional parameter.

I can see this being useful when the initiator is also providing remote authentication, and wants to force all clients to re-authenticate. The initiator can update its authentication logic/keys/etc. and then use kill_all to remove all clients and force them to rejoin and re-authenticate. Also, it could be useful when asking the router to perform any number of implementation-specific admin functions. The WAMP spec only needs to cover the kill_all meta procedure. A router implementation can then provide any functionality such as lock ream, do admin activity XYZ, unlock realm.

So conclusion: wamp.session.kill_all requires only the same two optional keyword arguments as the other wamp.session.kill... meta procedures: reason and message. Anything else is implementation specific. Calling the kill_all meta procedure will remove all other sessions, but leave the initiating session, which can close when it needs to.

@martin31821
Copy link
Contributor

martin31821 commented Jun 21, 2018

IMHO kill_all should kill all sessions except the initiator. This leads me to another interesting point: what should kill do when it is invoked with the session id of the caller? Should the caller get a result there or should that case be forbidden?

@gammazero
Copy link
Contributor Author

I cannot think of a suicidal use case, so if that happens it is probably an error. However, in the interest of not breaking any compatibility with Crossbar's current implementation, I think it reasonable to do whatever Crossbar currently does.

@oberstet
Copy link
Member

oberstet commented Jun 21, 2018

FWIW, I'm not sure how crossbar behaves in the suicide case, but I agree: it should raise an error (and do nothing). And I also agree: kill_all should kill all sessions but the calling one. In fact, the calling one should never be killed using any of the procedures. It can simply exit if it wants to.

@gammazero
Copy link
Contributor Author

gammazero commented Jul 6, 2018

@oberstet Now that this has been merged, does this mean it should be labeled as "RFC", or are there additional things to be completed first?

The nexus router now implements these session meta procedures.

@oberstet
Copy link
Member

oberstet commented Jul 9, 2018

@gammazero awesome!! closing ..

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

6 participants