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

Add support for the editor role #738

Merged
merged 7 commits into from
Feb 13, 2024

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Jan 21, 2024

Fixes #675.

Description

This pull request:

  • adds the alias editor for developer to role mappings, which is supported by the HTTP API and the DB
  • adds tests
  • updates documentation.

The auth flow is as follows:

  • an HTTP API is called to set the role (can be either editor or developer)
  • the role is written to the DB (always written as developer)
  • when some page is accessed, the role is checked on auth by matching the DB value against role_mappings
    • developer from the DB is matched against editor or developer in role_mappings (in this order).

All user-visible parameters (the HTTP API and the role_mappings setting) allow for both editor and developer.
The name editor is preferred, which is reflected in the documentation.
The name developer is supported for compatibility reasons.

How I tested this locally:

  • to make sure that role_mappings are actually used by the test, I commented out editor in the default role_mappings
    • the test errored out as expected
  • to make sure that developer is still supported, I replaced editor with developer in the default role_mappings
    • the test succeeded as expected
  • to make sure that permissions are checked, I replaced the permissions of editor in the default role_mappings
    • the test errored out as expected

After making each of these changes in the application, I ran

PYTEST_ADDOPTS="-rpfxs -k test_end_to_end_auth_flow" pytest tests

and looked at the test output.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

Copy link

netlify bot commented Jan 21, 2024

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name Link
🔨 Latest commit 2284852
🔍 Latest deploy log https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65b584bee0269a0008f41490

Lets go through a few examples to make this more concrete and assume
Additionally, the role `editor` can be used, which is an alias for `developer`.

Let's go through a few examples to make this more concrete and assume
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo fix: "Lets" -> "Let's"

One of the previous commits explicitly defined both aliases in
`role_mappings`, but that was wrong because only one value is used in
the DB, which is `developer`. Instead, you need to map `developer` from
the DB to `role_mappings`, which can either have `editor` (new name) or
`developer` (legacy name). The new name is checked first, then the old
one.
@nkaretnikov nkaretnikov marked this pull request as ready for review January 22, 2024 04:02
Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

Just the one area that I think needs changed. Other than that, looks good.

# Additionally, this code allows for legacy 'role_mappings' that
# used to specify the role as 'developer'. Because it's a
# user-visible setting, we cannot break compatibility here
assert role != "editor" # must NEVER be 'editor' in the DB
Copy link
Contributor

Choose a reason for hiding this comment

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

An assert outside of a test strikes me as odd. Looking into it, it seems like some compilation flags remove assert statements. If this check is valuable, I think raising an exception here would be more valuable. However, I am not sure that this represents an unrecoverable error, since we would still know what to do with that data. We could just handle it and log an warning that data that shouldn't be in the db is. Or if it really does represent an unrecoverable error, than raising an exception seems better than an assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dcmcand Changed to raise. It's not unrecoverable, but it's a mistake that's easy to make when potentially refactoring or adding new DB APIs.

For example, see the change in conda-store-server/conda_store_server/api.py in this PR. The old code would bypass validate_role because .update is a lowel-level API, which just skips ORM validation logic. That was caught by the tests, which is how I noticed. But we might not have good coverage for some new yet to be added APIs, so I want to make sure this is never a problem.

@nkaretnikov
Copy link
Contributor Author

@dcmcand PTAL. Note: I want to keep the diff minimal to make it easier to review, I'll rebase after this is approved.

Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

🚀

@trallard trallard merged commit d7d701d into conda-incubator:main Feb 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[ENH] - Support "editor" as an alias for "developer"
4 participants