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

Fix error that invite link doesn't work inside chat #2246

Merged
merged 27 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f9f2c60
chore: added a small update and Cypress try
Silver-IT Jul 19, 2024
4991725
chore: added comment and Cypress retry
Silver-IT Jul 19, 2024
4eacdaf
chore: check if group-chat passes three times in a row
Silver-IT Jul 19, 2024
fec63dd
chore: group-chat passes 4 times in a row
Silver-IT Jul 19, 2024
5afb858
chore: 5 times pass in a row means it fixes the heisenbug? maybe it's…
Silver-IT Jul 19, 2024
fb2fdcf
fix: error in invitelink
Silver-IT Jul 22, 2024
f266872
feat: update Cypress according to the updated invitelink format
Silver-IT Jul 22, 2024
53a035c
fix: cypress errors according to the changes of inviteLink format
Silver-IT Jul 23, 2024
073ed15
chore: added comment and Cypress retry
Silver-IT Jul 23, 2024
acf6cb9
chore: updated comment and Cypress retry
Silver-IT Jul 23, 2024
77df8ce
fix: added cy.wait and Cypress try
Silver-IT Jul 23, 2024
7b189d9
chore: added comment and Cypress retry
Silver-IT Jul 23, 2024
a1aa3ec
fix: removing cy.wait
Silver-IT Jul 23, 2024
65ae37c
chore: updated comment and check Cypress passed 2 times in a row
Silver-IT Jul 23, 2024
cde5c76
chore: really passes 3 times in a row?
Silver-IT Jul 23, 2024
57d7c2e
Merge branch '2226-heisenbug-in-group-chatspecjs-more-persistent-one'…
Silver-IT Jul 23, 2024
a4a9879
chore: updated comment and Cypress retry
Silver-IT Jul 23, 2024
60ffd1c
chore: cypress retry
Silver-IT Jul 23, 2024
6988a2d
chore: Cypress retry
Silver-IT Jul 23, 2024
268837d
Merge branch 'master' into 2240-invite-links-not-working-reliably
Silver-IT Jul 23, 2024
e220d33
chore: Cypress retry
Silver-IT Jul 23, 2024
7d35a2b
chore: Cypress retry
Silver-IT Jul 23, 2024
c5ba7fc
chore: Cypress retry
Silver-IT Jul 23, 2024
9ae5dc3
fix: heisenbug in group-proposal
Silver-IT Jul 24, 2024
93d4c32
feat: changed query to hash for security problem
Silver-IT Jul 25, 2024
64c7ff1
fix: reverted the format of hash param
Silver-IT Jul 25, 2024
6330f3d
fix: eslint error
Silver-IT Jul 25, 2024
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
4 changes: 2 additions & 2 deletions frontend/controller/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ const loginGuard = {

const inviteGuard = {
guard: (to, from) => {
// ex: http://localhost:8000/app/join#groupId=21XWnNRE7vggw4ngGqmQz5D4vAwPYqcREhEkGop2mYZTKVkx8H&secret=5157
return !(to.hash.includes('groupId=') && to.hash.includes('secret='))
// ex: http://localhost:8000/app/join?groupId=21XWnNRE7vggw4ngGqmQz5D4vAwPYqcREhEkGop2mYZTKVkx8H&secret=5157
return !(to.query.groupId && to.query.secret)
},
redirect: (to, from) => ({ path: '/' })
}
Expand Down
2 changes: 1 addition & 1 deletion frontend/model/contracts/shared/voting/proposals.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function notifyAndArchiveProposal ({ state, proposalHash, proposal, contr
export function buildInvitationUrl (groupId: string, groupName: string, inviteSecret: string, creatorID?: string): string {
const rootGetters = sbp('state/vuex/getters')
const creatorUsername = creatorID && rootGetters.usernameFromID(creatorID)
return `${location.origin}/app/join#?${(new URLSearchParams({
return `${location.origin}/app/join?${(new URLSearchParams({
Copy link
Member

Choose a reason for hiding this comment

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

@corrideat Need your review on this

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the hash symbol # was not needed since we saved all the parameters inside query which lies after ?.

Copy link
Member

@corrideat corrideat Jul 24, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

(also, if for some reason it works without a #, that's a bug: the query must not contain secrets)

Copy link
Member

Choose a reason for hiding this comment

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

@Silver-IT This PR needs to be closed while keeping this URL using a #.

The reason we're using a hash here is because the hash prevents the server from learning about what groups the user is joining, and also prevents the invite secret from being shared with the server.

Copy link
Member

@taoeffect taoeffect Jul 25, 2024

Choose a reason for hiding this comment

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

Why is it bug? I have updated all the related codes, and works fine all time.
The invite link is created, there is a secret inside. Why is it fine to contain secret in hash, and why not in query?

I answered both of these questions in my reply above.

The reason we're using a hash here is because the hash prevents the server from learning about what groups the user is joining, and also >>>> prevents the invite secret from being shared with the server. <<<

If the invite secret is shared with the server then it's not so secret anymore and the server could use it to join the group.

Regarding:

Why # and ? are there all together?

I'm not sure, perhaps it's to make it easier for using with URLSearchParams?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the invite secret is shared with the server then it's not so secret anymore and the server could use it to join the group.

So do you mean the first one is good in terms of secret and the second one is bad?

http://localhost:8000/app/join#?groupId=z9brRu3VXVfvXAHFxjuzphoq8mSsPt4Y8fVREeM5g9mPoSVEhFUG&groupName=Dreamers&secret=%5B%22edwards25519sha512batch%22%2Cnull%2C%224nGfJcEZjoVk7MOnPAZlKUBJeQbQthkK2YPAb%2BTXJwxpEIZfXdqJpTuQssNu0MrZUDbGsIrn5bOJdIODk5zRKw%3D%3D%22%5D
http://localhost:8000/app/join?groupId=z9brRu3VXVfvXAHFxjuzphoq8mSsPt4Y8fVREeM5g9mPoSVEhFUG&groupName=Dreamers&secret=%5B%22edwards25519sha512batch%22%2Cnull%2C%224nGfJcEZjoVk7MOnPAZlKUBJeQbQthkK2YPAb%2BTXJwxpEIZfXdqJpTuQssNu0MrZUDbGsIrn5bOJdIODk5zRKw%3D%3D%22%5D

And it's just the format change of invite link. There is no more changes. I can't understand what you mean by secret problem. What is the different on the server side when the user uses these two formats?

Copy link
Member

Choose a reason for hiding this comment

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

So do you mean the first one is good in terms of secret and the second one is bad?

Yes.

What is the different on the server side when the user uses these two formats?

The first one sends all of that information to the server and the second one doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, I think I understood what you mean, @taoeffect. So we should use hash instead of query for the security reason.

Copy link
Member

Choose a reason for hiding this comment

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

Yep! 👍

groupId: groupId,
groupName: groupName,
secret: inviteSecret,
Expand Down
18 changes: 9 additions & 9 deletions frontend/views/pages/Join.vue
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export default ({
},
mounted () {
// For some reason in some Cypress tests it loses the route query when initialized is called
this.ephemeral.hash = new URLSearchParams(this.$route.hash.slice(1))
this.ephemeral.query = Object.assign({}, this.$route.query)
if (syncFinished || !this.ourIdentityContractId) {
this.initialize()
} else {
Expand All @@ -114,8 +114,8 @@ export default ({
async initialize () {
try {
const messageToAskAnother = L('You should ask for a new one. Sorry about that!')
const groupId = this.ephemeral.hash.get('groupId')
const secret = this.ephemeral.hash.get('secret')
const groupId = this.ephemeral.query.groupId
const secret = this.ephemeral.query.secret
if (!groupId || !secret) {
console.error('Invalid invite link: missing group ID or secret')
this.ephemeral.errorMsg = messageToAskAnother
Expand Down Expand Up @@ -143,7 +143,7 @@ export default ({
}
if (this.ourIdentityContractId) {
const myGroupIds = Object.keys(this.$store.state[this.ourIdentityContractId]?.groups || {})
const targetGroupId = this.ephemeral.hash?.get('groupId') || ''
const targetGroupId = this.ephemeral.query?.groupId || ''
const targetGroupState = this.$store.state[targetGroupId] || {}

if (this.currentGroupId && [PROFILE_STATUS.ACTIVE, PROFILE_STATUS.PENDING].includes(targetGroupState?.profiles?.[this.ourIdentityContractId])) {
Expand All @@ -161,15 +161,15 @@ export default ({
}
return
}
const creatorID = this.ephemeral.hash.get('creatorID')
const creatorUsername = this.ephemeral.hash.get('creatorUsername')
const creatorID = this.ephemeral.query.creatorID
const creatorUsername = this.ephemeral.query.creatorUsername
const who = creatorUsername || creatorID
const message = who
? L('{who} invited you to join their group!', { who })
: L('You were invited to join')

this.ephemeral.invitation = {
groupName: this.ephemeral.hash.get('groupName') ?? L('(group name unavailable)'),
groupName: this.ephemeral.query.groupName ?? L('(group name unavailable)'),
creatorID,
creatorUsername,
message
Expand All @@ -196,8 +196,8 @@ export default ({
},
async accept () {
this.ephemeral.errorMsg = null
const groupId = this.ephemeral.hash.get('groupId')
const secret = this.ephemeral.hash.get('secret')
const groupId = this.ephemeral.query.groupId
const secret = this.ephemeral.query.secret

const profileStatus = this.$store.state.contracts[groupId]?.profiles?.[this.ourIdentityContractId]?.status
if ([PROFILE_STATUS.ACTIVE, PROFILE_STATUS.PENDING].includes(profileStatus)) {
Expand Down
5 changes: 4 additions & 1 deletion test/cypress/integration/group-proposals.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ describe('Proposals - Add members', () => {
})

it('an invalid invitation link cannot be used', () => {
cy.visit('/app/join#?groupId=321&secret=123')
cy.visit('/app/join?groupId=321&secret=123')
cy.getByDT('pageTitle')
.invoke('text')
.should('contain', 'Oh no! This invite is not valid')
Expand All @@ -408,6 +408,9 @@ describe('Proposals - Add members', () => {
cy.getByDT('openAllProposals').click()
cy.get('[data-test="modal"] > .c-container .c-title').should('contain', 'Archived proposals')
cy.getByDT('modal').within(() => {
// NOTE: this is to wait until all of the 5 proposals are loaded inside the modal
cy.get('.c-container > .c-header-info .has-text-1').should('contain', '5 proposals')

getProposalItems().eq(2).within(() => {
cy.getByDT('title', 'p').should('contain', 'You proposed')
cy.getByDT('statusDescription')
Expand Down
6 changes: 3 additions & 3 deletions test/cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ const API_URL = Cypress.config('baseUrl')
// util funcs
const randomFromArray = arr => arr[Math.floor(Math.random() * arr.length)] // importing giLodash.js fails for some reason.
const getParamsFromInvitationLink = invitationLink => {
const params = new URLSearchParams(new URL(invitationLink).hash.slice(1))
const url = new URL(invitationLink)
return {
groupId: params.get('groupId'),
inviteSecret: params.get('secret')
groupId: url.searchParams.get('groupId'),
inviteSecret: url.searchParams.get('secret')
}
}

Expand Down
Loading