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
changed the current version of thrift API from 6.58.0 to 6.59.0
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

…erification

Discookie · 2024-10-16T11:29:03Z

web/server/codechecker_server/api/authentication.py

+            with DBSession(self.__config_db) as session:
+                state_db = session.query(StateCodes) \
+                    .filter(StateCodes.id == state_id) \
+                    .first().state


Expired states might not have been purged at this point - add an explicit check here for expires_at.

Discookie · 2024-10-16T12:14:50Z

web/server/codechecker_server/api/authentication.py

+                    .filter(StateCodes.state == new_state.state
+                            and
+                            StateCodes.expires_at == new_state.expires_at) \
+                    .first().id


It might be worthwhile to explicitly store other parameters of the "state" here as well - oauth provider, PKCE if added, etc.

Pkce problem

Discookie

What's the workflow for OAuth authentication via the command-line client? If the user logs in via CodeChecker cmd, and only OAuth providers are enabled, how will they get their CodeChecker session token?

Discookie · 2024-11-27T10:49:32Z

docs/web/authentication.md

+#### OAUTH Configuration options <a name="oauth-configuration-options"></a>
+  * `enabled`
+
+    Indicated if OAUTH authentication is enabled (required for any methods below)


Official capitalization is OAuth.

Discookie · 2024-11-27T10:51:33Z

docs/web/authentication.md

+      * `oauth_callback_url`
+
+          User will be redirected back to the provided link after login with returned data.
+          It should be constructed in that format `http://codechecker_path/login/OAuthLogin/provider` wher `provider` is the the name of the provider of OAuth and should match existing `provider_name`. The `oauth_callback_url` should also match the callback url specified in the config of your provider on their webpage.


If the path of the URL is always the same, wouldn't it make sense to ask for just the domain name? Or at least give a server warning when the path doesn't match the format it should be in.

Discookie · 2024-11-27T10:52:17Z

docs/web/authentication.md

+          "oauth_client_id": "client id",
+          "oauth_client_secret": "client secret",
+          "oauth_authorization_uri": "https://accounts.google.com/o/oauth2/auth",
+          "oauth_callback_url": "http://localhost:8080/login/provider",


(As an example of where a server warning would be useful, this callback is in the wrong format :D )

Discookie · 2024-11-27T10:57:21Z

web/server/codechecker_server/api/authentication.py

+                    codechecker_api_shared.ttypes.ErrorCode.AUTH_DENIED,
+                    "User info fetch failed.")
+
+            if provider == "github" and \


Are providers case-sensitive?

fixed, now whatever the performLogin() gets as provider is being transformed into lowercase

Discookie · 2024-11-27T10:57:56Z

web/server/codechecker_server/api/authentication.py

+            token = None
+            try:
+                # code_verifier_db is not supported for github provider
+                # if it will be fixed the code should adjust automatically


This should probably be in the documentation as well, along with how the code fetches Github email addresses instead of just usernames.

Discookie · 2024-11-27T11:02:32Z

web/server/config/server_config.json

+          "oauth_client_id" : "ExampleClientID",
+          "oauth_client_secret": "ExampleClientSecret",
+          "oauth_authorization_uri": "https://github.com/login/oauth/authorize",
+          "oauth_callback_url": "http://path_To_CodeChecker_Login_Page/provider",


This should also follow the correct format.

Discookie · 2024-11-27T11:07:06Z

web/server/vue-cli/src/views/OAuthLogin.vue

+        const baseUrl = window.location.href.replace("#", "");
+        const baseUrlOAuthId = `${baseUrl}&oauth_data_id=${oauth_data_id}`;
+        const baseMethod = `${baseUrlOAuthId}&code_challenge_method=${method}`;
+        const fullUrl = `${baseMethod}&code_challenge=${code_challenge}`;


This snippet looks suspect. Why are you reconstructing a URL yourself based on what's in the localStorage, when you are trying to pass the callback URL along as-is? In fact, why are these details stored at all, other than what is strictly necessary?

The stored details in localStorage, and all other variables are checked on the server side, and that is the only check that matters in the chain.

These details in the localStorage also become prone to the same thing that caused the mix-up attack - when someone dispatches a second OAuth login session, and they still didn't finish the first one, these stored details are overwritten.

fixed, The localStorage is not used anymore, the necessary arguments are stored on server side in database, the oauth_id is removed, the database querying is happening based on state ,

…ng data

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 5 times, most recently from 9256875 to 75072a3 Compare October 15, 2024 16:56

aded new column in auth_sessions for storing access_token for later v…

d6561b8

…erification

feyruzb force-pushed the branch-2-backup branch from 75072a3 to d6561b8 Compare October 15, 2024 17:00

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

21d3a8a

Discookie suggested changes Oct 16, 2024

View reviewed changes

feyruzb and others added 8 commits October 22, 2024 14:20

pkce problem

65adbbd

pkce implemented

1617633

Merge pull request #1 from feyruzb/pkce_problem

3d22dfb

Pkce problem

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

0ac3d52

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

c396d59

fixed npm warnings

0caea7e

protection against expired session use

39a3ad4

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

161106c

feyruzb force-pushed the branch-2-backup branch from 5eb341c to c474b4f Compare November 22, 2024 12:39

mix-up attack protection

3f774da

feyruzb force-pushed the branch-2-backup branch from c474b4f to 3f774da Compare November 25, 2024 12:50

Discookie suggested changes Nov 27, 2024

View reviewed changes

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

8cd4793

feyruzb force-pushed the branch-2-backup branch 4 times, most recently from 7149ef4 to 1287f13 Compare December 9, 2024 14:05

fixed unnescessary passing of state and code_challenge to client

ad5beec

feyruzb force-pushed the branch-2-backup branch from 1287f13 to ad5beec Compare December 9, 2024 14:10

removed oauth_data_id, was prone to attack, now uses state for queryi…

e3d6d7a

…ng data

feyruzb force-pushed the branch-2-backup branch from 545caa6 to e3d6d7a Compare December 9, 2024 15:12

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

Discookie Oct 16, 2024

feyruzb Nov 6, 2024

Discookie Oct 16, 2024

feyruzb Oct 28, 2024

Discookie left a comment

Discookie Nov 27, 2024

Discookie Nov 27, 2024

Discookie Nov 27, 2024

Discookie Nov 27, 2024

feyruzb Dec 9, 2024

feyruzb Dec 11, 2024

Discookie Nov 27, 2024

Discookie Nov 27, 2024

Discookie Nov 27, 2024

feyruzb Dec 11, 2024

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

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

Discookie 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