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

Jump to bottom

[KEYCLOAK-6655] Javascript Adapter - Allow users to provide cordova-specific options to login and register #4998

Merged
merged 3 commits into from May 6, 2018
Merged

[KEYCLOAK-6655] Javascript Adapter - Allow users to provide cordova-specific options to login and register #4998

merged 3 commits into from May 6, 2018

Conversation

looorent
Copy link
Contributor

In keycloak-js (version 3.4.3), login and register functions do not allow to pass cordova-specific options to the in-app-browser.

This PR proposes an additional cordovaOptions key (to the options parameter).
cordovaOptions is an object where is key/value is passed to the in-app-browser.

For example, this parameter disables the in-app-browser's zoom capability on Android:

var options = {
  cordovaOptions: {
     zoom= "no"
  }
}

hidden and location options are not affected by this new parameter.

Available options are defined at https://cordova.apache.org/docs/en/latest/reference/cordova-plugin-inappbrowser/

@looorent
Javascript Adapter - Allow users to pass cordova-specific options (in…
9043e64 
…-app-browser) to the login and register functions
@looorent looorent mentioned this pull request Feb 14, 2018
Closed
@looorent
Javascript Adapter - Allow users to pass cordova-specific options (in…
65786f3 
…-app-browser) to the login and register functions
@looorent looorent changed the title Javascript Adapter - Allow users to provide cordova-specific options to login and register [KEYCLOAK-6655] Javascript Adapter - Allow users to provide cordova-specific options to login and register Feb 19, 2018
@stianst stianst self-assigned this Feb 20, 2018
@stianst
Copy link
Contributor

stianst commented Apr 13, 2018

Looks good to me. Could you also update the documentation:
https://github.com/keycloak/keycloak-documentation/blob/master/securing_apps/topics/oidc/javascript-adapter.adoc

@mhajas could you take a look as well? Perhaps you could also try it out? We don't have any Cordova tests yet right? So we can't really add any tests here.

@stianst stianst requested a review from mhajas April 13, 2018 07:04
@stianst
Copy link
Contributor

stianst commented Apr 20, 2018

@mhajas bump

mhajas
mhajas approved these changes Apr 20, 2018
View reviewed changes
Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

This PR looks good. I tried it on app in examples with my android mobile device and it works pretty good :-) @stianst after removing the empty line, this is good to go for me

adapters/oidc/js/src/main/resources/keycloak.js
@@ -1218,7 +1242,7 @@

logout: function(options) {
var promise = createPromise();

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this empty line

adapters/oidc/js/src/main/resources/keycloak.js
@@ -1253,7 +1277,8 @@

register : function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a notion: It is not possible to add app-specific options for registration. Probably because registration is mostly invoked from the login page and not from some application page. Maybe it is possible to somehow get around this for example with some global options specified by a creator of the app. @looorent and @stianst what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the options to register function for the Cordova moda is missing for some reason. It does have it for the default mode. Not sure why, but sure that's not correct. I don't think it's in the scope of this PR though as it's already missing in master.

stianst
stianst approved these changes May 6, 2018
View reviewed changes
adapters/oidc/js/src/main/resources/keycloak.js
@@ -1253,7 +1277,8 @@

register : function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the options to register function for the Cordova moda is missing for some reason. It does have it for the default mode. Not sure why, but sure that's not correct. I don't think it's in the scope of this PR though as it's already missing in master.

@stianst stianst merged commit f6125a2 into keycloak:master May 6, 2018
stokito pushed a commit to stokito/keycloak that referenced this pull request May 15, 2018
@looorent @stokito
[KEYCLOAK-6655] Javascript Adapter - Allow users to provide cordova-s…
d192c3a 
…pecific options to login and register (keycloak#4998)

* Javascript Adapter - Allow users to pass cordova-specific options (in-app-browser) to the login and register functions

* Javascript Adapter - Allow users to pass cordova-specific options (in-app-browser) to the login and register functions

* [KEYCLOAK-6655] On Android 8, explicit hidden=no fails on in-app-browser load.
@looorent looorent mentioned this pull request Jun 7, 2018
Merged
@looorent
Copy link
Contributor Author

looorent commented Jun 7, 2018

@stianst Sorry for the delay but here are two pull requests:

@stianst
Copy link
Contributor

stianst commented Jun 11, 2018

@looorent great thanks, both merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stianst stianst stianst approved these changes

@mhajas mhajas mhajas approved these changes

Assignees

@stianst stianst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@looorent @stianst @mhajas