[server] Redirect without hitting sorry endpoint. #5568

laushinka · 2021-09-07T09:03:31Z

Description

When an unauthorized GitLab user logs into Gitpod, we used to handle that by redirecting to /sorry. However, GitLab doesn't clearly show this to the user, and the user doesn't know that they have an incomplete auth process.
Now we redirect to the frontend and show the error message, so that the user will know that they should complete the auth process on GitLab.

Related Issue(s)

Fixes #5183

How to test

Same as how @AlexTugarev described the reproduction steps here:

Release Notes

GitLab users who have not completed the auth process on GitLab will be notified with an error message. ([#5568](https://github.com/gitpod-io/gitpod/pull/5568))

Fixes #5183

laushinka · 2021-09-07T09:06:59Z

@JanKoehnlein This does affect what the user sees, but I'm not sure if this should go into Changelog?

JanKoehnlein · 2021-09-07T09:11:24Z

@laushinka If in doubt, put it into changelog

gtsiolis · 2021-09-07T13:21:05Z

Looking at this now! 👀

gtsiolis · 2021-09-07T13:21:56Z

/werft run

👍 started the job as gitpod-build-laushinka-gitlab-tos-5183.8

corneliusludmann · 2021-09-07T14:12:26Z

Seems not exactly to touch this PR, but anyways: Does it perhaps make sense to make it clearer in the error message that it is the GitLab ToS and not the Gitpod ToS? Could imagine that this is easy to read over. 🤔

laushinka · 2021-09-07T14:26:08Z

Seems not exactly to touch this PR, but anyways: Does it perhaps make sense to make it clearer in the error message that it is the GitLab ToS and not the Gitpod ToS? Could imagine that this is easy to read over. 🤔

@corneliusludmann Yes that totally makes sense!

gtsiolis

UX looks great, @laushinka! Much better than hitting an error page. 🔮

However, I agree with what @corneliusludmann mentioned in #5568 (comment) about the message clarity. This could also follow the voice and tone used in other error messages we show up there like in #3950. ❗

Feel free to open a follow up issue or PR for improving that message. 🏓

🍊 🍊 🍊 🍊

I'd suggest to rephrase from:

Error: 403 Forbidden - You (@username) must accept the Terms of Service in order to perform this action. Please access GitLab from a web browser to accept these terms.

To:

The selected provider has updated their Terms of Service. Please, visit the provider to review and accept the terms before using @username to login in Gitpod.

BEFORE	AFTER

gtsiolis · 2021-09-07T14:49:08Z

components/server/src/auth/generic-auth-provider.ts

-            if (!UnconfirmedUserException.is(err)) {
-                // user did not accept ToS. Don't count this towards the error burn rate.
-                increaseLoginCounter("failed", this.host);
+            if (UnconfirmedUserException.is(err)) {


praise: Short and sweet!

roboquat · 2021-09-07T15:15:12Z

LGTM label has been added.

Git tree hash: c13e2c180fdddd9a8975f66caa208db42cb1fbb6

laushinka · 2021-09-07T19:01:56Z

I'd suggest to rephrase from:

Error: 403 Forbidden - You (@username) must accept the Terms of Service in order to perform this action. Please access GitLab from a web browser to accept these terms.

To:

The selected provider has updated their Terms of Service. Please, visit the provider to review and accept the terms before using @username to login in Gitpod.

@gtsiolis Much better text! Will do the change :) Should we make it specific to GitLab? i.e. "GitLab has updated their..." I think this error only happens with them. But generic also works so I don't have strong feelings either way.

gtsiolis · 2021-09-07T19:23:23Z

Should we make it specific to GitLab?

Hey @laushinka! In the scope of this PR and #5183, making it specific to GitLab could suffice. Your call! 📞

I think this error only happens with them.

That's correct!

🍋 🍋 🍋 🍋

However, keep in mind that:

This issue occurred due a recent update for the ToS and Privacy Policy for GitLab.com users a few months ago, see https://gitlab.com/gitlab-org/gitlab-foss/-/issues/44798. 💡
ToS and Privacy Policy are also an Admin Area feature which allows GitLab.com admins and GitLab self-hosted admins to define new ToS and Privacy Policy and require new and existing to accept the terms, see https://docs.gitlab.com/ee/user/admin_area/settings/terms.html. 💡
New changes to ToS and Privacy Policy can be re-enforced for both GitLab.com and self-hosted GitLab instances by design. 💡

This means that:

Both GitLab.com and GitLab self-hosted instances could update their ToS and Privacy Policy and require users to accept the terms. ❗
This issue is related to GitLab.com and GitLab self-hosted instances, however, including the term GitLab in the error message could be confusing if a user selects a GitLab self-hosted instance to login. ❗

laushinka · 2021-09-07T19:32:56Z

including the term GitLab in the error message could be confusing if a user selects a GitLab self-hosted instance to login.

@gtsiolis Ahh, that makes sense.

laushinka · 2021-09-07T19:37:43Z

Wait I actually can't find where this string comes from. @corneliusludmann do you know where the 403 error message comes from? Is it a response from GitLab?

laushinka · 2021-09-08T12:20:36Z

@gtsiolis @corneliusludmann ~~I think this message isn't under our control unfortunately, or at least I couldn't find it..~~ So maybe it's a good idea to have this as a separate issue after all.-

Yes from GitLab: https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/auth/user_access_denied_reason.rb#L17

JanKoehnlein · 2021-09-08T12:43:34Z

/lgtm

roboquat · 2021-09-08T12:43:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtsiolis, JanKoehnlein

Associated issue: #5183

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/server/OWNERS~~ [JanKoehnlein]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[server] Redirect without hitting sorry endpoint.

e9ebdf8

Fixes #5183

roboquat added the do-not-merge/release-note-label-needed label Sep 7, 2021

roboquat requested a review from corneliusludmann September 7, 2021 09:03

roboquat added the size/XS label Sep 7, 2021

laushinka requested review from gtsiolis and JanKoehnlein and removed request for corneliusludmann September 7, 2021 09:03

gtsiolis approved these changes Sep 7, 2021

View reviewed changes

roboquat assigned gtsiolis Sep 7, 2021

roboquat added the lgtm label Sep 7, 2021

roboquat assigned JanKoehnlein Sep 8, 2021

roboquat added the approved label Sep 8, 2021

laushinka removed the do-not-merge/release-note-label-needed label Sep 8, 2021

roboquat merged commit 2f8b87f into main Sep 8, 2021

roboquat deleted the laushinka/gitlab-tos-5183 branch September 8, 2021 14:32

roboquat added the release-note label Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server] Redirect without hitting sorry endpoint. #5568

[server] Redirect without hitting sorry endpoint. #5568

laushinka commented Sep 7, 2021 •

edited by JanKoehnlein

Loading

laushinka commented Sep 7, 2021

JanKoehnlein commented Sep 7, 2021

gtsiolis commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

corneliusludmann commented Sep 7, 2021

laushinka commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment

gtsiolis Sep 7, 2021

roboquat commented Sep 7, 2021

laushinka commented Sep 7, 2021 •

edited

Loading

gtsiolis commented Sep 7, 2021 •

edited

Loading

laushinka commented Sep 7, 2021

laushinka commented Sep 7, 2021

laushinka commented Sep 8, 2021 •

edited

Loading

JanKoehnlein commented Sep 8, 2021

roboquat commented Sep 8, 2021

[server] Redirect without hitting sorry endpoint. #5568

[server] Redirect without hitting sorry endpoint. #5568

Conversation

laushinka commented Sep 7, 2021 • edited by JanKoehnlein Loading

Description

Related Issue(s)

How to test

Release Notes

laushinka commented Sep 7, 2021

JanKoehnlein commented Sep 7, 2021

gtsiolis commented Sep 7, 2021 • edited by werft-gitpod-dev-com bot Loading

gtsiolis commented Sep 7, 2021 • edited by werft-gitpod-dev-com bot Loading

corneliusludmann commented Sep 7, 2021

laushinka commented Sep 7, 2021 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

gtsiolis Sep 7, 2021

Choose a reason for hiding this comment

roboquat commented Sep 7, 2021

laushinka commented Sep 7, 2021 • edited Loading

gtsiolis commented Sep 7, 2021 • edited Loading

laushinka commented Sep 7, 2021

laushinka commented Sep 7, 2021

laushinka commented Sep 8, 2021 • edited Loading

JanKoehnlein commented Sep 8, 2021

roboquat commented Sep 8, 2021

laushinka commented Sep 7, 2021 •

edited by JanKoehnlein

Loading

gtsiolis commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Sep 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Sep 7, 2021 •

edited

Loading

gtsiolis commented Sep 7, 2021 •

edited

Loading

laushinka commented Sep 8, 2021 •

edited

Loading