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

Rjm/remoteuser #42

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ A Cornell University CIT Custom Development starter kit package for Laravel.
```
This will publish a `README.md` and `.lando.yml` file to the base directory, configured on the project settings. It will also publish HTML/CSS/JS assets from [cwd_framework_lite](https://github.com/CU-CommunityApps/cwd_framework_lite) and a set of [view components](https://laravel.com/docs/9.x/blade#layouts-using-components) that can be used to begin a layout (see `views/cwd-framework-index.blade.php` for example usage).

Optional:
To publish the configuration file for the Starter Kit
```shell
php artisan vendor:publish --tag="starterkit-config"
```

## Contributing

Anyone on the Custom Development team should be welcome and able to contribute. See [CONTRIBUTING](CONTRIBUTING.md) for details on how be involved and provide quality contributions.
24 changes: 24 additions & 0 deletions config/starterkit.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

return [

/*
* The users' netid may be retrieved from an environment variable
* populated by the Apache shibboleth module.
*
* The default env variable is REMOTE USER, but this may be e.g,
* REDIRECT_REMOTE_USER on some servers, depending on how PHP is installed.
*
* This configuration allows configuration of the remote_user_variable so it
* is not hardcoded in the application.
*
* In project code, use env(config('starterkit.remote_user_variable')) to retrieve the netid
*
* For local development without shibboleth, you can add
* REMOTE_USER=<netid>
* to your project .env file to set the user netid.
*
*/
'remote_user_variable' => env("REMOTE_USER_VARIABLE", "REMOTE_USER"),

Choose a reason for hiding this comment

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

I think this PR is a great idea. I just suggest to name it 'remote_user' instead of 'remote_user_variable'. I think 'remote_user_variable' is too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worried that it would be too confusing, but as long as people used env(config('starterkit.remote_user')) it would be ok. Thoughts, @woodseowl ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the long name is appropriate. remote_user as a name suggests to me that it is some kind of entity.

Choose a reason for hiding this comment

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

I think '_variable' does not give any clue that it is local. Maybe 'remote_user_app' then?
Another question: do we need a separate "starterkit" config file? Would it be cleaner to use config/app.php. It is almost empty. We are using app.js and app.css for other application customizations.

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 value is the name of the environment variable used by the server (Apache module / PHP), so it really is a variable. We could call it remote_user_environment_variable to be most clear. I think that @woodsowl's idea of putting this in its own package with the middleware is a good one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is in a package we don't have access to config/app.php. starterkit is the shortname of the package, so that is what the config fille is called. If we push this into it's own cu-auth package as Eric suggests, then it can go in cu-auth.php (or cuauth.php? I'm not sure what happens to the dash in the package template).

Choose a reason for hiding this comment

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

I guess we all agree about location: config/cu-auth.php


];
1 change: 1 addition & 0 deletions src/StarterKitServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public function configurePackage(Package $package): void
{
$package
->name(self::PACKAGE_NAME)
->hasConfigFile()
->hasInstallCommand(function (InstallCommand $command) {
$command->startWith(fn (InstallCommand $c) => $this->install($c));
});
Expand Down