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

New Feature - Authentication Middleware Injection #123

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

leoraba
Copy link
Contributor

@leoraba leoraba commented Dec 11, 2024

Enhancements and Features

  • Authentication Middleware Injection: Added a new property in the provider package to support a custom auth implementation in the middleware. Applications using the Lyric provider can now pass a function customAuthHandler to enforce their authentication logic.

  • User Property Utilization: Updated Lyric to use the user property from the incoming Request object to obtain username.

  • Exported UserSession Type: Introduced and exported the UserSession type to define user related properties. Lyric will use the user property from the incoming Request object to obtain the user name.

  • Documentation Updates: Updated the documentation to include guidelines on implementing and integrating a custom authentication middleware.

@leoraba leoraba force-pushed the feat/auth-ego-token branch from f7d38d9 to e72b37b Compare December 11, 2024 16:02
}
}

export const authMiddleware = (req: Request, res: Response, next: NextFunction) => {
Copy link
Contributor Author

@leoraba leoraba Dec 11, 2024

Choose a reason for hiding this comment

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

Server in Lyric monorepo doesn't implement any authentication, although it provides a basic middleware to demonstrate how to set user property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Retrieving user details from request if provided.

@leoraba leoraba changed the title retrieve username from userSession Feat/ Retrieve username from user session Dec 11, 2024
@leoraba leoraba changed the title Feat/ Retrieve username from user session Feat/ Retrieve username from incoming request Dec 11, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating Docs. Adding section to implement auth middleware and database migrations

Copy link
Member

@justincorrigible justincorrigible Jan 29, 2025

Choose a reason for hiding this comment

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

those examples are really helpful! wonder if it would be more useful for integrators to have those as actual files instead (like a ready to copy and customize scaffolding)

thoughts?

Copy link
Contributor Author

@leoraba leoraba Jan 29, 2025

Choose a reason for hiding this comment

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

I think providing our database migration script allows integrators to ensure Lyric will keep track of codebase and database schema in sync. For example, integrators over time can jump to latest version of Lyric, and they will be sure the database schema will be updated as well. In essence, how Lyric stores data shouldn't be a headache for implementers.

@@ -48,6 +49,7 @@ export type AppConfig = {
limits: LimitsConfig;
logger: LoggerConfig;
schemaService: SchemaServiceConfig;
authMiddleware?: (req: Request, res: Response, next: NextFunction) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applications that implements Lyric provider will pass a custom middleware for authentication

@leoraba leoraba changed the title Feat/ Retrieve username from incoming request New Feature - Authentication Middleware Injection Dec 11, 2024
@leoraba leoraba mentioned this pull request Jan 9, 2025
2 tasks
@leoraba leoraba linked an issue Jan 9, 2025 that may be closed by this pull request
2 tasks
@leoraba leoraba linked an issue Jan 13, 2025 that may be closed by this pull request
// TODO: auth here
console.log(`auth`);
next();
export const authMiddleware = (authConfig: AuthConfig) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Middleware used when auth is enabled.
It uses the customAuthHanlder to performs the auth.
It returns to next() handler if user is valid authenticated or return a 401 or 403 error.

* @param req - Express request object
* @returns User session result
*/
export const authHandler = (_req: Request) => {
Copy link
Contributor Author

@leoraba leoraba Jan 22, 2025

Choose a reason for hiding this comment

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

This is the implementation of an auth handler used to provider Guest access.

Choose a reason for hiding this comment

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

I don't know the whole project context, so I'm wondering why the readme example for authHandler contains actual auth logic, but the actual authHandler implementation just returns a hardcoded value

Choose a reason for hiding this comment

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

I think the answer is related to this? #123 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The handler implemented in this apps/server package is a basic implementation for demonstration, however, the details how to customize it is described in readme file

README.md Outdated
@@ -88,6 +88,7 @@ The Environment Variables used for this application are listed in the table bell
| `PLURALIZE_SCHEMAS_ENABLED` | This feature automatically convert schema names to their plural forms when handling compound documents. Pluralization assumes the words are in English | true |
| `PORT` | Server Port. | 3030 |
| `UPLOAD_LIMIT` | Limit upload file size in string or number. <br>Supported units and abbreviations are as follows and are case-insensitive: <br> - b for bytes<br> - kb for kilobytes<br>- mb for megabytes<br>- gb for gigabytes<br>- tb for terabytes<br>- pb for petabytes<br>Any other text is considered as byte | '10mb' |
| `ALLOWED_ORIGINS` | Specifies a list of permitted origins for Cross-Origin Resource Sharing (CORS). These origins, separated by commas, are allowed to make requests to the server, ensuring only trusted domains can access resources. | |

Choose a reason for hiding this comment

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

Example value could be helpful here

},
};


const lyricProvider = provider(appConfig);
```

Use any of the resources available on provider on a Express server:
### Auth Custom Hanlder

Choose a reason for hiding this comment

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

Typo, should be Handler


```

```

Choose a reason for hiding this comment

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

This code block looks unused?

@demariadaniel
Copy link

A couple recommended tidying tasks but all in all this looks like a very clean implementation. I'll leave some time for team members closer to the project to take a look, but it looks good to me!

if (!origin) {
// allow requests with no origin
// (like mobile apps or curl requests)
return callback(null, true);
Copy link
Member

Choose a reason for hiding this comment

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

do we want to add a feature flag (env var config) to enable this, and have it disabled by default?
doesn't feel right to just have it "open" like this

Copy link
Contributor Author

@leoraba leoraba Jan 29, 2025

Choose a reason for hiding this comment

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

Thanks for noticing this. In regards to security this should be disabled by default.
And could be converted into a variable named CORS_ENABLED where when true only registered domains can access the server, otherwise rejected (default)

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.

WW - Built authentication on lyric WW - Lyric
3 participants