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

Add more fields when disclosing publisher or caller #101

Merged
merged 2 commits into from
May 21, 2018

Conversation

martin31821
Copy link
Contributor

This further improves the compatibility to crossbar.io by disclosing
caller_authid, caller_authrole, caller_authmethod and caller_authextra if set.

For pubsub, the same properties are exposed.

This further improves the compatibility to crossbar.io by disclosing
caller_authid, caller_authrole, caller_authmethod and caller_authextra if set.
@gammazero
Copy link
Owner

Thank you for this. However, I must request that only authid and authrole are supported.

The WAMP standardization group has reached the consensus that the supported fields will be authid and authrole. The discussion of this feature is here: wamp-proto/wamp-proto#57

Some background (from discussion): On the subject fo including these fields with publisher and caller identification, according to Tobias Oberstein (Crossbar.IO):

Crossbar.io had implemented that in the past. And we removed it again. The reason: the next thing after authid/authrole asked for was transport level details: "we need access to the WebSocket cookie in callees". CB has more information about the Caller than just session ID, authid and authrole. Where do we stop? It doesn't make sense to transmit all that information in each and every call.

Also from Oberstein:

This is how it is implemented in Crossbar.io now:

Both publisher and caller disclosure can be enabled on a per URI basis in the router configuration
The detail attributes in EVENT messages are: publisher (the WAMP session ID of the publisher), >publisher_authid and publisher_authrole
The detail attributes in the INVOCATION messages are: caller (the WAMP session ID of the caller), caller_authid and caller_authrole
I do think this reflects the end result of our discussion of the feature. @mbonneau does it?

It isn't reflected in the specification though. And eg Thruway uses different attribute names

Related issue with autobahn-js: crossbario/autobahn-js#219
and with Thruway: voryx/Thruway#206

If we want to include additional fields, these should be optional to enable, default disabled, and the non-standard field names should begin with x_nx_ I would not want to do that as part of this PR, if at all. Implementations that include additional fields are already updated or going to be updated to only include those fields as x_... custom fields.

Conclusion: Nexus needs to include authid and authrole, for publisher and caller identification, even though this is not yet in the WAMP spec. It should not include additional fields as those will not be in compliance with WAMP-protocol.

router/broker.go Outdated
"authrole",
"authid",
"authmethod",
"authextra",
Copy link
Owner

Choose a reason for hiding this comment

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

Only features needed are "authid" and "authrole". See PR discussion.

router/dealer.go Outdated
"authrole",
"authid",
"authmethod",
"authextra",
Copy link
Owner

Choose a reason for hiding this comment

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

Only features needed are "authid" and "authrole". See PR discussion.

Copy link
Owner

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

This looks good. I am going to hold off merging for a day or two until I get some feedback from WAMP standards group on whether they want publisher_authid or just authid, and same for other fields.

@gammazero gammazero merged commit 085375d into gammazero:master May 21, 2018
@martin31821 martin31821 deleted the improve_disclose branch June 4, 2018 08:21
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.

2 participants