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

MSC1730: Mechanism for redirecting to an alternative server during login #1730

Merged
merged 9 commits into from
Dec 11, 2018

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Nov 23, 2018

@richvdh richvdh added proposal-in-review proposal A matrix spec change proposal labels Nov 23, 2018
@maxidorius
Copy link
Contributor

maxidorius commented Nov 23, 2018

The full disclosure behind this proposal is that it is used for a closed-source project, possibly paid for, and this MSC reflects that effort, not the #1097 effort. Context is here

@0xilly
Copy link

0xilly commented Nov 23, 2018

What's wrong with a closed source project trying to contribute to the ecosystem and why is it necessary for a disclosure for that?

@ara4n
Copy link
Member

ara4n commented Nov 23, 2018

Yup, the use case for this proposal happened to come from a (probably) closed-source project, although honestly afaik the licensing hasn't been decided yet. But the feature is useful for everyone, hence proposing incorporating it into the core spec, as well as providing a possible solution to #1097.

Currently the expectation on MSCs is that the proposal should stand on its own merits, rather than getting hung up on the licensing of the software which is driving the proposal. I've made a note (as part of the ongoing #1318 work) to investigate whether we should require disclosures in future, given the risk of covert commercial motives influencing the spec. Needless to say, the concern is entirely pedantic and academic in the context of this MSC...

@maxidorius
Copy link
Contributor

rather than getting hung up on the licensing of the software which is driving the proposal

It's not, as part of the context I linked to. I've summarized what are the actual issues here.

Needless to say, the concern is entirely pedantic and academic in the context of this MSC...

As per the context I gave above, it isn't. See a summary of the actual issues here.
But for the record:

The problem is not that it's useful or not, or from where it comes from. The problem is that your goal is not to address the protocol issue you picked. Your goal is to address your closed-source project issue. That means:

  1. There is an assessment the protocol issue is relevant or not, which we cannot make or evaluate ourselves as you do not disclose your project, and it might be wrong in the first place or not relevant (like right now)
  2. There are "hidden" requirements which make you discard valid input from the community if you stick to the protocol issue you picked, creating tension
  3. People are mislead into thinking they are working on solving a protocol issue, which is actually not true
  4. We help you do work you are paid for without knowing it
  5. You do not disclose from where this comes from. Right now, this is a big conflict of interest, one you did not disclose on your own

To spell it another way:

  1. The presentation of the effort mislead contributors into thinking this was to solve a specific issue of the protocol as part of Mark home_server field deprecated #1097 but instead it was to solve another problem.
  2. It is not possible to offer good and insightful feedback/review if one is not aware of the full set of requirements and just have them offer solutions that have no chance to be accepted anyway.
  3. It is dishonest to bait people interested into solving Mark home_server field deprecated #1097 to solve another problem, decide to not care for the MSC and consider it as not relevant once the real requirements are exposed, and then make them think that maybe more work will be done on it.
  4. and 5. The current situation regarding NV, The non-profit and modular.im is quite difficult as we have no idea from where things are coming, what is behind them - there is basically no transparency.

At this point 2) is the most important: Without the full requirements, it is not possible to give valid and insightful feedback and is a waste of time for everyone. The other points are just morally problematic, but that's for you to decide if you care or not. I know I've been tricked here and spent time reviewing work you get paid for that is not aimed to solving community issues but only your own.

@maxidorius
Copy link
Contributor

And you'll notice that the same pattern of "missing" requirements or the relevant end-goal of a proposal was also hidden in #1194 until I uncovered it was really for GDPR which brought the full picture, and not just what was listed in the proposal.

@ara4n
Copy link
Member

ara4n commented Nov 23, 2018

Your goal is to address your closed-source project issue

this is false; you are putting words into our mouth.

the goal of this MSC is to solve the problem of redirecting to an alternative server during login, as per the title. it is expressly trying to solve #1097.

however, it's a valid point that MSCs should be careful about disclosing the original motivation for a feature for full visibility, and we can wrap it into the MSC guidelines. however, the lack of disclosure in #1194 or #1730 is oversight rather than maliciousness, as fun as conspiracy theories are.

@maxidorius
Copy link
Contributor

maxidorius commented Nov 23, 2018

this is false; you are putting words into our mouth.

I am not. @richvdh listed two more requirements that are NOT part of the whole #1097 idea or this MSC. Again, here they are and again, once given, the conclusion was that #1730 (as trying to address #1097) was possibly not a good choice in the first place (source).

The goal of the MSC was to address a set of requirements for your portal project, not to solve #1097 on its own.
In this case #1097 is just a mean to and end, not the end. Else why would @richvdh suddenly drop this and try to solve it another way?
The way everything was presented made it look like #1097 was the end, not a simple mean that can be dropped without notice.

is oversight rather than maliciousness

Nobody ever claimed otherwise. Personally, The reason behind is not as important as the failure to give full disclosure at all levels so nobody is wasting their time and can also make an informed decision if contributing is in their best interest or not.

@0xilly
Copy link

0xilly commented Nov 23, 2018

Honestly I don't think "disclosure" is the issue or maybe it's just a poor choice of words. Maybe an explicit Motivation/Real World Usecase section to the template to explain why it's needed would solve the problem of the problem of "disclosure" but I don't see that fixing the "I thought it would solve my issue and issue Y but it doesn't" lack of full context problem.

Sent from my Galaxy S7 using FastHub

@maxidorius
Copy link
Contributor

maxidorius commented Nov 23, 2018

I don't see that fixing the "I thought it would solve my issue and issue Y but it doesn't" problem.

@illyohs Disclosure about the technical requirements that actually need to be addressed, not about the project its for (even tho it greatly helps to know what is the technical background). The proposal is perfectly fine, there is no reason to drop it, yet it was dropped because of undisclosed technical requirements.

In those two technical requirements, only one actually is. For the record, here they are (source):

  1. I would like the target HS to be able to record the right IP address for the client (is a technical requirement)
  2. Implementing HTTP proxying in my portal turns out to be a PITA (is not a technical requirement)

For the first one, I replied to use X-Forwarded-For which is the standard header in case of proxying HTTP requests. Yet, it was ignored - at this point I'm guessing because it's actually irrelevant. There are possibly other requirements that are still not given, which is directly linked to the project being closed-source or a restriction to talk about it publicly which fits the definition of "disclosure".

So to your point: Full disclosure (in this case technical) would have directly shown this MSC was inappropriate and also that it doesn't seem related to #1097 given how quick it was dropped to work on another approach altogether. It would have avoided wasting people's time to review it, or even consider it in the first place, as I have stated multiple times in my previous comments here and in #matrix-spec.

Notes on problems, workaround, and another alternative
@0xilly
Copy link

0xilly commented Nov 24, 2018

I read the discussion again and i see nothing said that that can't be applied to a open source project. I see creating a free VS closed source "disclosure" as something used to generate FUD thus "creating tension". We should let MSCs stand on technical merit alone and not what licenses people use.

Sent from my Galaxy S7 using FastHub

@richvdh
Copy link
Member Author

richvdh commented Nov 24, 2018

Please can we take the argument about our motivations for proposing spec changes elsewhere. This MSC, like any other, is intended to stand on its own merits; if it does not, or requirements are insufficiently described, that is a bug in the MSC. I have updated the MSC so that it covers all of the points I consider relevant.

@kegsay
Copy link
Member

kegsay commented Nov 25, 2018

The reason behind is not as important as the failure to give full disclosure at all levels so nobody is wasting their time and can also make an informed decision if contributing is in their best interest or not.

I know I've been tricked here and spent time reviewing work you get paid for that is not aimed to solving community issues but only your own.

I don't know why you're so upset that some closed source project is proposing new spec features. Either the spec improves by people navel-gazing about what the perfect API shape is, or it improves by people having concrete problems and proposing fixes. This seems like a case of the latter.

I don't give a damn what their project is, but it annoys me to see you crap all over a valid proposal. If you don't want to spend time/effort on proposals driven by $PROJECT then just don't contribute: you seem to have a massive chip on your shoulder for some reason. I'm mainly here because I got pinged from #1097 and think this proposal is a good idea (and does fix the original intent for home_server).

@richvdh
Copy link
Member Author

richvdh commented Nov 25, 2018

@kegsay: yay, thanks for the review \o/. I'm not sure that I agree with all of your technical points, and will follow up later.

However, please let's not reopen the discussion about motivations for the MSC - at least not here. Please let's keep the discussion focused on the technical aspects.

@maxidorius
Copy link
Contributor

maxidorius commented Nov 25, 2018

@kegsay I got pinged from #1097 too, spent my time reviewing it, told @richvdh the proposal is perfectly fine (the first commit) and exactly on point. Only to then be told this was NOT about #1097. Did you read the relevant context and the links I gave? It would appear you did not.

@richvdh said the following: (source)

anyway now you know my requirements, and they mean that the MSC1730 approach isn't ideal for me

so yes MSC1730 fulfils the requirement of adding a field to the login response, but actually it turns out that there are other ways to skin this cat

This proposal is not about #1097, it's about that portal for which we are not told the requirements and therefore we cannot review the MSC for its intended purpose. How can there be other ways if the MSC is to "officialise" a working process?

I'll say this once again: This proposal is fine. I know it's fine because it's the proposal version of a mechanism already used in other projects which are in production and it works because it's been tested. But this MSC is not about this.

Actually, if you want the full technical picture to make a relevant technical review of this, you also need #1721 and #1731 which give a more complete picture, but still not the full picture. Other pieces of information that are in #matrix-spec but not documented in the various MSCs, quoting @richvdh

  • I would like the target HS to be able to record the right IP address for the client
  • Implementing HTTP proxying in my portal turns out to be a PITA
  • well, it's a thing that picks a target homeserver based on who is trying to log in which I thought was fairly clear from the description and the sequence diagram, though I concede it is never spelt out explicitly
  • oh, it's doing a saml login flow
  • [Talking about the CAS feature of Synapse] it's an SSO solution but it still doesn't provide a mechanism to pick an HS

These are directly relevant to the technical review of this. I'm sure there are others but I cannot check for myself since the code is closed-source. I've offered several conter-arguments in the room where further discussion took place that fits for the intended goal of #1097 but do not apparently fit for the goal of this portal.

So to sum it up: three MSCs are directly related to this portal which is not explained, presented or its technical requirements/features fully given/explained. Can we stop the misdirection towards the licensing and actually focus on the relevant matters which are summed up in those questions:

  1. Can we please get the full picture about this portal which spans at least 3 MSCs and is directly relevant?
  2. Can we know if this MSC, which looked like it was written without any ulterior motive than bringing Mark home_server field deprecated #1097 to life, will actually get time invested into by the core team so it makes it to the finish line even if not relevant for that portal project?
  3. If a closed-source project has no impact on anything and is not relevant, why does it trigger 3 MSCs which are given higher priority than the ones written before it, when the claimed priority has been to finish S2S r0 since August?

@kegsay
Copy link
Member

kegsay commented Nov 25, 2018

Did you read the relevant context and the links I gave? It would appear you did not.

I took care to look at every single matrix.to link you sent.

we are not told the requirements and therefore we cannot review the MSC for its intended purpose

I don't care what their requirements are. I care about what the proposal is, and if it makes sense for the protocol. The idea of giving a base CS URL was always the intention of the protocol so this MSC stands on its own two feet.

These are directly relevant to the technical review of this. I'm sure there are others but I cannot check for myself since the code is closed-source.

They aren't relevant. If you work for them then of course you'd care about the business requirements, but if you don't then you just care about how this affects the protocol.

I am under absolutely no illusion that the core team has a lot on their plate trying to keep things alive and people happy. Provided they have a clear separation for spec work vs other work (which https://matrix.org/blog/2018/10/29/introducing-the-matrix-org-foundation-part-1-of-2/ goes a long way to do) then I have no qualms with them proposing things relevant to them because it undergoes the same process as any other person or group working on the protocol.

Now that's out of the way, I won't raise this again in this issue since I appreciate it's inappropriate and only adds to the noise. @maxidor if you want to take this elsewhere then ping me or PM me: @kegan:matrix.org

@maxidorius
Copy link
Contributor

maxidorius commented Nov 25, 2018

They aren't relevant. If you work for them then of course you'd care about the business requirements, but if you don't then you just care about how this affects the protocol.

@richvdh did say they are relevant, pointed out in the matrix.to links you read it seems.

The idea of giving a base CS URL was always the intention of the protocol so this MSC stands on its own two feet.

@kegsay: if that is true, why was it de-prioritized by its author in favor of #1731?

Mostly this is clarification of the problem domain; it also updates some of the
discussion points to reflect my current thinking.
@richvdh
Copy link
Member Author

richvdh commented Nov 26, 2018

OK, I have updated the MSC to hopefully be a little clearer about how I think this could be useful.

I think it could be useful in general, not just for my current project. As such I do not believe more details of my current project are at all relevant in the MSC document, though I have answered plenty of questions about it in the #matrix-spec:matrix.org room, and a summary is:

I have a project which requires users to log in via SAML to a portal server; based on the results of the SAML authentication, the user should automatically be logged into one of several homeservers.

The homeservers in this project are not in the same datacenter, so X-Forwarded-For is not reliable for proxying without additional authentication between the proxy and the target - which I would prefer to avoid.

@richvdh
Copy link
Member Author

richvdh commented Nov 26, 2018

Given how trivial adding this field is in practice, and that I have become convinced that it is (a) useful and (b) superior to any of the alternatives (listed in "Tradeoffs" or MSC1731), I am once again enthusiastic about its addition to the protocol. Accordingly:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Nov 26, 2018

Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@richvdh
Copy link
Member Author

richvdh commented Dec 12, 2018

Right so after all that, this should be in its FCP, right?

@richvdh richvdh added proposal-final-comment-period final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Dec 12, 2018
@anoadragon453
Copy link
Member

Merged due to FCP completing in #1748.

@richvdh richvdh added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. disposition-merge labels Dec 17, 2018
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this pull request Dec 21, 2018
@richvdh
Copy link
Member Author

richvdh commented Dec 24, 2018

This is now implemented in riot-web (matrix-org/matrix-react-sdk#2384) and synapse (matrix-org/synapse#4319)

@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Dec 24, 2018
turt2live added a commit that referenced this pull request Jan 9, 2019
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Jan 9, 2019
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Jan 30, 2019
@turt2live
Copy link
Member

merged via #1790 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet