-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth: Add Generic oauth skip org role sync setting #62418
Auth: Add Generic oauth skip org role sync setting #62418
Conversation
@@ -936,10 +936,25 @@ The following table shows the OAuth provider's setting with the default value an | |||
| OAuth Provider | `oauth_skip_org_role_sync_update` | `skip_org_role_sync` | Behavior | | |||
| --- | --- | --- | --- | | |||
| GitLab | false | false | User organization roles are set with `defaultRole` and cannot be changed | | |||
| Github | true | false | User organization roles are set with `defaultRole` for GitLab, and Grafana Admins are set. For other providers, the synchronization is skipped, and the org role can be changed, along with other OAuth provider users' org roles. | | |||
| GitLab | true | false | User organization roles are set with `defaultRole` for GitLab, and Grafana Admins are set. For other providers, the synchronization is skipped, and the org role can be changed, along with other OAuth provider users' org roles. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: typo
default: | ||
return "OAuth" // FIXME: replace with "Unknown" and handle generic oauth as a case | ||
return "Unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now unknown instead of OAuth. Maybe we should still keep "oauth"?
Abivilant here
@@ -22,11 +22,12 @@ import ( | |||
"time" | |||
|
|||
"github.com/gobwas/glob" | |||
"github.com/prometheus/common/model" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go imports
@@ -39,8 +39,6 @@ interface OwnProps extends GrafanaRouteComponentProps<{ id: string }> { | |||
error?: UserAdminError; | |||
} | |||
|
|||
const SyncedOAuthLabels: string[] = ['OAuth']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can now remove this 👍
@@ -150,7 +150,7 @@ const UserListAdminPageUnConnected = ({ | |||
<Icon name="question-circle" /> | |||
</Tooltip> | |||
</th> | |||
<th style={{ width: '1%' }}></th> | |||
<th style={{ width: '1%' }}>Synced from</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added header for the columns, which provides the synced provider
@@ -392,3 +392,16 @@ Payload: | |||
... | |||
} | |||
``` | |||
|
|||
## Skip organization role sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eleijonmarck , the formatting of this text (it's in blue?) makes me think something is incorrect futher up in the doc. Please check Line 220. I think the quotes need to be a > symbol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -46,7 +46,9 @@ func GetAuthProviderLabel(authModule string) string { | |||
return "JWT" | |||
case AuthProxyAuthModule: | |||
return "Auth Proxy" | |||
case "oauth_generic_oauth": | |||
return "Generic OAuth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return "Generic OAuth" | |
return "OAuth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far we follow the configuration "pattern" of the [auth.provider] and then return that value. This would be the "outlier" for that pattern
Thought this would be good for the implementation when we want to have this more configured. I guess this is not really concerning as it is to the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's revisit this when we refactor!
67454e4
to
391bf9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
What is this feature?
This adds the
skip_org_role_sync
setting for the generic_oauth section of oauth.Fixes #
https://github.com/grafana/grafana-enterprise/issues/4235
Special notes for your reviewer:
demo -