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

feat/oauth #69

Merged
merged 15 commits into from
Feb 23, 2024
Merged

feat/oauth #69

merged 15 commits into from
Feb 23, 2024

Conversation

puria
Copy link
Member

@puria puria commented Feb 9, 2024

  • feat: first draft Slangroom plugin for OAuth
  • refactor: first draft using AuthorizationCodeModel
  • refactor: generate AccessToken with JWK as input
  • feat: create AuthorizationCode with default authentication

@RebeccaSelvaggini RebeccaSelvaggini marked this pull request as ready for review February 21, 2024 10:44
@andrea-dintino
Copy link
Member

andrea-dintino commented Feb 22, 2024

@puria the statement here:

https://github.com/dyne/slangroom/blob/feat/oauth/pkg/oauth/test/e2e.ts#L55

Takes 4 objects as input: 3 out of 4 "body", "headers" and "client" come from the client request at the same time.

Should we create a scheme in Slangroom that packages those 3, in order to shorten the Slangroom statement?
We could do the same with the statement https://github.com/dyne/slangroom/blob/feat/oauth/pkg/oauth/test/e2e.ts#L109

We need to investigate if this would impact interoperability.

package.json Outdated Show resolved Hide resolved
Comment on lines 16 to 34
if (!options.model) {
throw new InvalidArgumentError('Missing parameter: `model`');
}

if (!options.model.getAccessToken) {
throw new InvalidArgumentError('Invalid argument: model does not implement `getAccessToken()`');
}

if (options.scope && undefined === options.addAcceptedScopesHeader) {
throw new InvalidArgumentError('Missing parameter: `addAcceptedScopesHeader`');
}

if (options.scope && undefined === options.addAuthorizedScopesHeader) {
throw new InvalidArgumentError('Missing parameter: `addAuthorizedScopesHeader`');
}
// console.log(options.scope && !options.model.verifyScope) = undefined but pass the check
if (options.scope && !options.model.verifyScope) {
throw new InvalidArgumentError('Invalid argument: model does not implement `verifyScope()`');
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Bravo good, job. Nice error description, user oriented. Maybe you also want to look at Some[1] or Any functions when you want to check more than one values at once (like 2 or 3 if's it's okay in this case)

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

pkg/oauth/src/authenticateHandler.ts Outdated Show resolved Hide resolved
pkg/oauth/src/authenticateHandler.ts Outdated Show resolved Hide resolved
pkg/oauth/src/authenticateHandler.ts Outdated Show resolved Hide resolved
pkg/oauth/src/model.ts Outdated Show resolved Hide resolved
pkg/oauth/src/model.ts Outdated Show resolved Hide resolved
pkg/oauth/src/model.ts Outdated Show resolved Hide resolved
pkg/oauth/src/model.ts Outdated Show resolved Hide resolved
pkg/oauth/src/plugin.ts Outdated Show resolved Hide resolved
@puria puria merged commit 2d912d1 into main Feb 23, 2024
3 checks passed
@puria puria deleted the feat/oauth branch February 23, 2024 08:56
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.

3 participants