From 903e7033f4bc81413c939e1303f12dbd5da8bc44 Mon Sep 17 00:00:00 2001 From: Michael Desa Date: Wed, 20 Dec 2017 14:20:24 -0800 Subject: [PATCH 1/9] Give SuperAdmin DefaultRole on PUT /me Previously, SuperAdmins were given the admin role in an organization when they switched into it (via a PUT to /me). This is undesireable for the comonitoring organization. This PR gives SuperAdmins the default role for the organization when they switch into it. --- integrations/server_test.go | 2 +- server/me.go | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/integrations/server_test.go b/integrations/server_test.go index ec7ce42793..e87c4f42f5 100644 --- a/integrations/server_test.go +++ b/integrations/server_test.go @@ -1301,7 +1301,7 @@ func TestServer(t *testing.T) { "organization": "default" }, { - "name": "admin", + "name": "viewer", "organization": "1" } ], diff --git a/server/me.go b/server/me.go index 708a7b9dc4..52a36c6ff0 100644 --- a/server/me.go +++ b/server/me.go @@ -11,7 +11,6 @@ import ( "github.com/influxdata/chronograf" "github.com/influxdata/chronograf/oauth2" "github.com/influxdata/chronograf/organizations" - "github.com/influxdata/chronograf/roles" ) type meLinks struct { @@ -96,7 +95,7 @@ func (s *Service) UpdateMe(auth oauth2.Authenticator) func(http.ResponseWriter, } // validate that the organization exists - _, err = s.Store.Organizations(serverCtx).Get(serverCtx, chronograf.OrganizationQuery{ID: &req.Organization}) + org, err := s.Store.Organizations(serverCtx).Get(serverCtx, chronograf.OrganizationQuery{ID: &req.Organization}) if err != nil { Error(w, http.StatusBadRequest, err.Error(), s.Logger) return @@ -151,8 +150,8 @@ func (s *Service) UpdateMe(auth oauth2.Authenticator) func(http.ResponseWriter, // If the user is a super admin give them an admin role in the // requested organization. u.Roles = append(u.Roles, chronograf.Role{ - Organization: req.Organization, - Name: roles.AdminRoleName, + Organization: org.ID, + Name: org.DefaultRole, }) if err := s.Store.Users(serverCtx).Update(serverCtx, u); err != nil { unknownErrorWithMessage(w, err, s.Logger) From 0cc30d927f10fd5320db1f9e2ec9ca1dbdb78201 Mon Sep 17 00:00:00 2001 From: Michael Desa Date: Wed, 20 Dec 2017 15:17:08 -0800 Subject: [PATCH 2/9] Prevent SuperAdmin from modifying their own status Previously it was possible for SuperAdmins to remove their own status. This could create an application state where there were no super admins. This is not an acceptable application state. --- integrations/server_test.go | 53 +++++++++++++++ server/users.go | 15 +++++ server/users_test.go | 127 ++++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+) diff --git a/integrations/server_test.go b/integrations/server_test.go index ec7ce42793..c28943324a 100644 --- a/integrations/server_test.go +++ b/integrations/server_test.go @@ -1241,6 +1241,59 @@ func TestServer(t *testing.T) { }`, }, }, + { + name: "PATCH /users/1", + subName: "SuperAdmin modifying their own status", + fields: fields{ + Users: []chronograf.User{ + { + ID: 1, // This is artificial, but should be reflective of the users actual ID + Name: "billibob", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + Roles: []chronograf.Role{ + { + Name: "admin", + Organization: "default", + }, + }, + }, + }, + }, + args: args{ + server: &server.Server{ + GithubClientID: "not empty", + GithubClientSecret: "not empty", + }, + method: "PATCH", + path: "/chronograf/v1/users/1", + payload: map[string]interface{}{ + "id": "1", + "superAdmin": false, + "roles": []interface{}{ + map[string]interface{}{ + "name": "admin", + "organization": "default", + }, + }, + }, + principal: oauth2.Principal{ + Organization: "default", + Subject: "billibob", + Issuer: "github", + }, + }, + wants: wants{ + statusCode: http.StatusUnauthorized, + body: ` +{ + "code": 401, + "message": "user cannot modify their own SuperAdmin status" +} +`, + }, + }, { name: "PUT /me", subName: "Change SuperAdmins current organization to org they dont belong to", diff --git a/server/users.go b/server/users.go index eb425d0a58..1f0b71d31c 100644 --- a/server/users.go +++ b/server/users.go @@ -273,6 +273,21 @@ func (s *Service) UpdateUser(w http.ResponseWriter, r *http.Request) { return } + // Don't allow SuperAdmins to modify their own SuperAdmin status. + // Allowing them to do so could result in an application where there + // are no super admins. + ctxUser, ok := hasUserContext(ctx) + if !ok { + Error(w, http.StatusInternalServerError, "failed to retrieve user from context", s.Logger) + return + } + // If the user being updated is the user making the request and they are + // changing their SuperAdmin status, return an unauthorized error + if ctxUser.ID == u.ID && req.SuperAdmin != u.SuperAdmin { + Error(w, http.StatusUnauthorized, "user cannot modify their own SuperAdmin status", s.Logger) + return + } + if err := setSuperAdmin(ctx, req, u); err != nil { Error(w, http.StatusUnauthorized, err.Error(), s.Logger) return diff --git a/server/users_test.go b/server/users_test.go index fe4711bef2..7641d4922c 100644 --- a/server/users_test.go +++ b/server/users_test.go @@ -667,6 +667,13 @@ func TestService_UpdateUser(t *testing.T) { "http://any.url", nil, ), + userKeyUser: &chronograf.User{ + ID: 0, + Name: "coolUser", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: false, + }, user: &userRequest{ ID: 1336, Roles: []chronograf.Role{ @@ -715,6 +722,13 @@ func TestService_UpdateUser(t *testing.T) { "http://any.url", nil, ), + userKeyUser: &chronograf.User{ + ID: 0, + Name: "coolUser", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: false, + }, user: &userRequest{ ID: 1336, Roles: []chronograf.Role{ @@ -786,6 +800,119 @@ func TestService_UpdateUser(t *testing.T) { wantContentType: "application/json", wantBody: `{"code":422,"message":"duplicate organization \"1\" in roles"}`, }, + { + name: "SuperAdmin modifying their own SuperAdmin Status - user missing from context", + fields: fields{ + Logger: log.New(log.DebugLevel), + UsersStore: &mocks.UsersStore{ + UpdateF: func(ctx context.Context, user *chronograf.User) error { + return nil + }, + GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + switch *q.ID { + case 1336: + return &chronograf.User{ + ID: 1336, + Name: "bobbetta", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + Roles: []chronograf.Role{ + { + Name: roles.EditorRoleName, + Organization: "1", + }, + }, + }, nil + default: + return nil, fmt.Errorf("User with ID %d not found", *q.ID) + } + }, + }, + }, + args: args{ + w: httptest.NewRecorder(), + r: httptest.NewRequest( + "PATCH", + "http://any.url", + nil, + ), + user: &userRequest{ + ID: 1336, + SuperAdmin: false, + Roles: []chronograf.Role{ + { + Name: roles.AdminRoleName, + Organization: "1", + }, + }, + }, + }, + id: "1336", + wantStatus: http.StatusInternalServerError, + wantContentType: "application/json", + wantBody: `{"code":500,"message":"failed to retrieve user from context"}`, + }, + { + name: "SuperAdmin modifying their own SuperAdmin Status", + fields: fields{ + Logger: log.New(log.DebugLevel), + UsersStore: &mocks.UsersStore{ + UpdateF: func(ctx context.Context, user *chronograf.User) error { + return nil + }, + GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + switch *q.ID { + case 1336: + return &chronograf.User{ + ID: 1336, + Name: "bobbetta", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + Roles: []chronograf.Role{ + { + Name: roles.EditorRoleName, + Organization: "1", + }, + }, + }, nil + default: + return nil, fmt.Errorf("User with ID %d not found", *q.ID) + } + }, + }, + }, + args: args{ + w: httptest.NewRecorder(), + r: httptest.NewRequest( + "PATCH", + "http://any.url", + nil, + ), + user: &userRequest{ + ID: 1336, + SuperAdmin: false, + Roles: []chronograf.Role{ + { + Name: roles.AdminRoleName, + Organization: "1", + }, + }, + }, + userKeyUser: &chronograf.User{ + ID: 1336, + Name: "coolUser", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + }, + }, + id: "1336", + wantStatus: http.StatusUnauthorized, + wantContentType: "application/json", + wantBody: `{"code":401,"message":"user cannot modify their own SuperAdmin status"}`, + }, { name: "Update a SuperAdmin's Roles - without super admin context", fields: fields{ From 66a635e84e0edd6597611b81356d5b4614a18673 Mon Sep 17 00:00:00 2001 From: Alex P Date: Wed, 20 Dec 2017 16:00:34 -0800 Subject: [PATCH 3/9] Add styles for slide toggle disabled state --- ui/src/style/components/slide-toggle.scss | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ui/src/style/components/slide-toggle.scss b/ui/src/style/components/slide-toggle.scss index 09f9b9c533..eec03eb93a 100644 --- a/ui/src/style/components/slide-toggle.scss +++ b/ui/src/style/components/slide-toggle.scss @@ -29,6 +29,7 @@ } } +/* Active State */ .slide-toggle.active .slide-toggle--knob { background-color: $c-rainforest; transform: translate(100%,-50%); @@ -37,6 +38,23 @@ background-color: $c-honeydew; } +/* Disabled State */ +.slide-toggle.disabled { + &, + &:hover, + &.active, + &.active:hover { + background-color: $g6-smoke; + cursor: not-allowed; + } + .slide-toggle--knob, + &:hover .slide-toggle--knob, + &.active .slide-toggle--knob, + &.active:hover .slide-toggle--knob { + background-color: $g3-castle; + } +} + /* Size Modifiers */ .slide-toggle { From e53b702c318f3aeefd9657c79ee65cea695fa946 Mon Sep 17 00:00:00 2001 From: Alex P Date: Wed, 20 Dec 2017 16:04:37 -0800 Subject: [PATCH 4/9] Add disabled state logic to SlideToggle component --- ui/src/shared/components/SlideToggle.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/ui/src/shared/components/SlideToggle.js b/ui/src/shared/components/SlideToggle.js index 399fae2e0c..fa84675f86 100644 --- a/ui/src/shared/components/SlideToggle.js +++ b/ui/src/shared/components/SlideToggle.js @@ -14,7 +14,11 @@ class SlideToggle extends Component { } handleClick = () => { - const {onToggle} = this.props + const {onToggle, disabled} = this.props + + if (disabled) { + return + } this.setState({active: !this.state.active}, () => { onToggle(this.state.active) @@ -22,15 +26,15 @@ class SlideToggle extends Component { } render() { - const {size} = this.props + const {size, disabled} = this.props const {active} = this.state - const classNames = active - ? `slide-toggle slide-toggle__${size} active` - : `slide-toggle slide-toggle__${size}` + const className = `slide-toggle slide-toggle__${size} ${active + ? 'active' + : null} ${disabled ? 'disabled' : null}` return ( -
+
) @@ -46,6 +50,7 @@ SlideToggle.propTypes = { active: bool, size: string, onToggle: func.isRequired, + disabled: bool, } export default SlideToggle From ac43e3b50bcdd0c068e9f0255c7aa4b4082ad7ba Mon Sep 17 00:00:00 2001 From: Alex P Date: Wed, 20 Dec 2017 16:05:07 -0800 Subject: [PATCH 5/9] Disable SuperAdmin slide toggle on users own row --- ui/src/admin/components/chronograf/UsersTableRow.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/src/admin/components/chronograf/UsersTableRow.js b/ui/src/admin/components/chronograf/UsersTableRow.js index 283b069599..561ca99684 100644 --- a/ui/src/admin/components/chronograf/UsersTableRow.js +++ b/ui/src/admin/components/chronograf/UsersTableRow.js @@ -59,6 +59,7 @@ const UsersTableRow = ({ active={user.superAdmin} onToggle={onChangeSuperAdmin(user)} size="xs" + disabled={userIsMe} /> From 023705d9abebb69a056170d9ec1987e0aacc7f83 Mon Sep 17 00:00:00 2001 From: Jared Scheib Date: Wed, 20 Dec 2017 16:20:12 -0800 Subject: [PATCH 6/9] Notify user what role & organization they switched to Show that notification for 4 seconds. Change copy from 'signed in' to 'logged in'. Remove concept of userHasRoleInOrg since now obsolete. --- .../chronograf/OrganizationsTable.js | 11 ---- .../chronograf/OrganizationsTableRow.js | 17 +---- ui/src/shared/actions/auth.js | 14 ++-- ui/src/shared/constants/index.js | 1 + ui/src/sources/containers/SourcePage.js | 66 ++++++++++--------- 5 files changed, 48 insertions(+), 61 deletions(-) diff --git a/ui/src/admin/components/chronograf/OrganizationsTable.js b/ui/src/admin/components/chronograf/OrganizationsTable.js index e633af7c5c..272d574878 100644 --- a/ui/src/admin/components/chronograf/OrganizationsTable.js +++ b/ui/src/admin/components/chronograf/OrganizationsTable.js @@ -44,7 +44,6 @@ class OrganizationsTable extends Component { currentOrganization, authConfig: {superAdminNewUsers}, onChangeAuthConfig, - me, } = this.props const {isCreatingOrganization} = this.state @@ -93,7 +92,6 @@ class OrganizationsTable extends Component { onRename={onRenameOrg} onChooseDefaultRole={onChooseDefaultRole} currentOrganization={currentOrganization} - userHasRoleInOrg={!!me.organizations.find(o => org.id === o.id)} /> )} @@ -146,14 +144,5 @@ OrganizationsTable.propTypes = { authConfig: shape({ superAdminNewUsers: bool, }), - me: shape({ - organizations: arrayOf( - shape({ - id: string.isRequired, - name: string.isRequired, - defaultRole: string.isRequired, - }) - ), - }), } export default OrganizationsTable diff --git a/ui/src/admin/components/chronograf/OrganizationsTableRow.js b/ui/src/admin/components/chronograf/OrganizationsTableRow.js index 2f20bb9f3a..93690854a2 100644 --- a/ui/src/admin/components/chronograf/OrganizationsTableRow.js +++ b/ui/src/admin/components/chronograf/OrganizationsTableRow.js @@ -39,19 +39,9 @@ class OrganizationsTableRow extends Component { } handleChangeCurrentOrganization = async () => { - const { - router, - links, - meChangeOrganization, - organization, - userHasRoleInOrg, - } = this.props + const {router, links, meChangeOrganization, organization} = this.props - await meChangeOrganization( - links.me, - {organization: organization.id}, - {userHasRoleInOrg} - ) + await meChangeOrganization(links.me, {organization: organization.id}) router.push('') } @@ -204,7 +194,7 @@ class OrganizationsTableRow extends Component { } } -const {arrayOf, bool, func, shape, string} = PropTypes +const {arrayOf, func, shape, string} = PropTypes OrganizationsTableRow.propTypes = { organization: shape({ @@ -235,7 +225,6 @@ OrganizationsTableRow.propTypes = { }), }), meChangeOrganization: func.isRequired, - userHasRoleInOrg: bool.isRequired, } OrganizationsTableRowDeleteButton.propTypes = { diff --git a/ui/src/shared/actions/auth.js b/ui/src/shared/actions/auth.js index 06b480f5e0..79aeaa510c 100644 --- a/ui/src/shared/actions/auth.js +++ b/ui/src/shared/actions/auth.js @@ -5,6 +5,8 @@ import {linksReceived} from 'shared/actions/links' import {publishAutoDismissingNotification} from 'shared/dispatchers' import {errorThrown} from 'shared/actions/errors' +import {LONG_NOTIFICATION_DISMISS_DELAY} from 'shared/constants' + export const authExpired = auth => ({ type: 'AUTH_EXPIRED', payload: { @@ -84,18 +86,20 @@ export const getMeAsync = ({shouldResetMe = false} = {}) => async dispatch => { export const meChangeOrganizationAsync = ( url, - organization, - {userHasRoleInOrg = true} = {} + organization ) => async dispatch => { dispatch(meChangeOrganizationRequested()) try { const {data: me, auth, logoutLink} = await updateMeAJAX(url, organization) + const currentRole = me.roles.find( + r => r.organization === me.currentOrganization.id + ) dispatch( publishAutoDismissingNotification( 'success', - `Now signed in to ${me.currentOrganization.name}${userHasRoleInOrg - ? '' - : ' with Admin role.'}` + `Now logged in to '${me.currentOrganization + .name}' as '${currentRole.name}'`, + LONG_NOTIFICATION_DISMISS_DELAY ) ) dispatch(meChangeOrganizationCompleted()) diff --git a/ui/src/shared/constants/index.js b/ui/src/shared/constants/index.js index 1c095d4202..0bc9e48ac2 100644 --- a/ui/src/shared/constants/index.js +++ b/ui/src/shared/constants/index.js @@ -387,6 +387,7 @@ export const PRESENTATION_MODE_ANIMATION_DELAY = 0 // In milliseconds. export const PRESENTATION_MODE_NOTIFICATION_DELAY = 2000 // In milliseconds. export const SHORT_NOTIFICATION_DISMISS_DELAY = 2000 // in milliseconds +export const LONG_NOTIFICATION_DISMISS_DELAY = 4000 // in milliseconds export const REVERT_STATE_DELAY = 1500 // ms diff --git a/ui/src/sources/containers/SourcePage.js b/ui/src/sources/containers/SourcePage.js index d6b470a073..7d0d792e00 100644 --- a/ui/src/sources/containers/SourcePage.js +++ b/ui/src/sources/containers/SourcePage.js @@ -10,6 +10,7 @@ import { import {publishNotification} from 'shared/actions/notifications' import {connect} from 'react-redux' +import Notifications from 'shared/components/Notifications' import SourceForm from 'src/sources/components/SourceForm' import FancyScrollbar from 'shared/components/FancyScrollbar' import SourceIndicator from 'shared/components/SourceIndicator' @@ -200,42 +201,45 @@ class SourcePage extends Component { } return ( -
- {isInitialSource - ? null - :
-
-
-
-

- {editMode ? 'Edit Source' : 'Add a New Source'} -

-
-
- +
+ +
+ {isInitialSource + ? null + :
+
+
+
+

+ {editMode ? 'Edit Source' : 'Add a New Source'} +

+
+
+ +
-
-
} - -
-
-
-
- +
} + +
+
+
+
+ +
-
- + +
) } From 979ad34ed04ed1dcd7430de5d208914872534fe8 Mon Sep 17 00:00:00 2001 From: Michael Desa Date: Wed, 20 Dec 2017 16:34:31 -0800 Subject: [PATCH 7/9] Check specific case when SA changes their status SA - SuperAdmin --- server/users.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/users.go b/server/users.go index 1f0b71d31c..3d82406d81 100644 --- a/server/users.go +++ b/server/users.go @@ -283,7 +283,7 @@ func (s *Service) UpdateUser(w http.ResponseWriter, r *http.Request) { } // If the user being updated is the user making the request and they are // changing their SuperAdmin status, return an unauthorized error - if ctxUser.ID == u.ID && req.SuperAdmin != u.SuperAdmin { + if ctxUser.ID == u.ID && u.SuperAdmin == true && req.SuperAdmin == false { Error(w, http.StatusUnauthorized, "user cannot modify their own SuperAdmin status", s.Logger) return } From 0771d708eab703379a8bd2dfa7ce773e6eee040b Mon Sep 17 00:00:00 2001 From: Nathan Haugo Date: Wed, 20 Dec 2017 16:56:58 -0800 Subject: [PATCH 8/9] Add changelog for 2369 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17cb1cf7f8..99d09dfc78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ### UI Improvements ### Bug Fixes +## v1.4.0.0-rc2 [unreleased] +### Bug Fixes +1. [#2639](https://github.com/influxdata/chronograf/pull/2639): Prevent SuperAdmin from modifying their own status + ## v1.4.0.0-rc1 [2017-12-19] ### Features 1. [#2593](https://github.com/influxdata/chronograf/pull/2593): Add option to use files for dashboards, organizations, data sources, and kapacitors From 2da402fa53b31c0f1a23dd390679b7934909fb82 Mon Sep 17 00:00:00 2001 From: Jared Scheib Date: Wed, 20 Dec 2017 17:52:41 -0800 Subject: [PATCH 9/9] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99d09dfc78..a5c4c9dfd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,12 @@ ### Bug Fixes ## v1.4.0.0-rc2 [unreleased] +### UI Improvements +1. [#2632](https://github.com/influxdata/chronograf/pull/2632): Tell user which organization they switched into and what role they have whenever they switch, including on Source Page + ### Bug Fixes 1. [#2639](https://github.com/influxdata/chronograf/pull/2639): Prevent SuperAdmin from modifying their own status +1. [#2632](https://github.com/influxdata/chronograf/pull/2632): Give SuperAdmin DefaultRole when switching to organization where they have no role ## v1.4.0.0-rc1 [2017-12-19] ### Features