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

feat: access tokens user management UI #3587

Merged
merged 74 commits into from
Feb 21, 2023
Merged

feat: access tokens user management UI #3587

merged 74 commits into from
Feb 21, 2023

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Aug 6, 2022

Picks up from #2651 and #2074

Changes proposed in this pull request:
Introduces the following:

  • UI to manage access tokens (actor only).
    • Revoke developer tokens (only the actor can delete his own and no one else).
    • Create developer tokens (based on permission).
    • View developer token value.
    • See other active sessions (token values are not exposed from the API for sessions/system-created tokens).
      • Terminate a remote active session.
      • Terminate all other sessions (not including the current session, user should log out).

Reviewers should focus on:
After reading the above behavior ^, any opinions if anything should be done differently? such as:

  • Should admins/permission-given-users be able to terminate/revoke other users' sessions/dev-tokens?
  • Is this large enough to move to a separate bundled extension? (I personally think not).

Checkout integration tests method names for further behavior understanding.

Video
https://user-images.githubusercontent.com/20267363/183262689-0de50d68-91db-48d6-a5f9-c252d789c553.mp4

Screenshot
Screenshot from 2022-08-06 19-53-36

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

SychO9 and others added 20 commits August 6, 2022 19:46
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
@SychO9 SychO9 added this to the 1.x milestone Aug 6, 2022
@SychO9 SychO9 self-assigned this Aug 6, 2022
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

It looks really good!

I tried it out locally and didn't find anything major.

I would suggest adding a confirmation popup when Revoking a developer token. Maybe that would be useful for Terminate and Terminate all others as well.

In the developer tokens part I see the text says "on [...]" and then the string ends. Looking at the source it looks like it should describe the device. I did my test with cURL (UA curl/7.68.0). I think there should be some fallback "N/A" value or the raw UA?

image

I would also suggest replacing the hidden token value by a hard-coded number of stars. Showing **** would probably be enough, no need for such a long string.

And finally I'd suggest wrapping the revealed token value in <code></code> to make it stand out better and potentially make it easier to select in some browsers (?)

PS: video in first post doesn't play in Firefox. But works in Chrome

@SychO9
Copy link
Member Author

SychO9 commented Aug 9, 2022

Will do! thanks for the review!

@tankerkiller125
Copy link
Contributor

  • Should admins/permission-given-users be able to terminate/revoke other users' sessions/dev-tokens?

In my view the answer to this is yes, if the system get's compromised or something it would be good for it to be possible for the admin to revoke all sessions and/or tokens. HOWEVER, whether this should be a individual user view where the admin can close just a single users sessions, or just a button on the admin side that wipes all of the sessions is another matter.

I do think that token removal should be on an individual user level though in terms of admin deletion. As my view of it is that it's a way for admins to moderate tokens (if say someone is abusing them)

  • Is this large enough to move to a separate bundled extension? (I personally think not).

It should not be a separate extension, I view it as a critical security feature that should be part of core.

@SychO9
Copy link
Member Author

SychO9 commented Aug 9, 2022

In my view the answer to this is yes, if the system get's compromised or something it would be good for it to be possible for the admin to revoke all sessions and/or tokens. HOWEVER, whether this should be a individual user view where the admin can close just a single users sessions, or just a button on the admin side that wipes all of the sessions is another matter.

I do think that token removal should be on an individual user level though in terms of admin deletion. As my view of it is that it's a way for admins to moderate tokens (if say someone is abusing them)

Then maybe we should create a moderate other users' sessions/tokens to allow a moderator group access to a user's list of sessions/tokens (but make sure to hide the actual token value of course from non-actors). We'd allow viewing basic information and allow terminating/revoking. Does that sound good?

@tankerkiller125
Copy link
Contributor

That seems like a reasonable idea, I wouldn't want anyone seeing the values other than the users anyway. Most sites only allow the viewing of a token once anyway even for the users.

@luceos luceos added this to the 1.x milestone Nov 14, 2022
@SychO9 SychO9 modified the milestones: 1.x, 1.7 Nov 19, 2022
Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

This is amazing. Just a couple of things, really.

Also, I think the frontend additions need to be added to their respective compat files?

import app from '../app';
import Component, { ComponentAttrs } from '../../common/Component';
import icon from '../../common/helpers/icon';
import uaParser from 'ua-parser-js';
Copy link
Member

Choose a reason for hiding this comment

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

I am having concerns about our bundle size nowadays. Could we potentially lazy-load this?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, switched to parsing the UA on the backend side

framework/core/js/src/forum/components/UserPage.tsx Outdated Show resolved Hide resolved
framework/core/locale/core.yml Show resolved Hide resolved
SychO9 and others added 5 commits February 10, 2023 23:27
# Conflicts:
#	framework/core/js/src/common/Application.tsx
#	framework/core/locale/core.yml
#	yarn.lock
Signed-off-by: Sami Mazouz <[email protected]>
Co-authored-by: David <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Co-authored-by: David <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
@SychO9 SychO9 requested a review from davwheat February 11, 2023 09:57
value: Mithril.Children;
}

export default class DataSegment<CustomAttrs extends IDataSegmentAttrs = IDataSegmentAttrs> extends Component<CustomAttrs> {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe LabelValue-something?

@davwheat
Copy link
Member

Overall, I think this is great. DataSegment could be renamed, but it's quite a difficult component to name imo.

@SychO9
Copy link
Member Author

SychO9 commented Feb 21, 2023

LabelValue is a lot better indeed

@SychO9 SychO9 dismissed stale reviews from askvortsov1 and clarkwinkelmann February 21, 2023 13:10

stale

@SychO9 SychO9 merged commit 9342903 into main Feb 21, 2023
@SychO9 SychO9 deleted the sm/access-tokens-ui branch February 21, 2023 13:14
@SychO9 SychO9 removed the prio/high label Feb 21, 2023
@SychO9 SychO9 restored the sm/access-tokens-ui branch February 21, 2023 13:36
@SychO9 SychO9 deleted the sm/access-tokens-ui branch February 21, 2023 14:22
@luceos luceos mentioned this pull request Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: completed
Development

Successfully merging this pull request may close these issues.

8 participants