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.

Sign up for GitHub

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

Jump to bottom

Introduce meta authContext #7875

Merged
merged 5 commits into from
Nov 3, 2022
Merged

Introduce meta authContext #7875

merged 5 commits into from
Nov 3, 2022

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Oct 26, 2022 •
edited

Description

This PR introduces a meta.authContext prop on routes so that we give fine grained control to extension developers which kind of auth context they want to require from ownCloud Web.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@kulmann kulmann mentioned this pull request Oct 26, 2022
Closed
dschmidt
dschmidt approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment •
edited

Choose a reason for hiding this comment

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

Haven't tested and would be better with https://dev.to/ayc0/typescript-49-satisfies-operator-1e4i but alas ... better than before!

I especially like the very useful runtime/deprecation warnings!

@dschmidt
Copy link
Member

When this is merged, please add an entry here: #7338

Might be worth pinning that issue...

@kulmann
Copy link
Member Author

kulmann commented Oct 27, 2022

When this is merged, please add an entry here: #7338

Might be worth pinning that issue...

It was pinned when I created it in the first place (see screenshot)... thanks for re-pinning. And yes, of course, the deprecation info needs to go into the ticket. :-)

Screenshot 2022-10-27 at 08 59 45

diocas
diocas reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

Does what is really necessary for now (even if the presentation is inconsistent: see my comment in the original issue).

@kulmann kulmann requested a review from dschmidt October 28, 2022 14:39
@kulmann
Copy link
Member Author

kulmann commented Oct 28, 2022

@dschmidt I re-requested your review, because I added a substantial amount of code... added meta.pageLayout to route definitions, so that devs can not only choose the required authContext but also the rendering layout. Let me know what you think.

@kulmann kulmann changed the title Introduce meta auth context Introduce meta authContext and pageLayout Oct 28, 2022
@diocas diocas mentioned this pull request Nov 1, 2022
Closed
dschmidt
dschmidt approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Codewise the PR seems completely fine to me, I'm just not so enthusiastic about exposing the pageLayout setting. Especially the loading layout seems a bit weird, but then again it's probably not harmful

packages/web-pkg/src/composables/router/types.ts Outdated Show resolved Hide resolved
packages/web-pkg/src/composables/router/types.ts Outdated Show resolved Hide resolved
packages/web-runtime/src/App.vue Outdated Show resolved Hide resolved
@kulmann
Copy link
Member Author

kulmann commented Nov 2, 2022

Codewise the PR seems completely fine to me, I'm just not so enthusiastic about exposing the pageLayout setting. Especially the loading layout seems a bit weird, but then again it's probably not harmful

You're right, I didn't remember our decision to force all apps into the application layout. The more I think about it, the less I want to expose the pageLayout to developers. Instead I'd rather use the application layout as fallback for anonymous routes (which solves @diocas use case as well). Will discuss with @tbsbdr if we want to drop the plain layout for our own internal anonymous routes as well (like accessDenied page, oidcCallback page, etc)... converting back to draft.

@kulmann kulmann marked this pull request as draft November 2, 2022 12:20
@kulmann
feat: introduce meta.authContext
2ea194c 
Introduce meta.authContext to define which kind of context is required
on a route. This gets evaluated for target routes and (if present)
context routes.

The old meta.auth boolean has been deprecated. For now it gets
transformed into the respective meta.authContext value.
@kulmann
feat: set authContext requirement explicitly
361fca5 
All routes in the web mono repo now have a meta.authContext defined.
@kulmann
docs: add changelog item for authContext feature
1263659
@kulmann
refactor: enrich Plain page layout and simplify associated pages
a501b04
@kulmann
fix: don't wait for spaces if context is anonymous
b27bc11
@kulmann kulmann marked this pull request as ready for review November 2, 2022 22:15
@kulmann
Copy link
Member Author

kulmann commented Nov 2, 2022

Updated the PR to not expose/use the pageLayout prop anymore. As described earlier today, all app routes, including the ones requesting an anonymous context, will render in the Application layout. I kept the plain layout for login, logout, oidcCallback, oidcSilentRedirect, resolvePublicLink and accessDenied. Those were our implicitly anonymous page before. Now they are our implicitly plain-layout pages (they all received an authContext: "anonymous" meta prop in this PR).

@kulmann kulmann changed the title Introduce meta authContext and pageLayout Introduce meta authContext Nov 2, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

40.0% 40.0% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit 775151d into master Nov 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the introduce-meta-auth-context branch November 3, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschmidt dschmidt dschmidt approved these changes

@diocas diocas diocas approved these changes

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

@lookacat lookacat Awaiting requested review from lookacat

@JammingBen JammingBen Awaiting requested review from JammingBen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@kulmann @dschmidt @diocas