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

View authentication and permission fixes #5464

Merged
merged 24 commits into from Mar 25, 2024

Conversation

glennmatthews
Copy link
Contributor

Closes GHSA-m732-wvh2-7cq4

What's Changed

Security fixes

  • The endpoint /extras/job-results/<uuid:pk>/log-table/ now requires user authentication to access, and also enforces appropriate JobResult view permissions now.
  • The endpoint /extras/secrets/provider/<str:provider_slug>/form/ now requires user authentication to access.
  • Removed the URL endpoints /api/users/users/my-profile/, /api/users/users/session/, /api/users/tokens/authenticate/, and /api/users/tokens/logout/ as they are unused at this time.
  • The /api/graphql/ REST API endpoint now requires user authentication to execute GraphQL queries, even when EXEMPT_VIEW_PERMISSIONS is configured.
  • The nautobot.apps.api.APIRootView class now enforces user authentication by default. As a consequence, viewing the browsable REST API root endpoints (e.g. /api/, /api/circuits/, /api/dcim/, etc.) now requires user authentication.
  • The endpoints /dcim/racks/<uuid>/dynamic-groups/, /dcim/devices/<uuid>/dynamic-groups/, /ipam/prefixes/<uuid>/dynamic-groups/, /ipam/ip-addresses/<uuid>/dynamic-groups/, /virtualization/clusters/<uuid>/dynamic-groups/, and /virtualization/virtual-machines/<uuid>/dynamic-groups/ now require user authentication, even when EXEMPT_VIEW_PERMISSIONS is configured.

Additional permissions enforcement

  • The /api/dcim/connected-device/?peer_device=...&?peer_interface=... REST API endpoint now requires the requesting user to have view permission on the specified peer_interface.
  • The endpoints /extras/git-repositories/<uuid:pk>/sync/ and /extras/git-repositories/<uuid:pk>/dry-run/ have corrected permissions now - a user who has change permissions for a subset of Git repositories is no longer permitted to sync or dry-run other repositories for which they lack the appropriate permissions.
  • Viewing Notes associated to a given model now requires extras.view_note permissions for each note to display (this is consistent with how change log views for a model enforce extras.view_objectchange permissions).

Bug fixes, housekeeping, documentation

  • Fixed a 500 error when accessing any of the /dcim/<port-type>/<uuid>/connect/<termination_b_type>/ view endpoints with an invalid/nonexistent termination_b_type string.
  • The nautobot.apps.api.OrderedDefaultRouter class now accepts a view_name and/or view_description as parameters. Specifying these parameters is to be preferred over defining a custom APIRootView subclass when defining App API URLs.
  • Updated example views in the App developer documentation to include ObjectPermissionRequiredMixin or LoginRequiredMixin as appropriate best practices.
  • Updated custom views in the example_plugin to include LoginRequiredMixin as a best practice.
  • Added integration test case that enumerates all registered URL patterns and verifies that they return appropriate response codes when accessed by an unauthenticated user.

Screenshots

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@@ -0,0 +1 @@
Added `nautobot.apps.utils.get_url_for_url_pattern` and `nautobot.apps.utils.get_url_patterns` lookup functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

14 change log entries in 1 PR 😩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 😢 This one scope crept, but I think everything in it is still relevant/necessary.

@glennmatthews glennmatthews self-assigned this Mar 22, 2024
@glennmatthews glennmatthews added the emergent Unplanned work that is brought into a sprint after it's started. label Mar 22, 2024
yield from get_url_patterns(urlconf, item.url_patterns, base_path + str(item.pattern))


def get_url_for_url_pattern(url_pattern):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be get_valid_url_for_url_pattern or get_fake_url_for_url_pattern maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine with either.

\* with a very small number of known exceptions such as login and logout views.
"""

def test_all_views_require_authentication(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a good test to expose to apps. Not blocking but should we write a generic test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do plan/want to add a generic test to the cookiecutter that takes in the app's urlconf and iterates/validates its url patterns, yeah. I like the idea of implementing the bulk of it in core rather than in the cookiecutter but that'll take some thinking about how to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a follow-up issue for that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logger.warning(
"Something has changed an OrderedDefaultRouter's APIRootView attribute to a custom class. "
"Please verify that class %s implements appropriate authentication controls.",
self.APIRootView.__name__,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include the full class path?

Comment on lines +181 to +183
name = getattr(view, "name", None)
if name is not None:
return view.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is not:

if getattr(view, "name", None) is not None:
    return view.name

OR

name = getattr(view, "name", None)
if name is not None:
    return name

Just looks weird to me as written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be return name - transcription error on my part.

Copy link
Contributor

@gsnider2195 gsnider2195 left a comment

Choose a reason for hiding this comment

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

Just a couple nitpicks left but nothing blocking. This is great!

@gsnider2195 gsnider2195 merged commit dd623e6 into develop Mar 25, 2024
17 checks passed
@gsnider2195 gsnider2195 deleted the u/glennmatthews-746-view-authentication branch March 25, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emergent Unplanned work that is brought into a sprint after it's started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants