search

makeplane / plane

🔥 🔥 🔥 Open Source JIRA, Linear, Monday, and Asana Alternative. Plane helps you track your issues, epics, and product roadmaps in the simplest way possible.
http://plane.so
GNU Affero General Public License v3.0
29.55k stars 1.63k forks source link

[bug]: gitlab oauth incomplete, `def authentication_error_code()` missing #4897

Closed almereyda closed 2 months ago

almereyda commented 3 months ago

Is there an existing issue for this?

Current behavior

https://github.com/makeplane/plane/blob/94e6fd4b29a5345b26e19e94346e3d15f0e3f7f1/apiserver/plane/authentication/adapter/oauth.py#L65-L87

calls a method _provider_error_code(), which is not defined in the file, nor anywhere else in the repository.

https://github.com/search?q=repo%3Amakeplane%2Fplane%20_provider_error_code()&type=code

It's part of an effort to bring OAuth-based GitLab authentication to plane.

While some of the early implementations have already been improved, as with 84236f506b0b22d337492c1994abc855a1081c21, others have fallen behind.

_provider_error_code() has been part of the earliest implementation in #4692.

It was reintroduced in #4828

https://github.com/makeplane/plane/pull/4828/commits/73ea6455fd7ef07a83e30f4fe4784b49a3a96711#diff-e8db188a9821b1c82b6a9e8552288f209242e8c62133bd8b924e42db59411e63R42

https://github.com/makeplane/plane/commit/73ea6455fd7ef07a83e30f4fe4784b49a3a96711#diff-e8db188a9821b1c82b6a9e8552288f209242e8c62133bd8b924e42db59411e63R42-R50

and renamed to a more canonical version of its name, that caters to it's use

https://github.com/makeplane/plane/commit/22f09a6b31f8404fb6883f3655c55f70d03e9570

https://github.com/makeplane/plane/commit/22f09a6b31f8404fb6883f3655c55f70d03e9570#diff-e8db188a9821b1c82b6a9e8552288f209242e8c62133bd8b924e42db59411e63R92

I would suggest to test the implementation and would expect it to fail whenever get_user_token() raises an exception. Is it possible to test for such a case, e.g. in an integration test example?

Completion of this code-path seems suitable to people who find themselves in situations debugging a non-working OAuth authentication configuration of a plane instance.

Steps to reproduce

  1. Make yourself known to the course of action and conversation in:

    • 4692

    • 4828 (conflicts don't show anymore, but the diff to that branch gives insights what else to cherry-pick)

  2. Inspect the links in the previous section.
  3. Follow up with the rationale to change the name of _provider_error_code() to authentication_error_code().

Environment

Deploy preview

Browser

Other

Variant

Self-hosted

Version

v0.22.0-dev

almereyda commented 3 months ago

The method is now available on preview.

https://github.com/makeplane/plane/blob/734e920e08c73cda8664149aa08ca24a05ee020e/apiserver/plane/authentication/adapter/oauth.py#L42-L50

What is missing to being able to close here from your point of view, @pushya22? Have we observed unintended side-effects from the change?

pablohashescobar commented 2 months ago

@almereyda, the issue has been fixed. Closing the issue.