Implementation of Oauth of Github, Google and Microsoft #4298

feyruzb · 2024-07-18T15:47:17Z

fixes #4160

The right way it should look after logging in

new added button to log in with github

Changes:

added new buttons on login page for GitHub,Google Oauth authorization
added functions createLink and getOAuthToken into authentication.thrift
imported OAuth2Session in client.py
added a case for Oauth authorization in login_user function
added definition for thrift
imported OAuth2Session in authentication.py
added code to load the authentication configuration from
added oauth method for authentication in performLogin function
added oauth case in session manager
added fields for server_config file in the server_config.json
added oauth cases in auth.js
added reusable providers dict to server config to allow easier adding of new oauth providers
create a new function get_oauth_config for getting different providers configs in session manager
added documentation for configuration

vodorok

Please see my remarks.

I have some additional comments apart from the direct in code messages:

My major concern with the implementation is that the oauth related API and its implementation is not generalized enough. The configuration is good enough for the time being.
I am not sure if we are allowed to use the Git Hub logo in our repo.
Please invite @cservakt to review the JS and VueJS parts.

I did not do a thorough review of the oauth flow in authentication.py after you addressed the above issues I will do another round concentrating on that.

Thanks for the hard work!

web/server/config/server_config.json

docs/web/authentication.md

web/api/authentication.thrift

web/client/codechecker_client/client.py

web/server/vue-cli/src/store/actions.type.js

web/server/config/server_config.json

web/server/codechecker_server/api/authentication.py

web/server/codechecker_server/session_manager.py

…s, commented lines

bruntib

Thanks again for the huge work, well done! Most of my new comments are rather TODO for me to understand and discuss things.

bruntib · 2025-02-20T14:17:33Z

docs/web/authentication.md

+#### OAuth Details per each provider <a name ="oauth-details-per-each-provider"></a>
+
+* Important: `callback_url` must always match with the link specified in the
+providers' settings when issuing an access token.


Suggested change

providers' settings when issuing an access token.

providers' settings when issuing an OAuth application.

Fixed, used suggested change.

bruntib · 2025-02-20T14:47:20Z

docs/web/authentication.md

@@ -16,6 +16,9 @@ Table of Contents
        * [<i>LDAP</i> authentication](#ldap-authentication)
            * [Configuration options](#configuration-options)
    * Membership in custom groups with [<i>regex_groups</i>](#regex_groups-authentication)


This regex link could be on the same level as "OAuth authentication"

bruntib · 2025-02-20T15:27:09Z

web/server/codechecker_server/api/authentication.py

+                    "localhost" not in \
+                    user_info_url:


According to an earlier discussion this "localhost" is here because it's needed for a test code. I still don't agree distinguishing "localhost" here.

bruntib · 2025-02-20T16:04:53Z

web/server/codechecker_server/session_manager.py

+        provider_cfg = self.__auth_config.get(
+            'method_oauth', {}).get("providers", {}).get(provider, {})
+
+        # turn off configuration if it is set to default values


I still don't agree with hard-coding these example values. I would be interested in the other reviewer's reasons.

bruntib · 2025-02-20T16:07:03Z

web/tests/functional/authentication/__init__.py

@@ -64,6 +66,10 @@ def setup_class_common():

    codechecker.add_test_package_product(host_port_cfg, TEST_WORKSPACE)

+    subprocess.Popen(["python3", "oauth_server.py"],
+                     cwd="tests/functional/authentication")
+    sleep(5)


I still think, we shouldn't operate with sleep(). We should find another way to check if the server is up. Actually, tests shouldn't start until mock server is ready.

bruntib · 2025-02-20T16:11:07Z

docs/web/authentication.md

+
+          The scope of access requested from the OAuth provider.
+
+      * `user_info_mapping`


I have a feeling that this adds unnecessary extra complexity to the logic. Why do we want to use the e-mail address as user name? Why couldn't the auth provider decide what identifies the user? This way it wouldn't be needed to query the e-mail address at different API endpoints in GitHub. I can't see why e-mail address is important from authentication point of view. Let's discuss these questions in a separate meeting.

bruntib · 2025-02-20T16:13:32Z

web/server/codechecker_server/api/authentication.py

+            raise codechecker_api_shared.ttypes.RequestFailed(
+                codechecker_api_shared.ttypes.ErrorCode.AUTH_DENIED,
+                "OAuth authentication is not enabled for provider:", provider)


Does RequestFailed() work with 3 parameters? Is the 3rd parameter concatenated to the 2nd?

Fixed, used string formatting.

bruntib · 2025-02-20T16:14:41Z

web/server/codechecker_server/api/authentication.py

+                        or not provider_db or not expires_at_db:
+                    raise codechecker_api_shared.ttypes.RequestFailed(
+                        codechecker_api_shared.ttypes.ErrorCode.AUTH_DENIED,
+                        "Something went wrong. Please try again.")


A more informative message would be useful which tells that authentication is needed first.

Fixed, message now explains what went wrong.

The purpose of this check is to make sure that the data that was fetched is not empty and are actual values.

bruntib · 2025-02-20T16:26:57Z

web/server/codechecker_server/api/authentication.py

+                    LOG.error("State, provider or expiery time mismatch.")
+                    raise codechecker_api_shared.ttypes.RequestFailed(
+                        codechecker_api_shared.ttypes.ErrorCode.AUTH_DENIED,
+                        "Something went wrong. Please try again.")


A more informative message would be useful which tells what's wrong: is it the expiration date or something else?

Fixed, message now explains what went wrong.

bruntib · 2025-02-20T16:31:05Z

web/server/codechecker_server/database/config_db_model.py

        self.token = token
        self.user_name = user_name
        self.groups = groups
        self.description = description
        self.can_expire = can_expire
        self.last_access = datetime.now()
+        self.oauth_access_token = oauth_access_token


Let's discuss the database schema in a meeting.

… storing acess token and refresh token

feyruzb requested review from bruntib and vodorok as code owners July 18, 2024 15:47

feyruzb force-pushed the branch-2-backup branch 2 times, most recently from 5ce7f18 to 68eaf7b Compare July 25, 2024 11:05

feyruzb requested a review from dkrupp as a code owner July 29, 2024 15:14

feyruzb force-pushed the branch-2-backup branch 2 times, most recently from de2e213 to d3f6b29 Compare July 30, 2024 11:02

vodorok requested changes Jul 30, 2024

View reviewed changes

feyruzb force-pushed the branch-2-backup branch 10 times, most recently from 12c68e7 to f064c2b Compare July 31, 2024 14:41

feyruzb requested a review from vodorok July 31, 2024 15:03

feyruzb force-pushed the branch-2-backup branch 3 times, most recently from b4d5a0a to d3847d6 Compare July 31, 2024 16:05

feyruzb and others added 8 commits July 31, 2024 18:14

not working api

63d5f9b

Backup before OAuth integration changes

3d6ced1

commit with new version of code ,work in progress

74c1202

second backup

3c0b13f

github working implementation

d75edbd

working github implementation , allows everyone to log in

47ba1dd

working impelementation of github Oauth

822c626

removed useless server configuration variables, useless debug message…

251f7dd

…s, commented lines

feyruzb force-pushed the branch-2-backup branch 4 times, most recently from f4d0556 to e661d1d Compare February 14, 2025 13:38

Fixing second batch of comments

0a10122

feyruzb force-pushed the branch-2-backup branch 4 times, most recently from 353e117 to 14f8ac2 Compare February 14, 2025 14:45

final commit after second fixing batch

5855127

feyruzb force-pushed the branch-2-backup branch from 14f8ac2 to 5855127 Compare February 14, 2025 17:38

bruntib requested changes Feb 20, 2025

View reviewed changes

feyruzb and others added 3 commits February 21, 2025 16:44

fixing third batch of comments

ebba988

removed access token from auth table and create oauth_token table for…

fd23880

… storing acess token and refresh token

Merge branch 'Ericsson:master' into branch-2-backup

2592bad

feyruzb force-pushed the branch-2-backup branch 9 times, most recently from 477c868 to c474887 Compare March 4, 2025 19:14

added new tables working

572f6fc

feyruzb force-pushed the branch-2-backup branch 3 times, most recently from 719771e to 868b7e5 Compare March 5, 2025 16:10

removed the allowed_users feature

d12d510

feyruzb force-pushed the branch-2-backup branch from 868b7e5 to d12d510 Compare March 5, 2025 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Oauth of Github, Google and Microsoft #4298

Implementation of Oauth of Github, Google and Microsoft #4298

feyruzb commented Jul 18, 2024 •

edited

Loading

vodorok left a comment

bruntib left a comment

bruntib Feb 20, 2025

feyruzb Feb 21, 2025 •

edited

Loading

bruntib Feb 20, 2025

bruntib Feb 20, 2025

bruntib Feb 20, 2025

bruntib Feb 20, 2025

bruntib Feb 20, 2025

bruntib Feb 20, 2025

feyruzb Feb 21, 2025

bruntib Feb 20, 2025

feyruzb Feb 21, 2025

feyruzb Feb 21, 2025

bruntib Feb 20, 2025

feyruzb Feb 21, 2025

bruntib Feb 20, 2025

	providers' settings when issuing an access token.
	providers' settings when issuing an OAuth application.


		The scope of access requested from the OAuth provider.

		* `user_info_mapping`

Implementation of Oauth of Github, Google and Microsoft #4298

Are you sure you want to change the base?

Implementation of Oauth of Github, Google and Microsoft #4298

Conversation

feyruzb commented Jul 18, 2024 • edited Loading

vodorok left a comment

Choose a reason for hiding this comment

bruntib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feyruzb Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feyruzb commented Jul 18, 2024 •

edited

Loading

feyruzb Feb 21, 2025 •

edited

Loading