Better UX when role_attribute_path is not matched and role_attribute_strict is enabled

[rudeigerc]

What happened:

[auth.generic_oauth]
...
role_attribute_strict = true
role_attribute_path = contains(groups[*], 'GRAFANA_ADMIN') && 'Admin' || contains(groups[*], 'GRAFANA_EDITOR') && 'Editor' || contains(groups[*], 'GRAFANA_VIEWER') && 'Viewer'

When the configuration is set as above and the user info of the logged-in user does not match the role_attribute_path and role_attribute_strict is enabled, the user interface is shown as follows:

Actually, I expect it not to be a server error but an authentication failure.

What you expected to happen:

My expected behavior is a better UX such as 403 Forbidden, or a custom redirect URL, etc. instead of just showing server error and separating the error of invalid attribute path and authentication failure.

How to reproduce it (as minimally and precisely as possible):

Please see above.

Anything else we need to know?:

N/A

Environment:

• Grafana version: 8.0.5
• Data source type & version: N/A
• OS Grafana is installed on: N/A
• User OS & Browser: macOS 11.4 + Chrome 91.0.4472.164
• Grafana plugins: N/A
• Others: N/A

[rudeigerc]

I ran into the code and found out that if the error returned belongs to *social.Error it would redirect the login page with an alert box with the error message. Would such behavior be better in such a situation rather than just showing a server error page?

	userInfo, err := connect.UserInfo(client, token)
	if err != nil {
		var sErr *social.Error
		if errors.As(err, &sErr) {
			hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, sErr)
		} else {
			hs.handleOAuthLoginError(ctx, loginInfo, LoginError{
				HttpStatus:    http.StatusInternalServerError,
				PublicMessage: fmt.Sprintf("login.OAuthLogin(get info from %s)", name),
				Err:           err,
			})
		}
		return
	}

[Velociraptor85]

I would expect something like that, yes.
The above server error is quite cryptic and from a user standpoint its an unexpected message which leads to more questions.

[sebastianbuechler]

This would be perfect and the current error is misleading.

[candlerb]

FYI the code referenced above is here. However there are at least 7 different UserInfo functions in pkg/login/social/*_oauth.go that would need to be modified, and they seem to be inconsistent. For example, github_oauth.go returns models.RoleType("") if strictMode is true and there are no known roles, whereas generic_oauth.go returns errors.New("invalid role")

One solution would be to make this consistent: return empty role if valid role is not found, and the caller checks IsValid() on it and gives a suitable error message.

[candlerb]

This is still the case with Grafana 8.4.2. The screen shows "Server error" as per first post above. Logs show:

Feb 28 12:00:51 grafana grafana-server[60296]: logger=oauth t=2022-02-28T12:00:51.42+0000 lvl=info msg="state check" queryState=... cookieState=...
Feb 28 12:00:51 grafana grafana-server[60296]: logger=context t=2022-02-28T12:00:51.62+0000 lvl=eror msg="login.OAuthLogin(get info from generic_oauth)" error="invalid role"
Feb 28 12:00:51 grafana grafana-server[60296]: logger=context t=2022-02-28T12:00:51.62+0000 lvl=eror msg="Request Completed" method=GET path=/login/generic_oauth status=500 remote_addr=[...] time_ms=200 size=1372 referer=

Relevant config:

role_attribute_path = resource_access.grafana.roles &&
(contains(resource_access.grafana.roles[*], 'Admin') && 'Admin' ||
contains(resource_access.grafana.roles[*], 'Editor') && 'Editor' ||
contains(resource_access.grafana.roles[*], 'Viewer') && 'Viewer')

role_attribute_strict = true

This works fine as long as the user has one of those roles, but if this expression returns nil or empty string, the "Sadly something went wrong" message is displayed.

I think the current error message is fine if there is an error evaluating the JMESPath expression.

However if the expression is valid and returns nil or empty string then it would be much better if Grafana could display a page saying "Sorry, you are not authorized to access this application" or similar. The status code should be 403 not 500.

[ndemeshchenko]

any update on this from Grafana team?

[candlerb]

I re-checked with Grafana 10.1.1 and generic_oauth (talking to Hashicorp Vault as OIDC provider).

What I found was:

• If the role_attribute_path expression returns a null value (as in the example given above), or an unknown role string like "Wombat", then you get this error now:

  

  (Note that the exact message comes from pkg/login/social/generic_oauth.go - other social auth modules may behave differently)

• By experimentation: if the expression returns the string "None" then the user is allowed to login. It is possible to browse dashboards, but they can't see any data:

  

  (although if their account has been marked as a Grafana Admin in the database, they can still administer users). This doesn't appear to be a very useful access role, and in any case it doesn't appear to be documented, although I did find RoleNone in the source code in pkg/models/roletype/role_type.go.

In summary, I think the UI is now a bit better for unauthorized users than it used to be, as it's not showing a server error, but it still suggests that there's an IdP configuration error - as opposed to the user being at fault, by trying to access something they're not allowed to.

I would like to have a way to tell the user definitively "You are not authorized to access this application". To me it seems like a null/empty return from role_attribute_path should be interpreted this way. Otherwise, the role "None" could be changed to become a sentinel value which blocks web access entirely, in which case it could be returned as part of the access decision:

role_attribute_path = (resource_access.grafana.roles &&
(contains(resource_access.grafana.roles[*], 'Admin') && 'Admin' ||
contains(resource_access.grafana.roles[*], 'Editor') && 'Editor' ||
contains(resource_access.grafana.roles[*], 'Viewer') && 'Viewer')) || 'None'

[gelicia]

Hello, as you may have heard, we are transitioning away from using discussions to discuss feature requests. We are migrating this discussion to an issue and closing the discussion. The issue is #82971. Feel free to continue the discussion around this there. Thank you!