Skip to content
Snippets Groups Projects

Revert "Merge branch 'oauth-extra-scopes' into 'develop'"

Merged Revert "Merge branch 'oauth-extra-scopes' into 'develop'"
revert-96cab6d8 into develop
Merged lain requested to merge revert-96cab6d8 into develop

This reverts merge request !1024 (merged)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • lain enabled an automatic merge when the pipeline for b3992358 succeeds

    enabled an automatic merge when the pipeline for b3992358 succeeds

  • lain merged

    merged

  • lain mentioned in commit c2c48ec2

    mentioned in commit c2c48ec2

  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    @lambadalambda Once pleroma!2061 (merged) is merged, it's safe to revert this revertion.

  • HJ
    HJ @hj ·
    Maintainer

    so it does NOT work on older backends, huh?

  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    @hj It does work on back-ends older than 2 days (prior to merge of pleroma!2025 (merged)). And it'll work on new BEs once pleroma!2061 (merged) is merged. Older BEs didn't reject admin scopes, they didn't make any use of them. New BEs make use of admin scopes, but we need to gracefully clear such for non-admin users.

    Upd.: 2061 was merged.

    Edited by Ivan Tashkinov
  • HJ
    HJ @hj ·
    Maintainer

    so if i understand correctly either a DB migration adds missing scopes automatically or FE needs to request those itself? What was the reason for original revert?

  • rinpatch
    rinpatch @rinpatch ·
    Contributor

    What was the reason for original revert?

    If you requested admin scope as a non-admin user on a backend that support admin scopes, it would reject the request.

    So if i understand correctly either a DB migration adds missing scopes automatically or FE needs to request those itself?

    Currently it's the latter, though I think we should do the former. @i1t what do you think?

  • HJ
    HJ @hj ·
    Maintainer

    If you requested admin scope as a non-admin user on a backend that support admin scopes, it would reject the request.

    That sounds... weird. How do you ask for admin scope when logging in as admin or as a regular user? Ask backend if user is an admin sounds weird.

  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    PleromaFE creates an app on each load and sets supported OAuth scopes on it. It used to be read write follow, now it's read write follow push admin.

    On sign-in, PleromaFE requests OAuth scopes for the user. At this stage it's unknown whether a user is an admin or not, and it'd be incorrect to pre-request this info anyways. So, PleromaFE requests read write follow push admin for any user. BE determines if user is an admin and keeps admin scopes(s) only in this case, so the resulting token has read write follow push admin scopes for admins and read write follow push for non-admins.

    If new version of FE interacts with older BE, it'll result in read write follow push admin scopes set for all users. Older BEs don't deal with admin scopes (i.e. grant no special abilities if those are present) and don't control usage of those.

    @rinpatch, once we bundle updated FE versions with BE, I agree it'd be better to migrate existing apps / admin authorizations / admin tokens to add admin scope. Will take care of that once admin scope functionality is merged to PleromaFE again.

    Edited by Ivan Tashkinov
  • Ivan Tashkinov mentioned in commit 341416b0

    mentioned in commit 341416b0

  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    Created the revert of this revert: !1034 (merged)

  • Ivan Tashkinov mentioned in merge request !1034 (merged)

    mentioned in merge request !1034 (merged)

  • HJ
    HJ @hj ·
    Maintainer

    IIRC it should not create an app on each new load, it tries to reuse existing one stored in browser's storage.

    Another thing to consider: what will happen if you make some user an admin/moderator?

    Also what push scope does?

  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    @hj, push is a standard Mastodon scope for managing push subscriptions. Added a more detailed description to !1034 (merged).

    As for making someone an admin, it doesn't currently modify this user's tokens. Some apps (unfortunately, not all) allow user to explicitly select OAuth permissions on sign in, and it could be that an admin user intentionally deselected admin scopes in some client apps, so we shouldn't extend token scopes for every app. As for PleromaFE, we know that it (currently) doesn't allow users to deselect any scopes, so we could potentially extend PleromaFE (and bundled MastoFE, though there were good times it allowed to specifically select permissions on login) tokens with admin scope when user is made an admin.

    When an admin is downgraded to a regular user, such user is signed out of all apps. We could do the same when user is promoted to an admin from regular user, that'd be easier and more transparent (plus no need to make guesses on why any permissions that app supports are missing on token, was it a user decision or not).

    Edited by Ivan Tashkinov
  • Ivan Tashkinov
    Ivan Tashkinov @i1t ·
    Contributor

    Note: right now BE does not enforce admin scopes usage, and FE will work with older and new versions of BE without the need to do anything to tokens (e.g. sign out / sign in isn't needed). !1034 (merged) is ready for the merge.

  • Ivan Tashkinov mentioned in issue pleroma#1478 (closed)

    mentioned in issue pleroma#1478 (closed)

Please register or sign in to reply