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

Include ID server access token in 3pid invites. #1518

Merged
merged 2 commits into from
Jul 12, 2022

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Jul 11, 2022

Most of the changes in this PR are for invites as part of room creation. Sending an ordinary invite had all the right logic, but also a bug.

I did consider adding the access token field to MX3PIDInvite however given the homeserver could say it isn't needed, this didn't seem great with the json dictionary property checking for a specific number of fields.

Fixes element-hq/element-ios#6385

@@ -2523,7 +2584,7 @@ - (MXHTTPOperation*)inviteByThreePid:(NSString*)medium
operation = [self addIdentityAccessTokenToParameters:parameters success:^(NSDictionary *updatedParameters) {
MXStrongifyAndReturnIfNil(self);

MXHTTPOperation *operation2 = [self inviteByThreePidToRoom:roomId parameters:parameters success:success failure:failure];
MXHTTPOperation *operation2 = [self inviteByThreePidToRoom:roomId parameters:updatedParameters success:success failure:failure];
Copy link
Member Author

Choose a reason for hiding this comment

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

Everything was already setup for invites, there was simply a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we capture that bug in an integration test? (MXRestClientTests do not currently run on CI, but should be also enabled in the future)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good point, I'll take a look. I did have a scan through the SDK tests but we don't seem to have anything covering the identity server or 3pid functionality (possibly due to not having an identity server set up), but would be really good to have this covered with some sort of test.

@pixlwave pixlwave requested review from a team and Anderas and removed request for a team July 11, 2022 14:27
@codecov-commenter
Copy link

Codecov Report

Merging #1518 (f39f52a) into develop (da888f1) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #1518      +/-   ##
===========================================
- Coverage    11.67%   11.65%   -0.02%     
===========================================
  Files          507      507              
  Lines        83204    83254      +50     
  Branches     35536    35561      +25     
===========================================
- Hits          9711     9705       -6     
- Misses       73135    73191      +56     
  Partials       358      358              
Impacted Files Coverage Δ
MatrixSDK/MXRestClient.m 2.49% <0.00%> (-0.03%) ⬇️
MatrixSDKTests/MXHTTPAdditionalHeadersUnitTests.m 71.73% <0.00%> (-13.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da888f1...f39f52a. Read the comment docs.

MatrixSDK/MXRestClient.m Outdated Show resolved Hide resolved
@@ -2523,7 +2584,7 @@ - (MXHTTPOperation*)inviteByThreePid:(NSString*)medium
operation = [self addIdentityAccessTokenToParameters:parameters success:^(NSDictionary *updatedParameters) {
MXStrongifyAndReturnIfNil(self);

MXHTTPOperation *operation2 = [self inviteByThreePidToRoom:roomId parameters:parameters success:success failure:failure];
MXHTTPOperation *operation2 = [self inviteByThreePidToRoom:roomId parameters:updatedParameters success:success failure:failure];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we capture that bug in an integration test? (MXRestClientTests do not currently run on CI, but should be also enabled in the future)

@richvdh
Copy link
Member

richvdh commented Jul 12, 2022

Thanks for jumping on this so quickly @pixlwave! ❤️

@pixlwave pixlwave merged commit d1c8e18 into develop Jul 12, 2022
@pixlwave pixlwave deleted the doug/6385-3pid-access-token branch July 12, 2022 10: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.

ID server access token not included when making a 3pid invite
4 participants