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

Improve [Matrix] badge generation #2527

Merged
merged 30 commits into from
Dec 20, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a20f367
Resolve homeservers' names through DNS if possible
babolivier Dec 13, 2018
d3f4110
Add a connectivity check before trying to register
babolivier Dec 13, 2018
d02e309
Comply with the overall coding style
babolivier Dec 13, 2018
ffc3282
Bonus: Use room aliases (with correct syntax) instead of room IDs
babolivier Dec 13, 2018
8453c2f
Bonus: Store access tokens in memory to avoid countless registrations
babolivier Dec 13, 2018
a72bb2d
Update doc
babolivier Dec 13, 2018
55b33b0
Fix error message
babolivier Dec 13, 2018
a7468fd
Add new test for alias + improve existing tests
babolivier Dec 13, 2018
48cff03
Use optional URL parameter to determine where to reach homeservers ra…
babolivier Dec 17, 2018
0b6a19b
Get branch up to date with the latest master revision
babolivier Dec 17, 2018
e17fd8e
Remove in-memory cache for access tokens
babolivier Dec 18, 2018
3d9a5bb
Add test case for homeserver fqdn
babolivier Dec 18, 2018
3ae25b6
Rename room readibility test with a more explicit name
babolivier Dec 18, 2018
1c1fccd
Fix unknown request test
babolivier Dec 18, 2018
ea1433e
Get branch up to date with the latest master revision
babolivier Dec 18, 2018
3ac4a25
Use namedParams instead of exampleUrl
babolivier Dec 20, 2018
b6c9938
Get branch up to date with the latest master revision
babolivier Dec 20, 2018
d655fa1
Add serverFqdnOptional to example (as we run into errors otherwise)
babolivier Dec 20, 2018
75b799d
Make error thrown when parsing the alias more explicit
babolivier Dec 20, 2018
f929910
Remove mention of 'room host'
babolivier Dec 20, 2018
fad3add
Incorporate upstream's test suite improvements
babolivier Dec 20, 2018
2800387
Fix handling of port in alias
babolivier Dec 20, 2018
8bb5ebe
Add tests for invalid alias and port in alias
babolivier Dec 20, 2018
3ec0c77
Update comment
babolivier Dec 20, 2018
2a94b15
URL-encode room IDs and aliases
babolivier Dec 20, 2018
410015e
Move server FQDN to query parameters
babolivier Dec 20, 2018
2d4ec57
Update doc and examples with recent changes
babolivier Dec 20, 2018
56db2a9
Fix typo in doc
babolivier Dec 20, 2018
fcdbd8e
Validate query params + use snake case
paulmelnikow Dec 20, 2018
61f7194
Merge branch 'master' into babolivier/fix-matrix
paulmelnikow Dec 20, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 76 additions & 24 deletions services/matrix/matrix.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@

const Joi = require('joi')
const BaseJsonService = require('../base-json')
const errors = require('../errors')

const matrixRegisterSchema = Joi.object({
access_token: Joi.string().required(),
}).required()

const matrixAliasLookupSchema = Joi.object({
room_id: Joi.string().required(),
})

const matrixStateSchema = Joi.array()
.items(
Joi.object({
Expand All @@ -31,15 +36,36 @@ const documentation = `
<ul>
<li>Select the desired room inside the Riot.im client</li>
<li>Click on the room settings button (gear icon) located near the top right of the client</li>
<li>Scroll to the very bottom of the settings page and look under the <code>Advanced</code> tab</li>
<li>You should see the <code>Internal room ID</code> with your rooms ID next to it (ex: <code>!ltIjvaLydYAWZyihee:matrix.org</code>)</li>
<li>Replace the IDs <code>:</code> with <code>/</code></li>
<li>The final badge URL should look something like this <code>/matrix/!ltIjvaLydYAWZyihee/matrix.org.svg</code></li>
<li>Scroll to the very bottom of the settings page and look under the <code>Addresses</code> section</li>
<li>You should see one or more <code>room addresses (or aliases)</code>, which can be easily identified with their starting hash (<code>#</code>) character (ex: <code>#twim:matrix.org</code>)</li>
<li>If there is no address for this room, add one under <code>Local addresses for this room</code></li>
<li>Remove the starting hash character (<code>#</code>)</li>
<li>The final badge URL should look something like this <code>/matrix/twim:matrix.org.svg</code></li>
</ul>
</br>
Some Matrix homeservers don't hold a server name matching where they live (e.g. if the homeserver <code>example.com</code> that created the room alias <code>#mysuperroom:example.com</code> lives at <code>matrix.example.com</code>).
</br>
If that is the case of the homeserver that created the room alias used for generating the badge, you will need to add an extra parameter to the URL, by adding a forward slash followed by the server's FQDN (fully qualified domain name) between the room alias and the <code>.svg</code> extension.
</br>
The final badge URL should then look something like this <code>/matrix/mysuperroom:example.com/matrix.example.com.svg</code>.
Copy link
Member

@paulmelnikow paulmelnikow Dec 20, 2018

Choose a reason for hiding this comment

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

Another option we have with the API is to put the FQDN in the query string, like the way we handle custom NPM registries. Would that be better?

Usually obscure parameters are handled that way, whereas commonly used parameters are placed into the path.

I don't have an opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I didn't think about putting it there. I agree on that it belongs there rather than the path.

Copy link
Member

Choose a reason for hiding this comment

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

The gitlab pipeline status badge is a good modern example. Are you up for updating that? I'm good to get this merged after that! Really appreciate all your work on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 410015e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, didn't see your messages in the meantime. I based my work on the NPM badges (in a much simpler version though).

Copy link
Member

Choose a reason for hiding this comment

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

No prob :) I just pushed a commit with validation, and switching to snake case which for better or worse is what we've used for query params in services.

I'll merge this in a second if all looks good!

</p>
`

module.exports = class Matrix extends BaseJsonService {
async retrieveAccessToken({ host }) {
let auth
try {
auth = await this.registerAccount({ host, guest: true })
} catch (e) {
if (e.prettyMessage === 'guests not allowed') {
// attempt fallback method
auth = await this.registerAccount({ host, guest: false })
} else throw e
}

return auth.access_token
}

async registerAccount({ host, guest }) {
return this._requestJson({
url: `https://${host}/_matrix/client/r0/register`,
Expand All @@ -64,22 +90,51 @@ module.exports = class Matrix extends BaseJsonService {
})
}

async fetch({ host, roomId }) {
let auth
try {
auth = await this.registerAccount({ host, guest: true })
} catch (e) {
if (e.prettyMessage === 'guests not allowed') {
// attempt fallback method
auth = await this.registerAccount({ host, guest: false })
} else throw e
async lookupRoomAlias({ host, roomAlias, accessToken }) {
return this._requestJson({
url: `https://${host}/_matrix/client/r0/directory/room/%23${roomAlias}`,
Copy link
Member

Choose a reason for hiding this comment

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

What characters are valid for a room alias? Would it be better to use

`https://${host}/_matrix/client/r0/directory/room/${encodeURIComponent(`#${roomAlias}`)}`

in case there is some other character that needs URL-encoding?

Copy link
Contributor Author

@babolivier babolivier Dec 20, 2018

Choose a reason for hiding this comment

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

It should be fairly okay without it, but better safe than sorry. Good catch, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2a94b15

schema: matrixAliasLookupSchema,
options: {
qs: {
access_token: accessToken,
},
},
errorMessages: {
401: 'bad auth token',
404: 'room not found',
429: 'rate limited by rooms host',
},
})
}

async fetch({ roomAlias, serverFQDN }) {
let host
if (serverFQDN === undefined) {
const splitAlias = roomAlias.split(':')
// A room alias can either be in the form #localpart:server or
// #localpart:server:port. In the latter case, it's wiser to skip the name
// resolution and use that value right away.
switch (splitAlias.length) {
case 2:
host = splitAlias[1]
break
case 3:
host = splitAlias[2] + splitAlias[3]
break
default:
throw new errors.InvalidParameter()
Copy link
Member

Choose a reason for hiding this comment

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

Will this be immediately obvious to users what the error is with the value they have provided for roomAlias or would it be helpful to add a prettyMessage? For example:

...InvalidParameter({ prettyMessage: ' whatever makes sense here' })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, right, that was before the second (optional) parameter was added, so it was obvious back then, but not that much now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 75b799d

}
} else {
host = serverFQDN
}
const accessToken = await this.retrieveAccessToken({ host })
const lookup = await this.lookupRoomAlias({ host, roomAlias, accessToken })
const data = await this._requestJson({
url: `https://${host}/_matrix/client/r0/rooms/${roomId}/state`,
url: `https://${host}/_matrix/client/r0/rooms/${lookup.room_id}/state`,
schema: matrixStateSchema,
options: {
qs: {
access_token: auth.access_token,
access_token: accessToken,
},
},
errorMessages: {
Expand Down Expand Up @@ -109,11 +164,8 @@ module.exports = class Matrix extends BaseJsonService {
}
}

async handle({ roomId, host, authServer }) {
const members = await this.fetch({
host,
roomId: `${roomId}:${host}`,
})
async handle({ roomAlias, serverFQDN }) {
const members = await this.fetch({ roomAlias, serverFQDN })
return this.constructor.render({ members })
}

Expand All @@ -128,17 +180,17 @@ module.exports = class Matrix extends BaseJsonService {
static get route() {
return {
base: 'matrix',
format: '([^/]+)/([^/]+)',
capture: ['roomId', 'host'],
format: '([^/]+)(?:/([^/]+))?',
capture: ['roomAlias', 'serverFQDN'],
}
}

static get examples() {
return [
{
title: 'Matrix',
exampleUrl: '!ltIjvaLydYAWZyihee/matrix.org',
pattern: ':roomId/:host',
exampleUrl: 'twim:matrix.org',
Copy link
Member

@calebcartwright calebcartwright Dec 20, 2018

Choose a reason for hiding this comment

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

Can we use namedParams here instead of exampleUrl? See #2308 for more info

I think it would be something like this if I'm reading the route pattern correctly

namedParams: {
  roomAlias: 'twin:matrix.org',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 3ac4a25 (and d655fa1)

pattern: ':roomAlias/:server_fqdn_optional',
staticExample: this.render({ members: 42 }),
documentation,
},
Expand Down
Loading