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

Create an SPA sample. #8

Closed
wants to merge 1 commit into from

Conversation

shaunluttin
Copy link
Contributor

No description provided.

@ilmax
Copy link

ilmax commented Jul 26, 2016

Hy @shaunluttin what the status of this? Looks like a work in progress right? Btw a convention for folder naming was discussed with @PinpointTownes and we agreed on naming the folder corresponding to the implemented flow e.g. ImplicitFlow or ResourceOwnerPasswordCredentials.

We will need to update the current MVC sample folder name accordingly 😄

@@ -43,6 +43,8 @@ public class Startup {
.AllowAuthorizationCodeFlow()
.AllowRefreshTokenFlow()

.AddEphemeralSigningKey()
Copy link
Member

Choose a reason for hiding this comment

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

FYI, this missing line was added yesterday. Consider rebasing your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PinpointTownes Done.

Well, I rebased, but the change still appears. Maybe I didn't understand. My current history:

* b5670a4 (HEAD -> spa-sample, origin/spa-sample) Rename directory to CodeFlow.
* 15f9e1d Update readme.md.
* 3f3600c Seed a client.
* e020005 Obtain OpenID Provider Configuration Information.
* 929c423 Add OpenIddict and prerequisites.
* dbb7543 Configure WebHost.
* c81202e Add README.md.
* a43a1e6 Scaffold SPA sample.
| * 1b3fccd (origin/mvc-readme, mvc-readme) Create README for running MVC samples.
|/
* 4d03982 (upstream/master, origin/master, origin/HEAD, master) React to API changes in OpenIddict
* 9d332c0 Copy the MVC sample from the main repository
* 2aa39e0 Set up the initial project structure

Copy link
Member

Choose a reason for hiding this comment

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

Alright, looks like you'll have to remove it manually. Please push a new commit to get rid of this duplicate call.

Copy link
Member

Choose a reason for hiding this comment

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

This line is still there? 😄 We really don't want to generate 2 RSA keys at runtime (it takes time and there's no point doing that).

Copy link
Member

Choose a reason for hiding this comment

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

REVERT THAT!!!!!!!!!!!

@kevinchalet
Copy link
Member

We will need to update the current MVC sample folder name accordingly 😄

Lazy me, I've simply copied the MVC samples as-is without renaming them 😅
Thanks for the heads-up, I'll fix that.

@shaunluttin
Copy link
Contributor Author

@ilmax Definitely a work in progress. Probably about another 20 to 40 hours until completion.

@shaunluttin
Copy link
Contributor Author

@ilmax Thank you for letting me know about the naming convention. Renamed to CodeFlow.

@ilmax
Copy link

ilmax commented Jul 27, 2016

Well done, call me out for a review when your done implementing this

@shaunluttin
Copy link
Contributor Author

@ilmax While it isn't complete, login/logout now both work in a rudimentary fashion. If you have the time, I would love your feedback on the big picture decisions I am making. I.e. Is my overall architecture heading in a reasonable direction?

@kevinchalet
Copy link
Member

Looks promising. Just one remark: please consider excluding non-source JS files (like typings or external libraries). They add unnecessary noise.

@shaunluttin
Copy link
Contributor Author

shaunluttin commented Aug 6, 2016

@PinpointTownes Done. Thank you for that feedback. 1. I removed the the TypeScript definitions that typings manages. I left the custom_typings though, because they are not external but are my own definitions and are helpful to anyone trying to work with the code. 2. I think that there are no more external libraries there any more. If I'm mistaken, I welcome feedback.

@shaunluttin shaunluttin force-pushed the spa-sample branch 8 times, most recently from 645e9e7 to aea0c73 Compare August 10, 2016 18:54
@@ -0,0 +1,4 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

@shaunluttin curious: do we need all these "generator files" in the final code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PinpointTownes It appears that we do not need them. They are for code generation via the Aurelia CLI. Question: would you delete and purge them from the history too or just delete and commit?

https://www.npmjs.com/package/aurelia-cli#generators

Copy link
Member

Choose a reason for hiding this comment

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

Just delete and commit. The related commits will disappear when squashing (before merging your PR).

// Enable the authorization, logout, token and userinfo endpoints.
.EnableAuthorizationEndpoint("/connect/authorize")
.EnableLogoutEndpoint("/connect/logout")
.EnableTokenEndpoint("/connect/token")
Copy link
Member

Choose a reason for hiding this comment

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

Note: the token endpoint is not used for the implicit flow. Consider removing this line to disable it.


namespace ResourceServer01.Controllers
{
[Route("")]
Copy link
Member

Choose a reason for hiding this comment

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

There are still weird empty routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the simplest way that I know for having an action handle the empty route.

@shaunluttin shaunluttin force-pushed the spa-sample branch 2 times, most recently from 6b61a73 to 8b32560 Compare September 9, 2016 04:45
@kevinchalet
Copy link
Member

FYI, there are still a bunch of missing NPM modules: through2, minimatch and source-map.

options.ClientSecret = "secret_secret_secret";
});

app.UseStatusCodePagesWithReExecute("/error");
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to kill that.

@kevinchalet
Copy link
Member

Alright, looks like it's almost ready. Please fix the missing modules (flush your node_modules and restore everything before pushing to ensure nothing's missing) and the 2 or 3 nits I just added and I'll merge your PR today or tomorrow 👏

Thanks for this great contribution.

@shaunluttin
Copy link
Contributor Author

shaunluttin commented Sep 16, 2016

I've run git clean -xfd and also deleted ~\AppData\Roaming\npm.

I've also run the following, because git was not able to delete those deep node_modules.

Get-ChildItem -Recurse -Directory -Depth 5 | `
    ? { $_.Name -eq "node_modules" } | `
    % { rimraf $_.FullName }

Despite all that, I'm not missing any dependencies when running the demo. It might have to do with your node version. These are mine:

> node -v
v6.3.0
> npm -v 
3.10.3

I'll try running it on my spouses computer later tonight.

@shaunluttin shaunluttin force-pushed the spa-sample branch 2 times, most recently from 111f731 to 16b59a4 Compare September 22, 2016 18:27
@kevinchalet
Copy link
Member

Merged, thanks for this great sample! 5971adc

@shaunluttin shaunluttin deleted the spa-sample branch September 25, 2016 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants