-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Support for MSC2140 (terms of service for IS/IM) #988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! 😁 Lots of comments for a small change, sorry... 😅
Co-Authored-By: J. Ryan Stinnett <[email protected]>
and IMs shouldn't have a slash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me! 😁
However, do take a look at this React SDK comment before merging, as I am unsure which SDK should handle such things.
src/base-apis.js
Outdated
case SERVICE_TYPES.IS: | ||
return baseUrl + httpApi.PREFIX_IDENTITY_V2 + '/terms'; | ||
case SERVICE_TYPES.IM: | ||
return baseUrl + '/terms'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( at not using the namespace for the IM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at changing but stopped on thinking about changing all the other ones. I guess we could just use it for this one though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend it, just for the sake of not making the problem worse. The baseUrl
might have a path component (scalar is /api
, dimension is /api/v1/scalar
) which might have to be trimmed off before applying the spec prefix + /terms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, the fact that we have configs with the api prefix is the main problem - from a riot/scalar PoV we can just ship the config change with the new app version but I don't know how this would work for anyone with a custom IM: I was going to run it past you with your Dimension hat on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we can't just update this path because then we'd have to move the /api/
from the config to the hardcoded part in the source, and this would break dimension since for dimension, the /api
part goes in the middle.
I think our only option is going to be to update all the paths at once, so once scalar supports _matrix
paths, we can then patch riot develop to use _matrix
paths across the board and it will be compatible with deployed scalar and any dimension new enough to support the _matrix
paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dimension supports the route at the root, like the spec implies. You're welcome to copy the parsing from Dimension (which does use the root-level route on upstreams):
const parsed = new URL(baseUrl);
parsed.pathname = `/_matrix/integrations/v1/terms`;
return parsed.toString();
getTerms is un-authed so doesn't need the access token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works great from a Dimension perspective (thank you!). Looking forward to testing this against scalar-staging when that's available :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes look good to me as well! 😁
Support for MSC2140