PMM-14880 anonymous role#5170
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PMM-14880-rta-pmm-demo #5170 +/- ##
==========================================================
+ Coverage 47.79% 47.82% +0.02%
==========================================================
Files 410 411 +1
Lines 41974 42106 +132
==========================================================
+ Hits 20062 20136 +74
- Misses 19935 19981 +46
- Partials 1977 1989 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We need profiling on this one. |
|
|
||
| func (c *Client) getAnonymousRoleFromSettings(ctx context.Context, l *logrus.Entry) (bool, role) { | ||
| var settings frontendSettings | ||
| if err := c.do(ctx, http.MethodGet, "/api/frontend/settings", "", nil, nil, &settings); err != nil { |
There was a problem hiding this comment.
there is a separate method introduced getFrontendSettings for this
| } | ||
|
|
||
| // CurrentUser represents Grafana user payload. | ||
| type CurrentUser struct { |
There was a problem hiding this comment.
are all fields needed in these structs
| } | ||
|
|
||
| func (c *Client) getAnonymousRoleFromSettings(ctx context.Context, l *logrus.Entry) (bool, role) { | ||
| var settings frontendSettings |
There was a problem hiding this comment.
in this method the result from API endpoint /api/frontend/settings is parsed into frontendSettings struct, but the same endpoint call in getFrontendSettings is parsed into frontendSettingsFull struct. What is the purpose?
| IsDisabled: false, | ||
| IsExternal: false, | ||
| IsExtarnallySynced: false, | ||
| IsGrafanaAdmin: false, | ||
| IsGrafanaAdminExternallySynced: false, | ||
| Theme: "", |
There was a problem hiding this comment.
not sure these fields are required here
| } | ||
|
|
||
| var cErr *clientError | ||
| if !errors.As(errors.Cause(err), &cErr) || cErr.Code != http.StatusUnauthorized || hasAuthHeaders(authHeaders) { |
There was a problem hiding this comment.
- looks like no need to call
Cause()becauseerrors.As()iterates though errors chain - seems the condition shall be
if (errors.As(err, &cErr) && cErr.Code != http.StatusUnauthorized) || hasAuthHeaders(authHeaders) {
because cErr.Code has value only in case errors.As() call was successful
| case "/v1/users/current": | ||
| user, err := h.c.GetCurrentUser(req.Context(), authHeaders) | ||
| if err != nil { | ||
| h.l.WithError(err).Warn("failed to get current user") |
There was a problem hiding this comment.
in pmm-managed the following way for printing errors is used:
| h.l.WithError(err).Warn("failed to get current user") | |
| h.l.Errorf("failed to get current user: %v", err) |
| case "/v1/users/current/orgs": | ||
| orgs, err := h.c.GetCurrentUserOrgs(req.Context(), authHeaders) | ||
| if err != nil { | ||
| h.l.WithError(err).Warn("failed to get current user orgs") |
There was a problem hiding this comment.
| h.l.WithError(err).Warn("failed to get current user orgs") | |
| h.l.Errorf("failed to get current user orgs: %v", err) |
| err := c.do(ctx, http.MethodGet, "/api/user", "", authHeaders, nil, &m) | ||
| if err != nil { | ||
| var cErr *clientError | ||
| if anonymousEnabled && errors.As(errors.Cause(err), &cErr) && cErr.Code == http.StatusUnauthorized { |
There was a problem hiding this comment.
looks like errors.Cause() is not needed here
| if c.resolveAnonymousRole(settings) == none.String() { | ||
| // Anonymous mode is enabled but role is not configured. | ||
| // Return empty payload instead of Unauthorized. | ||
| return CurrentUser{}, nil |
There was a problem hiding this comment.
I think it is better to return an Unauthorized error here and decrease the number of variations (empty user w/ error vs empty user w/o error, etc). Anything (wrong credentials, other errors, not properly configured anonymous mode) - returns error
| if role == none.String() { | ||
| // Anonymous mode is enabled but role is not configured. | ||
| // Return empty payload instead of Unauthorized. | ||
| return []CurrentUserOrg{}, nil |
There was a problem hiding this comment.
the same regarding error as in GetCurrentUser
2 participants
PMM-14880
Allow org roles to anonymous users
Tied to percona/grafana#886