integrated signum fetching and using it as optional username #4517
Conversation
feyruzb
force-pushed
the
feature/get-signum-as-username
branch
from
51cee0b to
4b45197
Compare
There was a problem hiding this comment.
Please add the documentation of this new field to this document
https://github.com/Ericsson/codechecker/blob/master/docs/web/authentication.md#oauth-authentication
Is the handling of groups intentionally added to this PR?
Can you please factor it out into another PR? (anyway the graph API implementation is already in the baseline code, so I am not sure we need it at all)
| user_groups_url = oauth_config["user_groups_url"] | ||
| response = oauth2_session.get(user_groups_url).json() | ||
| group_response = oauth2_session.get(user_groups_url) |
There was a problem hiding this comment.
I see that in this pull request you are also fetching group names from the id token, however the description of the pull request is only describing that this PR will contain signum/email configuration.
Can you please place this group fetching code in another PR?
BTW Don't we use the graph APi to fetch the groups?
It's here
So why we need to take the groups from the ID token at all?
There was a problem hiding this comment.
I don't use id token for fetching groups, that is still using Microsoft graph API but groups handling as it was before wasn't secure enough and could fail without notifying us.
That is why this more secure way was introduced.
As per request, I will create a separate pr for upgrading group fetching.
| if group_name: | ||
| groups.append(group_name) | ||
|
|
||
| if oauth_config["user_info_mapping"]["username"] == "signum": |
There was a problem hiding this comment.
This will crash, if the "user_info_mapping" field is missing.
I think if it is missing it should default to the email address.
feyruzb
force-pushed
the
feature/get-signum-as-username
branch
from
3402823 to
ee3b12e
Compare
docs/web/authentication.md
Outdated
| Supported Providers and Options: | ||
|
|
||
| `github` | ||
| * Key: `login` |
There was a problem hiding this comment.
Please only accept username/email as a configuration and give a codechecker error if the configured value is something else.
feyruzb
force-pushed
the
feature/get-signum-as-username
branch
2 times, most recently
from
603aa17 to
fca042d
Compare
feyruzb
force-pushed
the
feature/get-signum-as-username
branch
from
fca042d to
0dd2670
Compare
docs/web/authentication.md
Outdated
| "user_emails_url": "https://api.github.com/user/emails", | ||
| "scope": "openid email profile", |
There was a problem hiding this comment.
I am getting this error at authentication
User info fetch failed: "jwks_url'
I guess it is missing from the config file
There was a problem hiding this comment.
But why is it still missing from this authentication md .json example?
docs/web/authentication.md
Outdated
| "user_emails_url": "https://api.github.com/user/emails", | ||
| "scope": "openid email profile", |
There was a problem hiding this comment.
But why is it still missing from this authentication md .json example?
integrated fetching of signum from
id_tokenso it can be used as username for Microsoft.Can be configured in configuration file what will be username:
"username": "mail"or"username": "signum".