Clarification needed regarding auth plugin interface

[dominykas]

Hey everyone! We have a discussion started here: n4bb12/verdaccio-github-oauth-ui#205 (comment) and we're looking for some clarification.

Documentation for allow_access says:

• If the request success [..] callback(null, isAllowed)
• If the request fails [..] Any other action different than success must return an error.

This creates a little bit of ambiguity around what "success" is. So to clarify, if the user is not allowed access, should there be a callback(err) or callback(null, false)?

The internal plugin memory seems to throw.

But also there is a need to have things working when multiple plugins are enabled - does allow_access allow fallback to another plugin, or is it only available for authenticate?

[mbtools]

afaik, callback(null,false) skips to the next auth plugin. callback(err) stops the loop over plugins and raises the error.

[dominykas]

Yes, that's how it works and is documented for authenticate and it makes sense there. But it's not how allow_access is documented?

More importantly, calling callback(null, false) for allow_access for a plugin that's the only auth plugin actually allows the user to proceed:

verdaccio/packages/auth/src/auth.ts

Line 303 in d028ce1

next(); // cb(null, false) causes next plugin to roll

- in which case that's probably a bug?

[mbtools]

If all plugins pass callback(null, false), it falls back to the built-in method

verdaccio/packages/auth/src/utils.ts

Line 182 in 9b44fbb

export function allow_action(action: ActionsAllowed, logger: Logger): AllowAction {

This will either confirm access callback(null, true) or error with callback(err). So there won't be a final callback(null, false) to allow access as the middleware has the final say:

verdaccio/packages/middleware/src/middlewares/allow.ts

Lines 42 to 63 in 9b44fbb

	auth['allow_' + action](
	{ packageName, packageVersion },
	remote_user,
	function (error, allowed): void {
	req.resume();
	if (error) {
	debug('user is NOT allowed to %o', action);
	next(error);
	} else if (allowed) {
	debug('user is allowed to %o', action);
	afterAll?.(
	{ action, user: remote_user?.name },
	`[middleware/allow][@{action}] allowed for @{user}`
	);
	next();
	} else {
	// last plugin (that's our built-in one) returns either
	// cb(err) or cb(null, true), so this should never happen
	throw errorUtils.getInternalError(API_ERROR.PLUGIN_ERROR);
	}
	}
	);

If it doesn't work that way, please create an issue with complete config and debug log.

But you are right, docs are lacking (and we should have more tests for the multi plugin case).

[dominykas]

If all plugins pass callback(null, false), it falls back to the built-in method

OK, that probably explains it.

What you're saying is that the plugins effectively cannot deny the access? Suppose this scenario:

• plugin reports via authenticate() that user is in a group (provided by external system) that gives access
• JWT is created and has the list of groups (incl. the one that grants access)
• plugin implements allow-access, checks if the user is still in the group and if it no longer is - calls cb(null, false)
• verdaccio then falls back to the built-in method, which checks the JWT and allows the access to be retained

Did I get it right? Is that the intended behavior? Is there any way for the plugin to disallow the access in this case (i.e. override the groups reported from JWT)?

[mbtools]

Afaik, yes, this is how it works. All this predates my engagement with Verdaccio and the repo is lacking tests for such scenarios. So I can only deduct from the code that this was intended, and that "deny-access" was not foreseen. The method names are "allow_..." after all.

I understand that the validity period of the JWT is a problem if groups change dynamically. Anything based on config.yaml is similarly limited to dynamic changes like package access or filter rules. The key is to first document - as you mostly did above - the exact requirements. Then we can look for the best solution (in Verdaccio or plugins).

PS: Keep in mind that Verdaccio is maintained by unpaid volunteers...

[dominykas]

I'll see if I can open a PR to update the docs for this.

Yeah, dynamic access or filters kind of makes sense - if it's in the config.yaml, then it's an app startup condition and then you do need to restart the app anyways.

The JWT bit is a bit unexpected though...

What would the correct way of approaching this be? I guess the plugin could skip returning the groups as part of the authenticate(), if it's going to revalidate them anyways? Otherwise I fail to see why allow_access is even needed on plugins?

From verdaccio perspective, it would probably make sense to record which plugin generated the JWT, and only use that plugin for allow_access (no fallbacks, no anything - plugin becomes the owner of the auth?).

At this point I'm just trying to figure out what the correct way of looking at this is - depending on what that is, I might be able to open some PRs, but ofc that needs guidance and agreement.

[mbtools]

Limiting the JWT to the generating plugin makes sense. However, I'm not sure if this is feasible with the current design. It's probably breaking, too, since other multi-plugin setups might depend on the current behavior.

[dominykas]

Yeah, I figure that's breaking.

Is not setting the groups for the user then the right way to go, as a plugin implementer?