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

Require private code to edit a project settings, document better the invite options #1204

Merged
merged 5 commits into from
Jul 29, 2023

Conversation

zorun
Copy link
Collaborator

@zorun zorun commented Jul 28, 2023

This is something we had documented in our security documentation, but we didn't actually do it...

As mentioned in this document, this has good security properties: you can invite somebody with an invitation link, and they will be able to access the project but not change the private code (because they don't know the current private code).

This new check also applies to all other settings (email address, history settings, currency), which is desirable. Only somebody with knowledge of the private code can now change these settings.

Finally, document these security properties in the invitation page.

@zorun
Copy link
Collaborator Author

zorun commented Jul 28, 2023

The tests need fixing, but I want to start the discussion.

Any opinion about it @almet @Glandos ? To be honest, I thought we were already doing it, I'm the one who wrote that in our documentation...

@almet
Copy link
Member

almet commented Jul 28, 2023

That makes sense to me, go ahead with this :-)

@zorun
Copy link
Collaborator Author

zorun commented Jul 29, 2023

Ok :)

The good thing I had not anticipated: it also restricts changing the settings through the API if you just have a token (which is the same as the invite token). So, you also need the current private code to change settings:

$ curl --oauth2-bearer 'WyJmb28iXQ....'  -X PUT http://localhost:5000/api/projects/foo -d 'name=newfoo&id=newfoo&password=newbar&[email protected]'
{
    "current_password": [
        "This field is required."
    ]
}

I need to update tests, add new tests that check this, and update the API doc.

@zorun zorun mentioned this pull request Jul 29, 2023
@zorun
Copy link
Collaborator Author

zorun commented Jul 29, 2023

Crap, you can still delete a project through the API with just an auth token, so it's not strictly equivalent :(

Baptiste Jonglez added 5 commits July 29, 2023 13:15
This is something we had documented in our security documentation [1], but
we didn't actually do it...

As mentioned in [1], this has good security properties: you can invite
somebody with an invitation link, and they will be able to access the
project but not change the private code (because they don't know the
current private code).

This new check also applies to all other settings (email address, history
settings, currency), which is desirable.  Only somebody with knowledge of
the private code can now change these settings.

[1] https://ihatemoney.readthedocs.io/en/latest/security.html#giving-access-to-a-project
We now display a success flash message, and we stay on the same page
(instead of redirecting to the list of bills, which makes little sense)
Also move the "invitation link" option first, because it's the preferred
way to give access to people that only need to handle participants and
bills.

Sharing the identifier and private becomes the last option, because it
gives full access to changing settings.
@zorun zorun force-pushed the require_password_for_project_edit branch from 25e588e to 451640e Compare July 29, 2023 11:15
@zorun
Copy link
Collaborator Author

zorun commented Jul 29, 2023

OK, I'm going to merge this, and leave the project deletion issue for #1206. This is already an improvement.

@zorun zorun merged commit 7d30794 into master Jul 29, 2023
@zorun zorun deleted the require_password_for_project_edit branch July 29, 2023 12:02
@almet
Copy link
Member

almet commented Aug 7, 2023

Thanks, it's good that we don't let things rot ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants