Make sure DDP._CurrentInvocation is never set inside publish functions

[mitar]

_setUserId is normally called from a Meteor method with DDP._CurrentInvocation set. But DDP._CurrentInvocation is not expected to be set inside a publish function, so we have to temporary unset it.

Otherwise, this can lead to a problem that if a publish function is calling server-side Meteor methods, their this.connection property is set when publish function is restarted here, but the property should not be set.

[mitar]

I added tests.

[abernix]

Is this just to make sure the expectation that this.connection is not set on a server-initiated Method call is met (i.e. that a server-initiated call has this.connection === null)? Or is some inaccurate connection data leaking into the server method from the publication?

I'm just a bit confused as I was under the impression that a server-method called from another server-method would maintain the this.connection of the client which called the first (entry) server method. I guess I don't see why not having this.connection in the method called from the publish would be a problem.

Edit: finished sentence.

[mitar]

Is this just to make sure the expectation that this.connection is not set on a server-initiated Method call is met (i.e. that a server-initiated call has this.connection === null)?

This.

I guess I don't see why not having this.connection in the method called from the publish would be a problem.

Because calling a method from a publish function is a server-initiated method call. And at least it should be consistent. But to be backwards compatible, we probably cannot change this (like, it would break a lot of my code, this is also how I discovered, because things are throwing an error every time a restart happens, because connection was not expected to exist, because this was to determine which arguments a method needs based on where is it called from). So we should just make it consistent.

[abernix]

Ok, got it. Not saying that I disagree, but the wording on the this.connection docs makes this sound intentional:

The connection that this method was received on. null if the method is not associated with a connection, eg. a server initiated method call. Calls to methods made from a server method which was in turn initiated from the client share the same connection.

(emphasis mine)

[mitar]

I agree. But calling a server method from publish function should always have connection null, no? Do you agree with that?

[abernix]

I don't see why that this.connection shouldn't be passed to the server method if it's known – specifically if it is a descendent of something which did have it. A server method would have a this.connection if it was called directly from the client, so why shouldn't it have the context if it's triggered via a publish? When the publish is restarted, it still has a connection, no?

Are you trying to solve a problem? Is this because of trying to detect where a server method was called from?

[mitar]

I don't see why that this.connection shouldn't be passed to the server method if it's known

I do not really care either way, but we should be consistent. Currently, methods called from a publish function do not have this.connection set, except in a rare case when publish function is restarted because of the login.

I think it is important that this is consistent also because of the security. I think how most people use this these days is (including me) to check if this.connection exist and if it does, do permission check, and if it does not, allow the call without the check knowing that it is coming from a trusted source. So changing this mike break some code as it would not be backwards compatible.

So this pull request now just handle the inconsistency while keeping it how it was before and backwards compatible. If we want to change that methods called from a publish function keep this.connection, this is fine by me as well, but it should be consistent. :-) And you should be sure it will be merged if we change it this way.

[abernix]

I do not really care either way, but we should be consistent. Currently, methods called from a publish function do not have this.connection set, except in a rare case when publish function is restarted because of the login.

Ok, now this is starting to become clear to me. I still didn't quite understand that this was the inconsistency. You're saying the first time a publish is run this.connection is not set still when performing a Meteor.call to a server method from that publish function. But when it is re-run then suddenly this.connection is set. That is definitely confusing.

I think it is important that this is consistent also because of the security. I think how most people use this these days is (including me) to check if this.connection exist and if it does, do permission check, and if it does not, allow the call without the check knowing that it is coming from a trusted source.

Agree! I do this too (though I don't do method calls from my publications).

So this pull request now just handle the inconsistency while keeping it how it was before and backwards compatible. If we want to change that methods called from a publish function keep this.connection, this is fine by me as well, but it should be consistent. :-)

I think I understand now. 🙂 What is the this.connection currently if you call a server method (from the client) which then calls another server method. Is this.connection set on the second method?

[mitar]

What is the this.connection currently if you call a server method (from the client) which then calls another server method. Is this.connection set on the second method?

That should not be changed at all. As you see, I changed only behavior inside the _setUserId, which restarts all the publish functions and this is where this.connection leaks to the publish functions.

[abernix]

I understand. I was asking what happens right now in that scenario. I honestly don't know as I don't do this anywhere. It seems to me that it would make sense to match that behavior.

[mitar]

I think we should not over-engineer this. This now is just a fix of an inconsistency. I think this is what is a bug. If we want to change how all this works, this should be a larger discussion. So this here is to fix a real bug which happens in production. Changing how this.connection propagates is to me currently unclear if it is really necessary. Maybe we should wait for somebody to open an issue first about that before we start changing things. At least for me I do not need to have that change. What I need is to have this inconsistency fixed.

[abernix]

There is no over-engineering happening here. I'm asking a question: What happens right now when a Meteor Method calls another Meteor Method when the first was called from the client?

I ask because I think it changes the proper solution for this. You are trying to fix an inconsistency and there has to be a "correct" way: Either the nested method always has this.connection, or it never has this.connection. Since, as we've already discussed, this could be a breaking change, it would make sense to figure the correct method so it's not a breaking change a second time, right?

So to help answer this question, I'm asking what happens in the case of a "Method => Method". It seems the answer should be the same as "Publish => Method". That's my belief, at least.

[mitar]

What happens right now when a Meteor Method calls another Meteor Method when the first was called from the client?

this.connection is propagated. We discussed this above.

So to help answer this question, I'm asking what happens in the case of a "Method => Method". It seems the answer should be the same as "Publish => Method". That's my belief, at least.

I thought that we discussed this already in this thread and I think we are going into circles. In "Method => Method" this.connection is kept. In "Publish => Method" it is not set, because publish functions seems to be more of a server-side than a client method call. The only exception is when publish function is restarted, then it is set. And this pull request is fixing that.

And what I am saying is that changing so that publish functions have this.connection set in methods which are called is a backwards incompatible change and is a broader question if we want to change that. I have not seen any request to change that and I am asking why then changing that?

[mitar]

Also, this fixes a problem in behavior nobody is relaying on (that this.connection is available after publish function restart). So I think this should be merged in. Then, if somebody wants to make a pull request changing how this.connection is available in publish function, they are free to do it. I think that it might be tricker to implement that because it is changing existing things. So my question is: who will do it. This pull request is open to editing, so feel free to add to it, if you want to tackle it. I just want consistency. You want a change in semantics + consistency. So add a change if you want.

[abernix]

this.connection is propagated. We discussed this above.
I thought that we discussed this already in this thread

If there is a misunderstanding here, I digress and apologize, but this is the first time you directly answered. It was not clear to me. I started off saying I was under the impression (that is to say, not certain) and referenced the documentation. I asked two more times, to both you replied with something that still didn't seem like the answer. If someone asks the same question a third time, it is reasonable that something is still not clear. If you think you answered, I didn't understand.

Also, this fixes a problem in behavior nobody is relaying on (that this.connection is available after publish function restart).

By making this statement aren't you making the assumption that publication restarts are rare? I don't know if that's the case. Publications might get restarted frequently or as many times as they are started in many applications. I would need data from users to make this conclusion.

I think either way, there is a breaking change for someone.

• You're proposing that client => publish => method should not have this.connection set.
• I'm proposing that maybe client => publish => method should have this.connection set.
  • This is consistent with client => method => method.
  • From a security standpoint, if developers use trusted = this.connection === null, wouldn't it be better to suddenly start defining this.connection (more permissive) than to stop defining it (less permissive)?

So my question is: who will do it. This pull request is open to editing, so feel free to add to it, if you want to tackle it. I just want consistency. You want a change in semantics + consistency. So add a change if you want.

My contribution on this issue is a discussion and my opinion. I'm not going to tackle it or change your PR. I want consistency too.

I'm happy to discuss further, but I think yes, we're going in circles now. Someone else will probably have to weigh in on this.

[mitar]

By making this statement aren't you making the assumption that publication restarts are rare? I don't know if that's the case. Publications might get restarted frequently or as many times as they are started in many applications. I would need data from users to make this conclusion.

Publications restarts because of the login while you are subscribed is definitely rarer than "as many times as they are started in many applications". In any case, I am not claiming or assuming anything about how often restart happens. I am saying that it seems that nobody until now cared to investigate or report why server-side method which uses information from this.connection and is called from inside the publish function, has this.connection set when logging in. You do agree it is rare that somebody is calling a server side method from inside a publish function? And I am saying that it is even rarer that that method happens to inspect this.connection, and even rarer that it would care for a behavior that this.connection is set during a login. I mean, only if somebody discovered this bug and made a workaround. But even then they should probably also report it. I have not seen one.

You're proposing that client => publish => method should not have this.connection set.

I am proposing that we should not change behavior, because this is how it is currently.

I'm proposing that maybe client => publish => method should have this.connection set.

You are proposing to make a backwards incompatible change, so I am asking why?

What I am reporting is a bug in current behavior and a fix for it. What you are proposing is a backwards incompatible change and a feature change.

What I am saying is that we should merge a bug fix. And then if somebody wants to tackle a feature change they can open a ticket, pull request, or whatever. I do not need that feature, but I need the bug fixed. If you do not need that feature either, then let's maybe just agree on fixing the bug?

[mitar]

Can we get some additional eyes on this?

[mjmasn]

As of this week I quite like having access to DDP._CurrentInvocation inside a publication... Just to throw that out there :) [See my comments at #5462 (comment)]

That said, I'm having to run a modified livedata_server.js (in ddp-server package) either way, so maybe my opinion doesn't matter here.

[abernix]

@mitar I've re-read this thread in its entirety and my feelings haven't changed: providing more context, in the form of DDP._CurrentInvocation (my proposal) versus less context (your proposal) still seems like the more secure route to go if a fix is to be provided.

I still don't accept your argument that my change is "breaking" and yours isn't – something you have stated multiple times now. Your proposal breaks backwards compatibility for some and mine breaks it for others (that's why this is identified above as a "consistency" issue). If someone is relying the this.connection to be there on a re-run of a publication and we take it away with this PR, then it's broken for them and based on our discussion above, this would be giving privileged access to a "trusted" source. If someone is not expecting this.connection to be there on a re-run of the publication and we add it, it's also broken for them, but this won't yield the result of privileged access (i.e. safer).

I'm inclined to believe that this isn't a popular problem given the lack of overall input and lack of reported issues on this but I think this is still important to merge – edge cases like this can be nasty to debug. The fix seems like it should take the more secure route and match the documentation which specifically states that this.connection should be passed along.

[mitar]

I still don't accept your argument that my change is "breaking" and yours isn't – something you have stated multiple times now. Your proposal breaks backwards compatibility for some and mine breaks it for others (that's why this is identified above as a "consistency" issue).

There is no way that anyone is relaying on this fact that in rerun connection is available. I spent a day debugging this until I realized what is happening. This is not documented and not intuitive at all (that something would be available only in such edge case).

I am saying that you are discussing hypothetical situation: somebody relaying on this edge case, while I am explaining you the real situation: me having issues because of this edge case. You have not convinced me that there is anyone who is relaying on this edge case.

this would be giving privileged access to a "trusted" source.

I do not see how this would happen. Why would you secure privileged access only during publish rerun, but not during normal run?

I'm inclined to believe that this isn't a popular problem given the lack of overall input and lack of reported issues on this but I think this is still important to merge – edge cases like this can be nasty to debug.

Exactly, this is really tricky to debug.

The fix seems like it should take the more secure route

Feel free to implement it differently. I do not know how. I know how to fix this. But I do not know how to assure that this is always passed along correctly, without connection leaking from one publish function to another by accident and so on (there is some multiplexing happening). There are many things to consider and it seems much harder to implement. I think this is simpler fix to do, and available. But if you feel strongly that it should be different than I invite you to implement it differently. Maybe I am mistaken that it is hard to implement.

match the documentation which specifically states that this.connection should be passed along.

Documentation is not saying anything that the server-side initiated method from the publish function should share the initial connection data.

[hwillson]

@mitar @abernix I think at this point the only way to settle this is with a friendly game of rock-paper-scissors, since I think you're both right! (oh, and I'm Canadian so I have to say this to help avoid conflict 😄)

First a quick recap:

• @mitar has uncovered a production bug that not many people have noticed, as calling server side Method's from publish functions is not a common practice. His supplied fix addresses this and seems reasonable in the short term.

• @abernix has brought up a really good point about how this functionality should be addressed further, to make sure this.connection handling functions the same when calling client -> publish -> method and client -> method -> method, which it does not currently.

Given the above, and to move out of stalemate, maybe we should consider addressing this in 2 steps then? Step 1 is to fix the immediate bug as outlined in this PR (since it's a fairly quick fix and easy win). Step 2 is to open a new issue to track the issue mentioned by @abernix, which when addressed will ultimately be the better fix (but will take more time to implement), and can replace the fix outlined in this PR. Would that work?

Otherwise, if we're looking for the best possible solution to this being implemented once and only once, then addressing the issues brought up by @abernix makes sense, before merging this PR.

Anyways, just adding some 3rd party feedback. Thanks!

[abernix]

this would be giving privileged access to a "trusted" source.

I do not see how this would happen.

Lack of this.connnection provides the assumption that it's a server call and gets full access. You are suggesting an implementation that in at least some cases, this.connection would no longer be available where it may have been before. This is the security risk. It has nothing to do with a logical decision to code a publish function that way.

There is no way that anyone is relaying on this fact that in rerun connection is available.

Of course they're not relying on it, but a difference would be observed in an edge case (whether your implementation or mine). With your change, privileges would be relaxed – in my proposal, they would be tightened. Surely you agree that if a random edge case results due to this change that it would be preferred that the publication get less access and not more?

I am saying that you are discussing hypothetical situation

Your real-situation edge case was once a hypothetical. 🙂

Documentation is not saying anything that the server-side initiated method from the publish function should share the initial connection data.

You're right. It doesn't specifically say anything about publish to method, but it does say:

Calls to methods made from a server method which was in turn initiated from the client share the same connection

I maintain the presence of this.connection in both gives a certain expectation of them to be passed along in the same fashion whether client => publish => method, client => method => method.

You have not convinced me that there is anyone who is relaying on this edge case.

You are seemingly the only person that is having this problem. I think the problem should be fixed securely and you can adjust your code accordingly.

[abernix]

@hwillson My core issue is that the above fix seems to relax security in an edge case and therefore I cannot support it.

[mitar]

@hwillson My core issue is that the above fix seems to relax security in an edge case and therefore I cannot support it.

But nobody is relaying on a security in edge case, if the main case does not work? How is this possible?

[hwillson]

@abernix Right, good point; I really like the idea of client -> method -> method and client -> publish -> method behaving the same anyways (which is definitely the bigger issue to address).

[mjmasn]

@mitar if we take @abernix's approach, would your code still work (with some modification presumably) or would it be literally unfixable?

If it's possible to make client => publish => method match client => method => method and it's not going to totally break things for you I think that's the better and more secure option. Probably nobody is relying on the edge-case as you say, but it's still better to increase security (or at least reduce the possibility of an edge-case security mishap) if possible.

Plus as I mentioned before I'm relying on DDP._CurrentInvocation always being present so I can do audit logging of all method and publication calls. From what I understand this would remove that ability from publish functions?

And finally this may be a dumb question but I'm not even sure why a Meteor method would need to be called from another method or publish function vs just calling a plain JS function? Happy to be enlightened though!

[mitar]

Given the above, and to move out of stalemate, maybe we should consider addressing this in 2 steps then?

I do like this proposal. I would like that this is fixed (I am getting errors regularly because of this). And then whoever wants to change to so that connection is available always, can do that. With a good notice in changelog.

From what I understand this would remove that ability from publish functions?

Publish functions already do not have this set, only during a rare moment when they are rerun after a login of the user (when publish functions are restarted).

If you need something like DDP._CurrentInvocation always, you can do a similar trick to what I am doing in this package.

And finally this may be a dumb question but I'm not even sure why a Meteor method would need to be called from another method or publish function vs just calling a plain JS function?

Just because they were defined in this way. Otherwise I would have to define them as a symbol and export it or something. But if they are already registered as a method, I can just call it. It is just a question how you want to organize your code.

Of course they're not relying on it, but a difference would be observed in an edge case (whether your implementation or mine).

No, they are already relaxed in main case as well. Currently, if you call a method from publish function, you do not have access to connection. So you had to do security in some other way. Otherwise your method already had full access. Only in the edge case you gain access to connection.

So you are saying that there is somebody who is calling a server method from publish function and do not care about its being trusted (because there is no connection), but then during rerun now they start worrying because they do not know that if it is trusted or not? Really strange logic you have here.

Please write me a publish function example which would work like this. So on purpose make an example function which fails in the way you are saying security would fail if we merge this pull request. I would really like to see how is this possible.

[mitar]

So, what is the next step here? Who will implement an alternative? Or will we keep a buggy version around?

[mitar]

I made publish-context package. I was not really thinking about this pull request when making it, but now I think this is the correct way to do it. When called from inside Meteor publish function, DDP._CurrentInvocation should never be set. Instead, DDP._CurrentPublish should be set always when called from inside Meteor publish function.

Then a simple function like this:

function userId() {
  const currentInvocation = DDP._CurrentInvocation.get();
  if (currentInvocation) return currentInvocation.userId;

  const currentContext = DDP._CurrentPublish.get();
  if (currentContext) return currentContext.userId;

  throw new Error("userId() not invoked from a method or publish function.");
}

Gives you user ID if you are inside meteor method or publish function, if you want your ACL checks to be the same across all codebase (and even inside nested functions). This is what this package does.

Similarly, you can get access to connection. But, it also opens new paths to the code which needs to know if it is inside publish or inside method. For example, you can have a nested function which then calls DDP._CurrentPublish.get().added(collection, id, fields) to publish a document. And you do not have to pass around publish handle.

So, I again think that the correct thing is to merge this pull request as it is. Publish function is not method invocation. But we could move into the core DDP._CurrentPublish which is set in all publish functions. It is literally 5 lines of code.

BTW, I think also that user-extra package should be moved to core. And then it would be really easy to make good ACL checks around the code, in any code called from methods or publish functions. In a backwards compatible way.

[zimme]

From what I understand of this issue the problem is that when a method is called from a publication, when a user is not logged in, the method won't inherit the connection from the publication.

Then when the same publication is re-run because a user logged in, the method now inherit the connection from the publication.

This does not reflect the behaviour of when a method is called from a method on the server. In this case no matter if a user is logged in or not, the method called from another method that was called from the client will always inherit the connection.

I think the "best" and I believe the most secure way of solving this issue is to make sure that methods called from publications inherits the connection in the case when the publication was called from the client when a user was not logged in.

This would probably have to be properly documented as it might "break" some people's applications that are doing method security based on the currently "broken" behaviour.

The publish-context package is a nice workaround, but we shouldn't have to add another api on DDP when we should be able to solve the problem in Meteor core.

If we can come to a decision on this issue I would try and create a PR to solve this.

[mitar]

I think we should really simply introduce public context ddp variable. Because then when you cal any function, one will be able to get to the current publish context. I think we should make DDP._CurrentInvocation a public API.

I think this is much better approach than giving access to this global variable only inside methods.

[zimme]

If we fix DDP._CurrentInvocation to also always be set on publications then I don't see a need for a publish context as you will always be able to use DDP._CurrentInvocation?

[mitar]

So DDP._CurrentInvocation will be set already inside main call to the publish function, not just when calling a server method from publish function? I think this might be OK, but might be a bigger breaking change. I think many packages in fact use DDP._CurrentInvocation and assume things about it. Maybe we can break the change because we declare DDP._CurrentInvocation as private.

This is why I went for a new variable.

Also, my DDP._CurrentPublish gives you access to the whole context of a publish function (including access to added, changed, removed and so on). I think having DDP._CurrentInvocation once be method context (one with setUserId and so on), and once publish context might be confusing. Also some code might want to access setUserId inside a method call, but it would not be available because it would be called from a publish function? Or would you override it and set it to a method context? Then you would lose access to publish function context and some other code might not have access to it when needed, just because it happens a meteor method call is in-between.

I think it would be simply cleaner to have two variables with clear semantics, and make them public and documented.

[zimme]

From what I understand, having DDP._CurrentInvocation set when running a publish function doesn't not affect the context (this) of the publish function.

When a publish function is run when a user is logged in, then DDP._CurrentInvocation is set and the context of the publish function is still what one would expect.

Maybe I misunderstood the point you were trying to make and I'm missing something?

[mitar]

What I am saying is that context (this) inside a Meteor method is different (has special methods/fields useful in a particular context) from context (this) inside a Meter publish. If we use one variable for both, then code will not be able always to access one or the other, depending on a strange fact from where it was called.

Example, if we set DDP._CurrentInvocation to publish context when first a publish function was called, and then that publish function calls a Meteor method. That Meteor method might call another function which expects that publish context is of a method, and not of a publish function. It might call setUserId on that context. And that will fail. And it is hard to debug this.

So I think it is much better that we expose this as it is, do not try to be too smart about it. Make one variable always expose the publish context, if it is, and another always expose the method context, if it is.

We could make it so that a context is both, but then callers would have to expect that, and this might be a breaking change. Maybe we could have also both things: two variables, and then a new helper function (public one) which returns a current context from one or the other.

[zimme]

Ok, so I believe that might be a different issue all together.

What I'm suggesting isn't going to change the context of either publications or methods it will only make DDP._CurrentInvocation available in publications even when there's no user logged in. DDP._CurrentInvocation is already available in publications but only when a user is logged in.

[mitar]

it will only make DDP._CurrentInvocation available in publications even when there's no user logged in.

But what will be the value of DDP._CurrentInvocation? Context of a publish function or context of some method? If it is of publish function, then my comment above applies and makes it an issue for every developer who expects it to be a method context. If it is of a method (which method?) then you loose access to fancy methods of publish context.

DDP._CurrentInvocation is already available in publications but only when a user is logged in.

No, DDP._CurrentInvocation is available in a publication only when a user is logging in (so when one method calls setUserId and all publish functions are rerun). Not when they are logged in (later on, when publish function is run and user is already logged in, DDP._CurrentInvocation is not set). To be precise.

And this is why this pull request exists. Because I think it is bad that DDP._CurrentInvocation is set only when logging in, but not when a publish function is called when user is not logged in or is logged in (large large majority of the time). It is set in one very edge case which just breaks the code, and I do not think anybody is really expecting that DDP._CurrentInvocation will magically be set when user is logging in.

[mitar]

I think we should make DDP._CurrentInvocation not be set while user is logging in (merge this pull request) and then expose a new variable exposing a full publish context, whenever it is available. Simple, clean, predictable, and backwards compatible API.

[zimme]

You are actually correct. I just tested this and it's only when the publications are re-run because a user logged in, but not in following subscribe calls to the publications.

Maybe DDP._CurrentMethodInvocation and DDP._CurrentPublishInvocation would be a good idea to be more explicit.

[zimme]

Ok so I've look at this once more now.

If we just accept this patch we will fix the current inconsistency of a method being called from a publication having this.connection only in the case when the publication is re-run because a user just logged in.

In all other cases a method called from a publication will not have this.connection, this is because DDP._CurrentInvocation is never set on a publication, except in the specific case of a publication being re-run because a user logged in.

I'm totally fine with this and I don't see this as being "loose" on security as all of a sudden this.connection isn't available any more as @abernix suggested. It's rather the opposite in this case.

If you have a method that only run whenever this.connection === null then I believe there's a lot of security flaws in apps today as users might believe that this.connection will be present in methods called from publications which is not the case today. It's only true in one scenario and that is whenever a publication re-ran because a user just logged in and in this case the method would not run it's code because the check this.connection === null would not be true all of a sudden.

So in this issue @mitar is and always have been correct as far as I can tell.

However this issue have revealed a different issue all together and this is where I believe the confusion has arisen.

As a developer I would expect that methods called from publications would inherit the connection. i.e. a method called with Meteor.call('methodName'); from inside a publication would have the same this.connection as the publication from which it was called.

Today this is not the case because in all the normal cases a publication don't have DDP._CurrentInvocation set and because of this a method won't inherit the connection from the publication, except in the case when a user just logged in because of the previous issue.

One solution to this problem is to make sure that publications always have DDP._CurrentInvocation set to the equivalent to that of a method, this way methods would always be able to inherit the connection from publications not just in the edge-case discussed.

@mitar however brought up a case for having a separate publish context set on publications which will be accessible via DDP._CurrentPublish as he has done in a separate package.

From what I can tell though, this will not directly solve the problem of methods inheriting the connection from publications but if we were to make this available we could just update the server side apply function also try and inherit connection from DDP._CurrentPublish when DDP._CurrentInvocation is isn't set.

If we go down this route I would suggest that we introduce DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation(DDP._CurrentPublishInvocation) and maybe even remove the underscore to mark the API as public.

Sorry for the wall of text but I just wanted to try and clear this issue up as I would really like to see a fix for these 2 issues.

[mitar]

Yes. I think this might get be a consensus we can all agree on. It will also satisfy points brought by @abernix.

I would then suggest that we:

• merge this pull request to get rid of leaking of method context when user is logging in
• introduce a new DDP variable which is set to the current publish context
• when a server-side method is called we populate this.connection (and related values) based on the current publish context, if it is available
• DDP._CurrentInvocation is never set for a server-side method called from a publish context, so methods can know from where they are called (and also use their whole context); the existing public interfaces like this.connection do not expose from where they are called; this is also why it is important to merge this pull request, so that this separation between methods called from another method or from publish context is clear and does not get confused on user logging in

In this way the behavior which is arguably useful as pointed out by @abernix, having this.connection available in method called from the publish function will be there. Only the mechanism of how it gets there will be different. Or probably this would be the way it would have to be implemented anyway, because I do not know how otherwise we could populate this.connection. So maybe what we get to describe now is how to implement what @abernix proposed.

[abernix]

Closing in favor of #8629 which is a more complete solution for this issue and already contains all of the commits for this thanks to it being rebased on this. Will be great to see this finally land and agreeable to all parties!