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

refactor(auth): Various build improvements and minor refactoring #2959

Closed
wants to merge 57 commits into from

Conversation

rpatterson
Copy link
Member

Revert the commits that changed the components that refer to the logged in user to use
the response from the actions API endpoint. IOW, those components now, once again, use
the Bearer token instead. Preserves all the other improvements made to the code along
the way.

The new branch for the plone.restapi includes the API changes to the PAS plugin needed
to set both the Plone classic session cookie and the Bearer token cookie that Volto can
optionally use to initialize authentication without using the Volto login component.

All this put together means that:

  • the Volto components related to logged in users now behave largely as they did before
    in terms of interpreting state
  • logging in to or out of either classic or Volto also does so in the other

One edge case that remains is when logging in via the Volto UI component as a user from
the Zope root acl_users (e.g. admin or SITE_OWNER_NAME), they aren't logged into
Plone classic. The other direction is fine, logging in as admin to Plone classic
results in a new request to the Volto UI being logged in. Digging into that now.

These changes do not address any of the issues that happen when logged in via the ZMI at
the Zope root. A user logged in that way will still appear logged in to classic Plone,
but won't be recognized by Volto components as logged in but API responses will still
reflect their Manager access rights. Tackling that is coming up.

This PR supersedes both #2535 #2687.

rpatterson and others added 30 commits June 22, 2021 23:06
A testbed that reproduces accessing all of the ZMI, the classic HTML plone UI, and the
Volto UI under the same hostname.

Originally needed to workout issues with different kinds of authentication working for
all UIs.

refs #131171
Use `collective.recipe.omelette` to reproduce a single directory tree of the Python
packages installed in the `./api/` buildout's `instance` part.  Useful for searching,
browsing, or otherwise exploring all the source code involved in the application in a
way that's more readable and avoids duplicates from older versions of eggs.
This repository has no `master` branch.  At least according to GitHub, `main` is the
default branch and it also has the most recent activity in `$ git log`.
More sensible ordering of the folder structure documentation so that the library, React,
is first, the state framework, Redux, is next, followed by more project specific
structure.
Refactor the logic around user session state out of the multiple components into Redux
selectors.  This is a step on the way to refactoring the UI user session logic to be
agnostic of *how* the user is authenticated.  Currently this includes 2 selectors:

- detecting whether a user is currently authenticated and logged in
- retrieving the authenticated users login or ID

I note this project isn't using selectors prior to this.  From [the Redux
docs](https://redux.js.org/tutorials/fundamentals/part-2-concepts-data-flow#selectors):

> Selectors are functions that know how to extract specific pieces of information from a
> store state value. As an application grows bigger, this can help avoid repeating logic
> as different parts of the app need to read the same data

That's *exactly* what's needed here.  This is particularly true for getting user data
state, such as the `userId`.  As such, I went ahead and introduced the usage of
selectors here.

Refs #134784
Seeking an action by id, for example to check for the presence of an action or to lookup
the `url` property of a specific action, is a very common task.  As such, transform the
arrays of actions into objects of actions by action id and include that in Redux state
as well.

Next, I plan to use this to replace the JWT implementation-dependent check if a user is
logged in.

Refs #134784
Defer to the API, specifically the presence of the `login` action, to determine if a
user is logged in instead of depending on the implementation detail of the UI JWT login
process.

Refs #134784
* master: (35 commits)
  Installluxon (#2565)
  Inlineform default values (#2559)
  Back to development
  Release 13.6.0
  Prepare for release
  Fix edit warning (#2443)
  Allowing user to click on the breadcrumbs of objectBrowser (#2541)
  Update docs about robotstxt (#2546)
  #2492 added landmark role to skiplink-container (#2493)
  #2489 made the breadcrumb link text color slightly darker for a11y (#2491)
  #2487 changed headline color to black for better contrast (#2488)
  Add DictWidget (new name: VocabularyTermsWidget) (#2532)
  Update Brazilian Portuguese translation. (#2558)
  Back to development
  Release 13.5.0
  Prepare for release
  Add og tags and seo support (#2510)
  Upgradestorybook2 (#2556)
  Revert "Upgrade to Storybook 6.3, refresh deps for babel (#2551)" (#2555)
  Upgrade to Storybook 6.3, refresh deps for babel (#2551)
  ...
Switch from a feature to a breaking change in the changelog
[because](#2535 (comment)):

> In the RenderedView, the `token` is removed and used `userLoggedIn` instead. This will
> potentially break all customizations using component shadowing for all customized
> Views outthere, so we need to mark it as breaking.
Addresses [feedback](#2535 (comment)):

> I would like to maintain the purity of what means to start the backend, what means to
> start the frontend without abstractions (docker in the middle using traeffic). It's
> fine to maintain it as a separate command, but not as "canonical". The reasons are
> mainly for training and newbies to really know what's going on under the hood.
* master: (79 commits)
  Back to development
  Release 14.0.0-alpha.0
  Prepare for release
  Remove compatibility with older configuration way based on imports (#2516)
  Locking support (#2594)
  Back to development
  Release 13.15.0
  Prepare for release
  feat: add placeholder to wysiwyg widget (#2653)
  chore(i18n): update ita translations (#2652)
  Show loading indicator in listing view when content is loading (#2644)
  Validate touched-only `required` fields in Forms (#2642)
  Get SchemaWidget field factories from backend (#2651)
  Change the batch size of folder content (#2654)
  Show item title and item type when hovering over item title and item type icon in folder content view (#2649)
  Back to development
  Release 13.14.0
  Prepare for release
  cypress: user/groups controlpanel tests (#2650)
  Reimplement the architecture of user/groups controlpanel (#2064)
  ...
* master: (38 commits)
  Fix buildout. Remove indentation in buildout.cfg (#2679)
  Do not show lead image block when the content type does not have the behavior enabled (#2676)
  Back to development
  Release 14.0.0-alpha.8
  Prepare for release
  Fix logout action, removing the  cookie as well, if present. (#2677)
  Back to development
  Release 14.0.0-alpha.7
  Prepare for release
  German translations updated (#2675)
  translate Japanese (#2671)
  Back to development
  Release 14.0.0-alpha.6
  Prepare for release
  Fix ssr for /edit when using devproxy (#2674)
  Back to development
  Release 14.0.0-alpha.5
  Prepare for release
  Fix on form errors in block editor, not changing to metadata tab (#2668)
  Back to development
  ...
* master:
  Back to development
  Release 14.0.0-alpha.9
  Prepare for release
  Use the block `title` as the source of the translation `id` instead o… (#2684)
This is a regression from when I added defensive make settings in:

    build(devel): Avoid silent make errors/failures
The `./Makefile` originally referenced a `test` target, since moved to the
`./api/Makefile`, but the `./api/buildout.cfg` installs no such buildout part.  Figuring
it's better to restore that than remove it, I tried restoring that part to the API
buildout and found it easy and that most of the tests are already passing.
For each step in the build process, use the real artifacts that result as targets in
`./api/Makefile`.  This prevents wasting time in the inner loop of individual
development and when the prerequisites, `... ./buildout.cfg ./src/*/setup.py`, are
maintained carefully this rarely results in surprising behavior such as something
failing because a build step wasn't detected as needed.
When revisiting the proxy test bed I couldn't remember how to start it correctly.
Clearly I should have documented it better!  :-/ Clarify and expand the comments to
address that.

Also improves how the proxy test bed is run as well as the output from that test bed.

I change the target names from `start-*` to `run-*` because to my mind `start` means to
launch something successfully "in the background" and return the shell to the user, such
as with `# systemctl ...` and other system service control commands.  Whereas `run`
means to keep running "in the foreground" until the user interrupts.  This means the
terminology in the target names is inconsistent but I figured it was better to use
incrementally better terminology in the targets I'm touching and avoid breaking things
for other developers than to not improve the terminology at all.
These popped up after merging master.  The conflicting version messages in order
encountered are:

    Version and requirements information containing plone.app.vocabularies:
      [versions] constraint on plone.app.vocabularies: 4.3.0 # plone.volto requires it
      Requirement of plone.volto: plone.app.vocabularies>=4.3.0
    While:
      Updating instance.
    Error: The requirement ('plone.app.vocabularies>=4.3.0') is not allowed by your
    [versions] constraint (4.3.0 # plone.volto requires it)

then:

    Version and requirements information containing plone.restapi:
      [versions] constraint on plone.restapi: 8.16.2 # force to latest p.restapi
      Requirement of plone.volto: plone.restapi>=8.13.0
      Requirement of Plone==5.2.6: plone.restapi
    While:
      Installing instance.
    Error: The requirement ('plone.restapi>=8.13.0') is not allowed by your [versions] constraint (8.16.2 # force to latest p.restapi)

then:

    Version and requirements information containing plone.rest:
      [versions] constraint on plone.rest: 2.0.0a1 # new JSON only traversal
      Requirement of plone.volto: plone.restapi>=8.13.0
      Requirement of Plone==5.2.6: plone.restapi
      Requirement of plone.restapi>=8.13.0: setuptools
      Requirement of plone.restapi>=8.13.0: pytz
      Requirement of plone.restapi>=8.13.0: python-dateutil
      Requirement of plone.restapi>=8.13.0: plone.schema>=1.2.1
      Requirement of plone.restapi>=8.13.0: plone.rest>=1.0a6
      Requirement of plone.restapi>=8.13.0: plone.behavior>=1.1
      Requirement of plone.restapi>=8.13.0: PyJWT
    While:
      Installing instance.
    Error: The requirement ('plone.rest>=1.0a6') is not allowed by your [versions] constraint (2.0.0a1 # new JSON only traversal)
CHANGELOG.md Outdated Show resolved Hide resolved
@rpatterson
Copy link
Member Author

For future reference, here's a ./Makefile target I've been using to reproduce upgrade/migration issues with existing ZODBs:

.PHONY: test-upgrades
test-upgrades:  ## Test upgrading existing ZODBs
# Start out before the PR changes
	git checkout master
# Delete and recreate the Zope instance ZODB
	rm -r "./api/.installed.cfg" "./api/var/filestorage/" "./api/var/blobstorage/"
	$(MAKE) build
# Activate job control so back-grounded process get their own PGID so we can kill all
# their descendant processes: https://stackoverflow.com/a/6552484/624787
	set -m
# Start the clean instance
	$(MAKE) start &
	set +x
	until nc -vz "localhost" "3000"; do sleep 1; done
	echo "In the browser, log into the Volto/React UI as the Zope root"
	echo "'/acl_users' user 'admin' to ensure the browser has a JWT token."
	read -p "Hit <ENTER> when done."
	set -x
	kill %1
	while nc -vz "localhost" "3000"; do sleep 1; done
# Force the backend Plone classic UI to be served on the same port so the browser will
# send the same cookies, etc..
	rm -v "./api/.installed.cfg"
	$(MAKE) -C "./api/" INSTANCE_PORT=3000 start &
	set +x
	until nc -vz "localhost" "3000"; do sleep 1; done
	echo "In the browser, visit the Zope root ZMI:"
	echo "http://localhost:3000/manage_main"
	read -p "Hit <ENTER> when done."
	set -x
	kill %1
	while nc -vz "localhost" "3000"; do sleep 1; done
# Checkout the branch that changes the plugins
	git checkout fix-unify-auth
# Start the backend again on the same port, but don't install the `plonesite` part to
# avoid updating/upgrading the ZODB
	cd "./api/"
	"./bin/buildout" instance:http-address=3000 install instance
	"./bin/instance" fg &
	set +x
	until nc -vz "localhost" "3000"; do sleep 1; done
	echo "In the browser, visit the Zope root ZMI:"
	echo "http://localhost:3000/manage_main"
	read -p "Hit <ENTER> when done."
	set -x
	kill %1

These entries were from when there were changes in Volto to support the unified auth
work.  That's no longer the case, so cleaning up these changelog entries.
I kept going to Plone`s `buildout.coredev/sources.cfg` to add new checkouts, such as
while developing upstream PRs in combination with work here or after upstream PRs have
merged but not yet released.  It was extra work because I had to copy and paste the
`sources` line, but then manually replace the `${remotes:...}` substitutions.

This buildout is for development, we already use external `buildout:extends` URLs, so
would we repeat ourselves.
@netlify
Copy link

netlify bot commented Feb 25, 2022

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 813ba72
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/62566f059dc805000a9e242d

@rpatterson
Copy link
Member Author

mag Inspect the deploy log: https://app.netlify.com/sites/volto/deploys/6218aeab225ab70008a1c0a6

The error message there is about a missing ./Makefile target, netlify, but from the logs I can't see any changes of mine that could have removed such a target. Can someone help me understand how my changes might have affected that or may I ignore that message?

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Minor grammar and typo fixes, and attempt to improve docstring for arrayWIdsToObject.

docs/source/recipes/folder-structure.md Outdated Show resolved Hide resolved
src/helpers/Utils/Utils.js Outdated Show resolved Hide resolved
src/helpers/Utils/Utils.js Outdated Show resolved Hide resolved
src/helpers/Utils/Utils.js Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

mag Inspect the deploy log: https://app.netlify.com/sites/volto/deploys/6218aeab225ab70008a1c0a6

The error message there is about a missing ./Makefile target, netlify, but from the logs I can't see any changes of mine that could have removed such a target. Can someone help me understand how my changes might have affected that or may I ignore that message?

It fails because the version of the Makefile in this PR does not include the netlify target that was added within the last couple of weeks to the master branch to preview the build of docs.

You could ignore it, but it might be a good idea to rebase your branch on master, then force push, or merge master onto your branch then push, in order to get any of the goodies that have come through in the last two months since opening this PR.

@rpatterson
Copy link
Member Author

It fails because the version of the Makefile in this PR does not include the netlify target that was added within the last couple of weeks to the master branch to preview the build of docs.

Doh! I should have checked for new commits on master before asking for help there, sorry! Thanks for the helping hand. I've merged and pushed.

rpatterson and others added 2 commits February 25, 2022 12:45
Introduced when [I cleaned up the no-longer-relevant
entry](8d887af)
but didn't replace it.
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Suggestions for improving the function name.

src/helpers/Utils/Utils.js Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

I pushed commit ffc731e for review.

I also commented on the function name: #2959 (comment)

Copy link
Member

@avoinea avoinea left a comment

Choose a reason for hiding this comment

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

LGTM.

Tested on localhost and demo website with LDAP:

Copy link
Member Author

@rpatterson rpatterson left a comment

Choose a reason for hiding this comment

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

I can't find any discussion of the why for this change. Am I missing that somewhere?

@rpatterson rpatterson force-pushed the fix-flashy-toolbar-in-2661 branch from 8ec98aa to aa938e3 Compare April 13, 2022 06:25
Apparently [the `clean-tmp-worktrees` target doesn't work on OS
X](#3194 (comment)).  Rather than remove all
support for testing a clean checkout in a worktree, use a less invasive approach and
just remove running it from the `clean` target so that others may still use the
`test-clean` and `clean-tmp-worktrees` targets.

This reverts commit f7648f5.
@rpatterson rpatterson changed the title feat(auth): Unified log in/out both API/classic refactor(auth): Various build improvements and minor refactoring Apr 13, 2022
@ichim-david ichim-david added the Abandoned PRs that are abandoned and in a stale state label Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned PRs that are abandoned and in a stale state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants