Skip to content

feat: more flexible composited authenication#195

Merged
mcollina merged 1 commit intofastify:masterfrom
shenxdtw:master
Apr 20, 2023
Merged

feat: more flexible composited authenication#195
mcollina merged 1 commit intofastify:masterfrom
shenxdtw:master

Conversation

@shenxdtw
Copy link
Copy Markdown
Contributor

According to the current version, only a default relationship of either all AND or all OR can be set.

If I want to configure the following verification relationship:
[(verifyUserPassword and verifyLevel) or (verifyVIP)]

it cannot be configured.

This PR is intended to address more complex authorization scenarios in practice, including both AND and OR relationships.

I will treat it as a two-dimensional array where each element represents a verification method, and the second dimension of the array always has an AND relationship.

And it is backwards compatible with defaultRelation OR and AND.

Checklist

@Fdawgs
Copy link
Copy Markdown
Member

Fdawgs commented Apr 20, 2023

Bruh, just update the same PR/branch, no need to keep creating and deleting PRs. :)

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 567d8aa into fastify:master Apr 20, 2023
/* eslint-disable-next-line no-var */
for (var i = 0; i < functions.length; i++) {
functions[i] = functions[i].bind(this)
if (functions[i] instanceof Array) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (functions[i] instanceof Array) {
if (Array.isArray(functions[i])) {

functions[i][j] = functions[i][j].bind(this)
}
} else {
functions[i] = [functions[i].bind(this)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would invert the if/else case

First the most common and with less code

@shenxdtw shenxdtw mentioned this pull request Apr 21, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants