Skip to content
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

Browse as Parse.User #1523

Closed
wants to merge 121 commits into from
Closed

Conversation

NinoZX
Copy link
Contributor

@NinoZX NinoZX commented Apr 11, 2020

Browse as Parse.User

  • Adds Browse as Parse.User dialog
  • Login and browse data as Parse.User
  • Switch logged in Parse.User
  • Toggle using master key when logged in as Parse.User
  • Logout as Parse.User

For starters, let me just explain our motivation to develop this feature.
When setting ACL, either on class level or role level, and newly introduced ProtectedFileds,
to test your settings, you would need to test it with some client implementation outside Parse Dashboard.
So to speed up and simplify testing your ACL settings, we developed the option to browse your data as logged in Parse.User inside of Parse Dasbhoard.

To explain it easier we will use default Parse model, Role and User classes.
We have two basic roles: "admin" and "user". Along with that we have four users, one for "admin" role and the other three for "user" role.

Going from that, as we browse our users, we see our list of all users (both of them) as we browse data by default with master key.
So to show off our new feature, we added class level permission for admin role so it can browse all users with all columns available.
Also we added permission for user2 so it can read all the rows, but it has some protected fields set.
As we will be changing users, three new options (switch user, toggle master key and logout) will appear in submenu of Security menu item.
Toggling master key just overwrites logged in user, so you can see user permissions even faster when testing permissions.

To understand everything better, please see it in action:

s3_amazonaws_com-6B7339QbT2

P.S. please dismiss chrome warnings for username/password.
P.P.S. please visit https://recordit.co/6B7339QbT2 for better quality video of using example.

NinoZX added 6 commits March 31, 2020 13:36
Expand / Collapse TextArea fields on EditRowDialog;
Fix for live reload data on EditRowDialog when data is updated on server
* origin/master:
  Update parse to the latest version 🚀 (parse-community#1507)
  Update webpack to the latest version 🚀 (parse-community#1504)
  Update marked to the latest version 🚀 (parse-community#1503)
  Update babel-loader to the latest version 🚀 (parse-community#1501)
  Update react to the latest version 🚀 (parse-community#1500)
  Update marked to the latest version 🚀 (parse-community#1499)
  Update file-loader to the latest version 🚀 (parse-community#1498)
  Update commander to the latest version 🚀 (parse-community#1496)
  Update regenerator-runtime to the latest version 🚀 (parse-community#1495)
  Update eslint-plugin-react to the latest version 🚀 (parse-community#1490)
  Update eslint-plugin-jest to the latest version 🚀 (parse-community#1489)
  Update cross-env to the latest version 🚀 (parse-community#1488)
@TomWFox
Copy link
Contributor

TomWFox commented Apr 15, 2020

This seems like a really neat feature, I can't fully evaluate this PR but I have a couple thoughts on the UI...

  1. I think there should it should be more obvious that you are browsing as a user (otherwise it might get quite confusing & worrying if you forget you are browsing as a user and think all of your objects have disappeared). I'm thinking a prominent badge and maybe a change in the background colour of the browser nav bar...

Screenshot 2020-04-15 at 12 18 04

  1. The browse with Master Key option is a bit confusing, I think it would work better with a switch.
  2. I'm not super fussed about this but maybe 'Browse as user' would be nicer than 'Browse as Parse.User'

@mtrezza
Copy link
Member

mtrezza commented May 8, 2020

Interesting feature!

  • I agree with @TomWFox, browsing as user should be clearly indicated as it's a special, non-default browsing mode. How about adding a new menu button before "Security" with a user icon and you turn the button orange when the "browse as user" mode in on?
  • I wouldn't use the terminology login / logout in the menu to avoid confusion with the "user" that is logged into the dashboard. I would also avoid displaying the "browsing username" with the same or higher prominence than the "dashboard username" in the top left corner.
  • Why does it require to type in the password of a user, when you are actually limiting what you see in the browser? As I understand this feature, you log into the dashboard with highest level privileges and then for debug purpose temporarily downgrade your privileges regarding what you see in the Data Browser. In a real world scenario I also wouldn't know the user's password.

@NinoZX
Copy link
Contributor Author

NinoZX commented May 20, 2020

Hi guys!

Thank you all for your feedback and my apologies for getting back to you this late.

@TomWFox

I think there should it should be more obvious that you are browsing as a user (otherwise it might get quite confusing & worrying if you forget you are browsing as a user and think all of your objects have disappeared). I'm thinking a prominent badge and maybe a change in the background colour of the browser nav bar...

I agree with you, was thinking about that myself, but decided to go with this simpler implementation which didn't request many changes in design. But rather then setting some badge or colour whole nav bar, I am going to use @mtrezza 's proposition of having separate menu item with user icon. Only question here is how to call this menu item? I was thinking of ACL test for example, but am willing to listen to other suggestions. Also use of orange, or some other distinct colour, to colour this new menu item will be used when user is logged in.

The browse with Master Key option is a bit confusing, I think it would work better with a switch.

Once again, was thinking about this earlier, but for development and design conveniences went with easier way. Will implement it this week.

I'm not super fussed about this but maybe 'Browse as user' would be nicer than 'Browse as Parse.User'

Wasn't sure if just user would be clear enough, but will remove Parse label if you think it is ok to shorten it.

@mtrezza

I agree with @TomWFox, browsing as user should be clearly indicated as it's a special, non-default browsing mode. How about adding a new menu button before "Security" with a user icon and you turn the button orange when the "browse as user" mode in on?

As mentioned in my answer before, I decided with implementing this your way. Of course if @TomWFox that is ok with you and others reviewing this PR.

I wouldn't use the terminology login / logout in the menu to avoid confusion with the "user" that is logged into the dashboard. I would also avoid displaying the "browsing username" with the same or higher prominence than the "dashboard username" in the top left corner.

Ok, do you have some other suggestion which labels to use? Maybe as I mentioned above, Test ACL?

Why does it require to type in the password of a user, when you are actually limiting what you see in the browser? As I understand this feature, you log into the dashboard with highest level privileges and then for debug purpose temporarily downgrade your privileges regarding what you see in the Data Browser. In a real world scenario I also wouldn't know the user's password.

Well I am using Parse.User.logIn to set current user. This way without much code changes, everything is set for testing. And yes, as you said, you downgrade your privileges temporary so you can test some user/role privileges. In a real world scenario I presume you won't test every user's privilege, rather just roles with test users, which are not some real users. This feature (from my point of view) is not meant to be used as production tool for testing every one user. I would use this as dev and preproduction tool for easily testing roles and specific example users permissions.

Thank you once again for your feedback @TomWFox @mtrezza. I am going to start implementing things mentioned above tomorrow and wait for your feedback on non-decided details.

@mtrezza
Copy link
Member

mtrezza commented May 20, 2020

Well I am using Parse.User.logIn to set current user.

Which requests are then made as that "browsing user" instead of with master key? In other words, which sections in the dashboard are affected by assuming another user's identity?

I was thinking of ACL test for example

While "test(ing)" is what you do afterwards, the actual functionality is assuming another user's identity. That is usually called "assume role/user" (AWS IAM, Salesforce). Since we don't want to conflate with the dashboard user, maybe you want to use something like glasses, as in "you look at the data through the other user's eyes".

@NinoZX
Copy link
Contributor Author

NinoZX commented May 26, 2020

@mtrezza

Which requests are then made as that "browsing user" instead of with master key? In other words, which sections in the dashboard are affected by assuming another user's identity?

Please navigate here where you can see newly added userMasterKey state property. If you follow changes down the file you will notice login, logout and toggleMasterKeyUsage. If you inspect carefully you can see that useMasterKey is true by default and is only set to false when you assume role of some other user.
Following changes on mentioned file above, look for fetchParseData function which was always using useMasterKey: true before. Now it uses newly introduced userMasterKey state which can be toggled by user (wrote a comment for you there so you can easily spot it).
All other API calls from dashboard remain the same and are not affected by this changes.

While "test(ing)" is what you do afterwards, the actual functionality is assuming another user's identity. That is usually called "assume role/user" (AWS IAM, Salesforce). Since we don't want to conflate with the dashboard user, maybe you want to use something like glasses, as in "you look at the data through the other user's eyes".

Will implement your example "assume role/user" with glasses icon.

@NinoZX
Copy link
Contributor Author

NinoZX commented May 26, 2020

Will implement your example "assume role/user" with glasses icon.

As glasses icon is not available from current pool of icons, I have used users-solid icon.
Also I sticked up with label Browse rather then Assume.

Toggle new type added - HIDE_LABELS;
@mtrezza
Copy link
Member

mtrezza commented May 26, 2020

Have you looked into how much more work it would be to apply the useMasterKey option also to the writing commands? Since it seems to just require replacing useMasterKey: true with the option variable. That would eliminate any confusion that you read as one user but write as another user. And you'd fully "assume" the user role, so that would also eliminate any confusion about the terminology.

If this only affects what you see in the browser without impact on write permission, then maybe the idea of "assuming role / browsing as user" implies a bit too much. It is more like a Filter, for which we already have a menu option. Maybe it makes sense to integrate it there?

NinoZX added 3 commits May 27, 2020 12:20
* master:
  Release 2.1.0 (parse-community#1516)
  Update puppeteer to the latest version 🚀 (parse-community#1531)
  Update semver to the latest version 🚀 (parse-community#1527)
  Update semver to the latest version 🚀 (parse-community#1526)
  Update semver to the latest version 🚀 (parse-community#1525)
  Update semver to the latest version 🚀 (parse-community#1524)
Enabled using useMasteKey property for save actions and other actions
@NinoZX
Copy link
Contributor Author

NinoZX commented May 27, 2020

@mtrezza I have implemented using useMasterKey everywhere inside Browser component. So now if you don't have permission to write, when you browse as user, you won't be able to create, update or delete data.
Also removed Browser option from ObjectPickerDialog.
EditRowDialog and ObjectPickerDialognow use useMasterKey passed from Browser component via props.

@mtrezza
Copy link
Member

mtrezza commented Jun 19, 2020

@NinoZX Sorry for the late reply, I missed your update.

That's a pretty nice feature, I just tried it out. The terminology and flow is well done, it is also distinct from "login / logout" of the dashboard user.

  • I don't know how frequently this feature will be used, but from a usability aspect I would move it more to the left in the menu bar, between Manage Columns and Refresh. The most frequently used menu options should be positioned on the right, although that is not even entirely true in the current design. Another reason is that the Filter tab expands to the left when open. (I suggested the current position earlier, but after testing the usability, I think it would be better to reposition, sorry about that).
  • When starting / stopping browsing as user, any triggers that are set up in Cloud Code for when the "real" user logs in / out are triggered. While this is understandable, it can be problematic, for example if a user is unassigned from the installation after logout. Do you think there is a way to avoid the trigger? If not, we should mention this as a warning in the docs, as one would not expect any data write implication when simply "browsing" as a user. This is probably less of an issue now because to "browse as user" (still) requires the password, which is unknown for user accounts in production, so we can assume that this feature will be used only with test accounts.

@mtrezza
Copy link
Member

mtrezza commented Jul 15, 2020

@NinoZX I was wondering what the status of this PR is? I think it is basically ready for a review, the only thing that seems to be missing is the repositioning of the new menu item for usability reasons. Or did you have any new insights since then?

@NinoZX
Copy link
Contributor Author

NinoZX commented Jul 15, 2020

@mtrezza Sorry for my late reply, obviously i have missed your comment from 26 days ago.

Regarding that comment, will do repositioning by the end of this week, currently little bit busy.

@stale
Copy link

stale bot commented Aug 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state:wont-fix Won’t be fixed with a clearly stated reason label Aug 29, 2020
@NinoZX
Copy link
Contributor Author

NinoZX commented Aug 29, 2020

@mtrezza sry mate, I completely forgot about this PR.
Will do it in a few days and get back to you soon.

@stale stale bot removed the state:wont-fix Won’t be fixed with a clearly stated reason label Aug 29, 2020
…oard into browse-as-parse-user

* 'browse-as-parse-user' of github.com:italkco/parse-dashboard: (45 commits)
  Clear user & pass from LoginDialog on handleClose
  await fix for user logout
  Used object shorthand on useMasterKey
  Reposition Browse as User menu item
  Removed EditRowDialog fixes for next PR
  Removed quickfix for next PR
  Removed quickfix for next PR
  Use master key state property on editRowDialog and ObjectPickerDialog; Enabled using useMasteKey property for save actions and other actions
  fix: eslint fixes
  UX changes in main nav bar for browse as user; Toggle new type added - HIDE_LABELS;
  fix: eslint fixes
  fix: don't update Array,  Object or Polygon field on EditRowDialog when no changes
  Login dialog upgrades; Expand / Collapse TextArea fields on EditRowDialog; Fix for live reload data on EditRowDialog when data is updated on server
  Test ACL with parse user login
  Reposition Browse as User menu item
  Removed EditRowDialog fixes for next PR
  Removed quickfix for next PR
  Removed quickfix for next PR
  Use master key state property on editRowDialog and ObjectPickerDialog; Enabled using useMasteKey property for save actions and other actions
  fix: eslint fixes
  ...
@NinoZX
Copy link
Contributor Author

NinoZX commented Jan 19, 2021

@dplewis branch now ready to be merged I believe.

@dplewis
Copy link
Member

dplewis commented Jan 19, 2021

One small issue I found.

If a field is selected while viewing a class, you can't press the backspace / delete button while in the login popup to edit username / password. Might have to unfocus on the fields. If you press backspace it will update the object since the field is selected which can have side effects

refreshing the screen fixes this issue

@NinoZX
Copy link
Contributor Author

NinoZX commented Jan 25, 2021

@dplewis interesting bug, reproduced it and fixed it. Ready for further testing and review.

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @davimacedo How does it look to you?

@davimacedo
Copy link
Member

It looks good to me. I only wanted to better understand why we are requiring the password in order to switch the user. I am basically afraid of two things:

  • By requiring the password, make it harder to use the feature;
  • It does not add in terms of security, since the master key is available, but can make the users to think that it adds.

Also, when using the api console, it is possible to run a query as another user without typing in the password.

@davimacedo
Copy link
Member

I've just read the whole thread and seen @mtrezza also asked about it, but I believe this point is not pacified yet, right?

@mtrezza
Copy link
Member

mtrezza commented Jan 25, 2021

Yes, I think the open question is what is this feature really for?

The docs should clearly describe potential side effects, which one could not intuitively expect, see comment:

When starting / stopping browsing as user, any triggers that are set up in Cloud Code for when the "real" user logs in / out are triggered. While this is understandable, it can be problematic, for example if a user is unassigned from the installation after logout. Do you think there is a way to avoid the trigger? If not, we should mention this as a warning in the docs, as one would not expect any data write implication when simply "browsing" as a user. This is probably less of an issue now because to "browse as user" (still) requires the password, which is unknown for user accounts in production, so we can assume that this feature will be used only with test accounts.

On the other hand, "browsing as user" counter-intuitively requires a password to downgrade (!) privileges, see comment:

Why does it require to type in the password of a user, when you are actually limiting what you see in the browser? As I understand this feature, you log into the dashboard with highest level privileges and then for debug purpose temporarily downgrade your privileges regarding what you see in the Data Browser. In a real world scenario I also wouldn't know the user's password.

So the conclusive question is, is it a development tool or an operations tool?

  • For a development tool, this feature could only be enabled for Parse Server apps that have the development flag enabled, but then it wouldn't need the password login, because that could quickly become annoying for a dev tool.
  • For an operations tool, this feature could also be enabled for apps with production flag, in which case the password option prevents the feature from being used with any user but the developer's own test users where the password is known, so what is the operational use case?

@NinoZX
Copy link
Contributor Author

NinoZX commented Feb 1, 2021

@davimacedo let me try to answer your questions:

By requiring the password, make it harder to use the feature

Well this way was the fastest for me to get sessionToken to test my ACL settings for user roles. Not much code changes were required to get wanted functionality which is primarily testing testing.

It does not add in terms of security, since the master key is available, but can make the users to think that it adds.

This is correct. Moreover it even downgrades current user's privileges to the one's of inserted username.

As @mtrezza asked:

Yes, I think the open question is what is this feature really for?

I still stick to my answer from bottom of comment.

The docs should clearly describe potential side effects, which one could not intuitively expect

Agree to update the docs since I am not familiar with turning off triggers for login/logout.

So the conclusive question is, is it a development tool or an operations tool?

I see this as development tool at the moment. So yes, we can enable it only if dev flag is enabled if you want.

And for using password, as I said, this way the quickest and easiest way to implement this functionality. If someone has more time to research how to get sessionToken without using password, I would very much welcome you to either implement it or point me in the right direction.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

And for using password, as I said, this way the quickest and easiest way to implement this functionality.

I think that's a fair argument. If the feature gains traction, someone will make the effort to refine it eventually. Notwithstanding @davimacedo's points, which I agree with.

If someone has more time to research how to get sessionToken without using password, I would very much welcome you to either implement it or point me in the right direction.

Can you get any inspiration from the API console that has been mentioned where a query can be run as another user without entering the password?

I see this as development tool at the moment. So yes, we can enable it only if dev flag is enabled if you want.

Actually I row back on this one. We've seen in the past that for many restrictions that have been made in good faith, chances are someone will come up with a use case and PR to remove the restriction in the future.

@mtrezza
Copy link
Member

mtrezza commented Feb 8, 2021

To pick up the conversion here, for me, the only question that remains is whether the approach from the API console can be used to browse as user. If that avoids triggers in Cloud Code and the password is not needed anymore, I think the feature usability would benefit. If not, then I think the trigger execution would not be intuitive which merits a warning in the docs.

And by the way, @NinoZX thanks for your patience and contribution in the discussion around this PR, which is going on since April 2020. I think that is also a tribute to the complexity of this great feature.

@NinoZX
Copy link
Contributor Author

NinoZX commented Feb 11, 2021

Well, at the moment I am not able to further R&D this feature regarding API console.
So for now, I would say this is ready for merged as-is to be used as development tool when testing ACL.
According to that, as you said @mtrezza, we should mention this as warning in the docs.

@mtrezza
Copy link
Member

mtrezza commented Jul 26, 2021

@davimacedo What should we do with this PR?

It seems the only open points are:

  • why require a password, which seems unnecessary
  • add a warning that the cloud code triggers will be executed when logging in as another user; a docs note is necessary to make sure the developer understands the limitations of this feature

I think it may be an interesting dev tool for some, and once it is merged chances may be higher that it get improved over time. If we keep this stale for longer, I am afraid it will get harder to merge.

@NinoZX Would you mind merging this with master and resolving any conflicts?

@davimacedo
Copy link
Member

@mtrezza I think we can merge once we have solved these points.

@mtrezza
Copy link
Member

mtrezza commented Jul 26, 2021

Do you mean to merge only if "browse as another user" does not require password? Or would you merge it anyway, just so we get the feature out there.

@davimacedo
Copy link
Member

I'd prefer to merge not requiring the password but we can go as is and improve it later.

@mtrezza
Copy link
Member

mtrezza commented Jul 26, 2021

I would also prefer to merge this without PW, but I am afraid we do not have the resources to investigate that any further.

@mtrezza
Copy link
Member

mtrezza commented Jul 27, 2021

This branch seems to be locked because I cannot push any commits, so continuation in #1750.

@mtrezza mtrezza closed this Jul 27, 2021
@mtrezza mtrezza removed their request for review August 3, 2021 00:17
@frouo
Copy link

frouo commented Jan 16, 2023

Thank you all for you hard work! I wanted to know if you plan to remove the password requirement.

I am using the dashboard to provide support for my users (in production), and I can't obviously ask them for their password. I didn't find a way to filter by ACL, so debugging things for a given user is almost impossible using the dashboard (or I maybe miss something?).

Maybe this could be a parse-dashboard option, like PARSE_DASHBOARD_BROWSE_AS_USER_NEEDS_PASSWORD?

@mtrezza
Copy link
Member

mtrezza commented Jan 16, 2023

I suggest you search through the existing issues and if there isn't one then open a new feature request. That way your feature request gets tracked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.