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

MSC1961: Integration manager authentication APIs #1961

Merged
merged 6 commits into from
Aug 23, 2019

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Apr 9, 2019

Rendered
Upstream proposal: MSC1956 - Integrations API
Discussion room: #msc-integrations-api:t2bot.io
Implementation (IM): turt2live/matrix-dimension@57d585d
Implementation (Client): Incomplete


This is being contributed under the hat of "author of Dimension":

Signed-off-by: Travis Ralston <[email protected]>

@turt2live turt2live added proposal-in-review proposal A matrix spec change proposal labels Apr 9, 2019
@turt2live turt2live changed the title Proposal for basic integration manager authentication APIs MSC1961: Integration manager authentication APIs Apr 9, 2019
turt2live added a commit to turt2live/matrix-dimension that referenced this pull request Jun 28, 2019

#### POST `/_matrix/integrations/v1/account/logout`

Logs the token out, rendering it useless for future requests.
Copy link
Member

Choose a reason for hiding this comment

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

Should this say that all tokens associated with the user should be logged out, not just the token this request was made with?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, just the token that was requested. This MSC does not propose an equivalent for /logout/all

Copy link
Member

Choose a reason for hiding this comment

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

I guess the follow-up question is then; should IM's have a /account/deactivate? When a user deactivates their account, it would be good for clients to pass that information to the IM, to tell it that this user will no longer exist and that all possible tokens and user information should be deleted. Or should this be a different MSC?

Pinging @lampholder for the privacy side re IM's.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd make that a separate MSC. I'm trying to keep these integration manager MSCs tightly scoped to avoid creeping features, given they are all trying to introduce a whole new API surface. This particular MSC is scoped to just authentication: deactivation is a feature which affects authentication but is not required for authentication. For instance, an integration manager could just delete everything after the last token is logged out. Similarly, there is no API for listing how many tokens you have assigned to you (and logging them out).

@turt2live
Copy link
Member Author

MSC2140 and various clients are going to require this in order to continue functioning. This also hasn't received a lot of feedback.

To get more feedback and to unblock some part of the work...

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 9, 2019

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

No concerns currently listed.

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.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Aug 9, 2019
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Short, sweet and sane.

@jaywink
Copy link
Member

jaywink commented Aug 15, 2019

All good here 👍

@richvdh
Copy link
Member

richvdh commented Aug 16, 2019

I'm not familiar with the integrations API as it stands, but no objections from me here.

@mscbot reviewed

@erikjohnston
Copy link
Member

Process question: will these MSCs land separately or only once the full "integrations api" MSC lands? I'm comfortable with saying these APIs look fine, but I can equally imagine that after we land a bunch of integration API related MSCs it becomes clear that already MSCed APIs need to change.

Maybe we basically say: "this MSC is part of the unstable integration API module" and then the final MSC basically says "stabilise the integration API module"?

@turt2live
Copy link
Member Author

I'm not familiar with the integrations API as it stands, but no objections from me here.

ftr, there isn't one. This is part 1 of many to make it a thing.

@mscbot mscbot added 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 Aug 18, 2019
@mscbot
Copy link
Collaborator

mscbot commented Aug 18, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot 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. labels Aug 23, 2019
@mscbot
Copy link
Collaborator

mscbot commented Aug 23, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit 4ad9bf7 into master Aug 23, 2019
@turt2live turt2live deleted the travis/msc/integrations/auth branch August 23, 2019 20:24
jaywink added a commit to element-hq/element-ios that referenced this pull request Aug 26, 2019
As per MSC1961, add to the whitelisted integrations_widget_urls
the new paths. This allows us to switch Scalar over to use the
new path as default.

Note, the legacy "scalar-staging.riot.im" is these days just a redirect
to scalar-staging.vector.im, so there is no addition for that. It still
needs Riot side whitelisting though for existing widgets.

Refs: matrix-org/matrix-spec-proposals#1961
jaywink added a commit to element-hq/element-ios that referenced this pull request Aug 26, 2019
As per MSC1961, add to the whitelisted integrations_widget_urls
the new paths. This allows us to switch Scalar over to use the
new path as default.

Note, the legacy "scalar-staging.riot.im" is these days just a redirect
to scalar-staging.vector.im, so there is no addition for that. It still
needs Riot side whitelisting though for existing widgets.

Refs: matrix-org/matrix-spec-proposals#1961
jaywink added a commit to element-hq/element-ios that referenced this pull request Aug 26, 2019
As per MSC1961, add to the whitelisted integrations_widget_urls
the new paths. This allows us to switch Scalar over to use the
new path as default.

Note, the legacy "scalar-staging.riot.im" is these days just a redirect
to scalar-staging.vector.im, so there is no addition for that. It still
needs Riot side whitelisting though for existing widgets.

Refs: matrix-org/matrix-spec-proposals#1961
Signed-off-by: Jason Robinson <[email protected]>
jaywink added a commit to element-hq/riot-android that referenced this pull request Aug 26, 2019
As per MSC1961, add to the whitelisted integrations_widget_urls
the new paths. This allows us to switch Scalar over to use the
new path as default.

Note, the legacy "scalar-staging.riot.im" is these days just a redirect
to scalar-staging.vector.im, so there is no addition for that. It still
needs Riot side whitelisting though for existing widgets.

Refs: matrix-org/matrix-spec-proposals#1961
Signed-off-by: Jason Robinson <[email protected]>
@turt2live turt2live self-assigned this Aug 26, 2019
@turt2live
Copy link
Member Author

@turt2live turt2live 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 Aug 26, 2019
@jaywink
Copy link
Member

jaywink commented Aug 30, 2019

This has now been implemented and deployed for Scalar as well.

@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue. disposition-merge integrations Integration (Manager) API kind:feature MSC for not-core and not-maintenance stuff privacy-sprint Temporary label: privacy-related stuff proposal A matrix spec change proposal spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec
Projects
Status: BLOCKED, requires spec writing
Development

Successfully merging this pull request may close these issues.

7 participants