Change token path authentication to /PROJECT/join/TOKEN

[Glandos]

See #802 for initial conversation.

I left project_id/token since we need to validate that the project exist before validating the token.

Otherwise, it means extracting it from the token without verifying the signature in a first step, and I'm not comfortable with this. With this, the project is checked with pull_project preprocessor, as every other endpoint.

Discussion is opened 😄

[almet]

Thanks for opening a new discussion on this.

Otherwise, it means extracting it from the token without verifying the signature in a first step, and I'm not comfortable with this.

Could you please elaborate on why this would be bad from your standpoint? Thanks :-)

If the URL is related to a project id, then the name could be {project_id}/join/{token} (or without {project_id} if we decide against it)

[almet]

If the URL is related to a project id, then the name could be {project_id}/join/{token} (or without {project_id} if we decide against it)

Disregard this, I haven't understood this was a pull request with code attached and was thinking about it the wrong way.

I find it a bit hackish to check on the dot in the URL, but that could work!

[Glandos]

@almet commented on 10 oct. 2021, 16:34 UTC+2:>

Otherwise, it means extracting it from the token without verifying the signature in a first step, and I'm not comfortable with this.

Could you please elaborate on why this would be bad from your standpoint? Thanks :-)

When deserializing, URLSafeSerializer will throw an exception BadSignature. I used to call loads twice, one without the password to get the project, and another one with the password after fetching it from the database, but it seems hackish.
However, the result nowadays is that I had to escape some other things:

• The pull_project that has a built in check for "authenticate"
• Redirection to .authenticate if the token is invalid
• Redundant data/project in verify_token

Regarding the path, I have mixed feeling:

• /join/<token> is much easier to handle, but it will broke on existing installation with a project join (silly, isn't it?)
• /project_id/<token> needs a hack, otherwise it's a catchall.
• /project_id/join/<token> is best from the developer point of view, but it's a long URL…

[almet]

I feel we should go this way with /join. The odds that this is used are fairly low in my opinion. I see two ways to solve this :

• Create a migration in the database to see if the "join" project exists, and if so tell the operator to do something about it. We could even migrate it to another name and send en email to the contact email of the "join" project ;
• When /join is used without a token, redirect to the project page. That could be the simpler solution, because /join without anything after is not used, only /join/{token}.

But… whatever works! :-)

[Glandos]

@almet commented on 10 oct. 2021, 22:02 UTC+2:

• When /join is used without a token, redirect to the project page. That could be the simpler solution, because /join without anything after is not used, only /join/{token}.

In this case we need to keep the regexp converter for /join/settle_bills, /join/statistics and all other subpaths for a project. I am really lacking an argument to decide a best solution here :)

[zorun]

I feel like /<project_id>/join/<token> is the most clear and clean way. In the other solutions, I don't like this check for a dot in the middle.

If you think it's too long, just shorten join to j?

[almet]

Or it could mean that we need to change the URL structure altogether but… yeah, this would kinda suck. Sorry for making this wrong choice in the first place.

I feel like /<project_id>/join/ is the most clear and clean way.

Okay, I think I'm coming to the same conclusion. I would be against the /j. After all explicit is better than implicit :-)

[Glandos]

OK, done, I think it can be merged.

[almet]

It looks good. I would change the name of the route though. Cheers!

[Glandos]

The last change here is on invalid token. At first, I mimic the old behavior by redirecting to the authentication form, but it was error prone from both the code and the user point of view.
Instead, let's redirect to the home with a flash message.

[Glandos]

Tests are using a lot of CPU an (wo)manpower, but sometimes, you know it worth it.