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

…y email for GitHub

…the token fetching process

dkrupp

Please make sure that you correctly and consistently use the token, session variable names.
Please consider factoring out the oauth access token into a separate table.
When do we remove rows from teh oauth session table?
How do we handle token refreshing? Would it be automated? what happens if an access token expires?

dkrupp · 2025-01-08T13:12:32Z

web/api/authentication.thrift

+  // Returns list of providers for oauth for respective appearence of buttons.
+  list<string> getOauthProviders(),
+
+  // Create a link for the user to log in for github Oauth.


why github? I guess you could write instead: Create a link for the user to log in with an Oauth provider

dkrupp · 2025-01-08T13:17:11Z

web/server/codechecker_server/api/authentication.py

+        try:
+            date = datetime.datetime.now().strftime("%Y-%m-%d %H:%M:%S")
+            with DBSession(self.__config_db) as session:
+                session.execute("DELETE FROM oauth_sessions "


I guess we should not use direct SQL statements in codechecker code. You should use sqlachemy statments instead.
https://docs.sqlalchemy.org/en/20/tutorial/data_update.html#the-delete-sql-expression-construct

also see other examples from the codechecker codebase. then you can also remove the sqlite3 dependency.

Fixed, now uses sqlachemy statements for removal of expired sessions.

dkrupp · 2025-01-29T12:57:03Z

web/server/codechecker_server/api/authentication.py

+
+                LOG.info("State inserted successfully.")
+
+        except sqlite3.Error as e:


I think you should not use sqlite3 exception handling and in general direct sqlite3 dependency should be removed.

Resolved. Replaced all SQLite3-specific exceptions with standard exceptions. sqlite3 dependency has been removed

dkrupp · 2025-01-29T13:07:27Z

web/server/codechecker_server/api/authentication.py

@@ -167,6 +270,160 @@ def performLogin(self, auth_method, auth_string):
                    codechecker_api_shared.ttypes.ErrorCode.AUTH_DENIED,
                    msg)

+        elif auth_method == "oauth":
+            date_time = datetime.datetime.now().strftime("%Y-%m-%d %H:%M:%S")
+            provider, url = auth_string.split("@")


could you please comment above what the auth_string is expected to look like in this case? an example is good enough.

Fixed, added a comment with example of auth_string in case of OAuth authentication..

dkrupp · 2025-01-29T13:15:00Z

web/server/codechecker_server/api/authentication.py

+    @timeit
+    def createLink(self, provider):
+        """
+        For creating a autehntication link for OAuth for specified provider


Please describe better what the funtion is expected to do.
This function returns an URL given the provider.
It also creates a session and inserts something(insertDataOauth()) in the dabase.

Fixed, extended description of what the function does.

dkrupp · 2025-01-29T15:56:43Z

web/server/codechecker_server/session_manager.py

@@ -630,6 +669,65 @@ def create_session(self, auth_string):

        return local_session

+    def create_session_oauth(self, provider, username, token):


please call token as access_token. If I understand correctly here token is the oauth access token

dkrupp · 2025-01-29T15:57:26Z

web/server/codechecker_server/session_manager.py

+                     }
+
+        # Generate a new token and create a local session.
+        token = generate_session_token()


this is the codechecker session token, right?
maybe call is codechecker_session_token

dkrupp · 2025-01-31T15:04:32Z

web/server/codechecker_server/session_manager.py

+        # Generate a new token and create a local session.
+        token = generate_session_token()
+        # token_d is the access token of the oauth provider.
+        token_d = user_data.get('oauth_access_token')


why not call it oauth_access_token then?

dkrupp · 2025-01-31T15:08:09Z

web/server/codechecker_server/api/authentication.py

+
+            if allowed_users == ["*"] or username in allowed_users:
+                LOG.info("User %s is authorized.", username)
+                session = self.__manager.create_session_oauth(


this is the codechecker session, right?
call it session_codecheker

dkrupp · 2025-01-31T15:16:12Z

web/server/codechecker_server/api/authentication.py

+                code_verifier=pkce_verifier
+            )
+        # inserting data in database
+        self.insertDataOauth(state=state,


If I understand correctly, we insert a row in this table every time a user tries to login. Then when will a row be removed from this table. Or will it grow indefinitely?

bruntib

Thank you for this development. This is a great and important work!

bruntib · 2025-01-24T13:17:51Z

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)


The regex_groups could be a sub-point like OAuth authentication.

bruntib · 2025-01-27T22:53:52Z

web/api/authentication.thrift

+  // Returns list of providers for oauth for respective appearence of buttons.
+  list<string> getOauthProviders(),
+
+  // Create a link for the user to log in for github Oauth.


Not just github.

bruntib · 2025-01-27T23:00:19Z

web/server/codechecker_server/api/authentication.py

+                                "< DATETIME(\"" + date + "\")")
+                session.commit()
+            LOG.info("Expired state, validation codes removed successfully.")
+        except sqlite3.Error as e:


Why is it catching only sqlite3 errors? How about other database engines?

bruntib · 2025-01-27T23:01:41Z

web/server/codechecker_server/api/authentication.py

+
+                LOG.info("State inserted successfully.")
+
+        except sqlite3.Error as e:


Why is it catching only sqlite3 errors? How about other database engines?

bruntib · 2025-01-27T23:21:20Z

web/server/codechecker_server/api/authentication.py

+    @timeit
+    def createLink(self, provider):
+        """
+        For creating a autehntication link for OAuth for specified provider


Suggested change

For creating a autehntication link for OAuth for specified provider

For creating an authentication link for OAuth for specified provider.

bruntib · 2025-02-03T17:18:20Z

web/server/codechecker_server/api/authentication.py

+            user_info = None
+            username = None


Unnecessary initialization.

bruntib · 2025-02-03T17:26:33Z

web/server/codechecker_server/api/authentication.py

+            # if the provider is github it fetches primary email
+            # from another api endpoint to maintain username as email
+            # consistency between GitHub and other providers


What do we use "username" for in CodeChecker? If we want to identify authorized users by their e-mail addresses, then couldn't we use the "email" attribute of their user info? In this case it wouldn't be needed to distinguish GitHub here.

bruntib · 2025-02-03T22:50:55Z

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


What is the purpose of this protection? I would say that examples can be provided in the documentation, but the template config file can have empty values and OAuth method being disabled.

bruntib · 2025-02-03T22:54:23Z

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)


Is this sleep() operation intended or accidental?

bruntib · 2025-02-03T22:57:54Z

web/server/codechecker_server/session_manager.py

        self.last_access = last_access if last_access else datetime.now()

+    def get_access_token(self):


Where is this function used?

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

added documentation that explains the resoning behind fetching primar…

4393e55

…y email for GitHub

feyruzb force-pushed the branch-2-backup branch from 9630841 to 4393e55 Compare January 9, 2025 16:55

modified documentation and format checker for callback url

4b6cc2e

feyruzb force-pushed the branch-2-backup branch 2 times, most recently from 06d2ed8 to c2d4ba5 Compare January 15, 2025 15:59

now during the login process all providers send refresh token during …

e18d6ab

…the token fetching process

feyruzb force-pushed the branch-2-backup branch from c2d4ba5 to e18d6ab Compare January 15, 2025 16:04

dkrupp requested changes Jan 31, 2025

View reviewed changes

feyruzb added 3 commits February 3, 2025 14:12

fixed wrong comment about function functionality

3f4a491

removed sqlite3 dependency and related exceptions

e858a49

updated the version of api to 6.60.0

87c324b

feyruzb force-pushed the branch-2-backup branch from dfce37c to 87c324b Compare February 3, 2025 13:48

merge resolve attempt 1

337d86c

feyruzb force-pushed the branch-2-backup branch 2 times, most recently from ba4683b to 3417d4d Compare February 3, 2025 14:27

resolving merge consequences

a64fecb

feyruzb force-pushed the branch-2-backup branch from 3417d4d to a64fecb Compare February 3, 2025 14:31

attempt to fix tests

b2992f8

feyruzb force-pushed the branch-2-backup branch 7 times, most recently from d107d90 to fcc5cd5 Compare February 3, 2025 18:06

bruntib requested changes Feb 3, 2025

View reviewed changes

feyruzb force-pushed the branch-2-backup branch 2 times, most recently from 1f5985d to 45aa94d Compare February 4, 2025 15:47

fixing first batch of comments

2c535da

feyruzb force-pushed the branch-2-backup branch from 45aa94d to 2c535da Compare February 4, 2025 17:06

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

dkrupp left a comment

dkrupp Jan 8, 2025

feyruzb Feb 3, 2025 •

edited

Loading

dkrupp Jan 8, 2025

feyruzb Feb 4, 2025

dkrupp Jan 29, 2025

feyruzb Feb 3, 2025

dkrupp Jan 29, 2025

feyruzb Feb 4, 2025

dkrupp Jan 29, 2025

feyruzb Feb 4, 2025 •

edited

Loading

dkrupp Jan 29, 2025

dkrupp Jan 29, 2025

dkrupp Jan 31, 2025

dkrupp Jan 31, 2025

dkrupp Jan 31, 2025

bruntib left a comment

bruntib Jan 24, 2025

bruntib Jan 27, 2025

bruntib Jan 27, 2025

bruntib Jan 27, 2025

bruntib Jan 27, 2025

bruntib Feb 3, 2025

bruntib Feb 3, 2025

bruntib Feb 3, 2025

bruntib Feb 3, 2025

bruntib Feb 3, 2025


		LOG.info("State inserted successfully.")

		except sqlite3.Error as e:

		@@ -630,6 +669,65 @@ def create_session(self, auth_string):

		return local_session

		def create_session_oauth(self, provider, username, token):

	For creating a autehntication link for OAuth for specified provider
	For creating an authentication link for OAuth for specified provider.

		self.last_access = last_access if last_access else datetime.now()

		def get_access_token(self):

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

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feyruzb Feb 3, 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

feyruzb Feb 4, 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

bruntib left a 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 3, 2025 •

edited

Loading

feyruzb Feb 4, 2025 •

edited

Loading