Commit 177cd7a
authored
Add access token-related functionality including auto-refresh (#83)
The following new functionality has been **extracted** from
`code-club-frontend` and `experience-cs` to reduce duplication and
hopefully make useful functionality available to other Rails apps using
this gem.
[This pull
request](https://github.com/RaspberryPiFoundation/experience-cs/pull/295)
where it was recently added to `experience-cs` and [this follow-up pull
request](https://github.com/RaspberryPiFoundation/experience-cs/pull/315)
are good references.
* ✅ I've used the new `rpi-auth` code in the context of `experience-cs`
in [this pull
request](https://github.com/RaspberryPiFoundation/experience-cs/pull/317)
and it seems to be OK.
* ✅ I've used the new `rpi-auth` code in the context of
`code-club-frontend` in [this pull
request](https://github.com/RaspberryPiFoundation/code-club-frontend/pull/280)
and all the test pass.
## Add `RpiAuth::Models::WithTokens` concern
This can optionally be included into your user model in order to obtain
an access token, a refresh token, and an expiry time when logging in.
These attributes are set by the same mechanism in
`AuthController#callback` that populates `user_id` on the user via
`Authenticatable.from_omniauth`, but instead calls
`WithTokens.from_omniauth` which in turn calls
`Authenticatable.from_omniauth` via the ancestor chain.
This also relies on:
- `RpiAuth.configuration.scope` including the "offline" scope in the
Rails app which is using the `rpi_auth` gem.
- In the `profile` app `hydra_client` config for the Rails app,
`grant_types` must include "refresh_token" and `scope` must include
"offline".
This has been substantially copied from `code-club-frontend`:
- [`app/models/user.rb`][1]
- [`spec/models/user_spec.rb`][2]
## Add `RpiAuth::Controllers::AutoRefreshingToken` concern
This can optionally be included into your controller to automatically
use the user's refresh token to obtain a new access token when the old
one expires. It adds a before action to the target controller which
automatically calls `OauthClient#refresh_credentials!` if the user is
signed in and their access token has expired. The latter makes a request
to the `profile` app using the refresh token to obtain a new access
token.
Again this has been substantially copied from `code-club-frontend`:
- [`app/controllers/application_controller.rb`][3]
- [`spec/requests/refresh_credentials_spec.rb`][4]
- [`lib/oauth_client.rb`][5]
- [`spec/lib/oauth_client_spec.rb`][6]
## Questions/issues
In general I'd prefer to postpone tackling any of the following to
separate PRs, because I think it's useful to have a version of the code
which matches what's currently in `code-club-frontend` & `experience-cs`
as closely as possible before we start changing things further. However,
hopefully the following notes are useful:
- I haven't included the aliasing of `#user_id` & `#user_id=` to `#id` &
`#id=` respectively, because I'm not yet sure whether it's more
generally useful - we don't seem to need it in `experience-cs` yet.
- I think it might simplify things a bit to move some of the
classes/modules that are currently in `lib` into relevant directories
under `app`, e.g. I think that moving
`lib/rpi_auth/models/authenticatable.rb` ->
`app/models/rpi_auth/authenticatable.rb` (or maybe
`app/models/concerns/rpi_auth/authenticatable.rb`) would mean the
classes were automatically loaded. This seems like a more idiomatic use
of a Rails engine. However, that's probably a bigger piece of work,
especially to make sure it works with the relevant versions of Rails.
- I'm slightly concerned about a few details of the implementation of
`AutoRefreshingToken#refresh_credentials_if_needed`:
- Rescuing all `ArgumentError` exceptions seems risky, because it's
quite a common exception. It would be better to be more specific.
- Even rescuing all `OAuth2::Error` exceptions seems a bit broad,
although I haven't investigated what this encompasses. At the very least
it would be good to log/report the details of the exception.
- While resetting the session in the event of an exception seems OK
security-wise, it probably doesn't result in a great user experience. It
would be better to redirect and/or display an error message to the user
to explain what's happened.
[1]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/models/user.rb
[2]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/models/user_spec.rb
[3]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/controllers/application_controller.rb#L8
[4]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/requests/refresh_credentials_spec.rb
[5]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/lib/oauth_client.rb
[6]:
https://github.com/RaspberryPiFoundation/code-club-frontend/blob/main/spec/lib/oauth_client_spec.rbFile tree
21 files changed
+504
-7
lines changed- gemfiles
- lib
- rpi_auth
- controllers
- models
- spec
- dummy
- app
- controllers
- models
- spec/requests
- rpi_auth
- models
- support
21 files changed
+504
-7
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
| 15 | + | |
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
46 | | - | |
| 46 | + | |
| 47 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
187 | 187 | | |
188 | 188 | | |
189 | 189 | | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
190 | 213 | | |
191 | 214 | | |
192 | 215 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
74 | 75 | | |
75 | 76 | | |
76 | 77 | | |
| 78 | + | |
77 | 79 | | |
78 | 80 | | |
79 | 81 | | |
| |||
88 | 90 | | |
89 | 91 | | |
90 | 92 | | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
91 | 96 | | |
92 | 97 | | |
93 | 98 | | |
| |||
106 | 111 | | |
107 | 112 | | |
108 | 113 | | |
| 114 | + | |
109 | 115 | | |
110 | 116 | | |
111 | 117 | | |
| |||
117 | 123 | | |
118 | 124 | | |
119 | 125 | | |
| 126 | + | |
| 127 | + | |
120 | 128 | | |
121 | 129 | | |
122 | 130 | | |
| |||
137 | 145 | | |
138 | 146 | | |
139 | 147 | | |
| 148 | + | |
| 149 | + | |
140 | 150 | | |
141 | 151 | | |
142 | 152 | | |
| |||
152 | 162 | | |
153 | 163 | | |
154 | 164 | | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
155 | 172 | | |
156 | 173 | | |
157 | 174 | | |
| |||
237 | 254 | | |
238 | 255 | | |
239 | 256 | | |
| 257 | + | |
240 | 258 | | |
241 | 259 | | |
242 | 260 | | |
| |||
290 | 308 | | |
291 | 309 | | |
292 | 310 | | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
293 | 314 | | |
294 | 315 | | |
295 | 316 | | |
| |||
313 | 334 | | |
314 | 335 | | |
315 | 336 | | |
| 337 | + | |
316 | 338 | | |
317 | 339 | | |
318 | 340 | | |
319 | 341 | | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
320 | 346 | | |
321 | 347 | | |
322 | 348 | | |
| |||
342 | 368 | | |
343 | 369 | | |
344 | 370 | | |
| 371 | + | |
345 | 372 | | |
346 | 373 | | |
347 | 374 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
80 | 81 | | |
81 | 82 | | |
82 | 83 | | |
| 84 | + | |
83 | 85 | | |
84 | 86 | | |
85 | 87 | | |
| |||
94 | 96 | | |
95 | 97 | | |
96 | 98 | | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
97 | 102 | | |
98 | 103 | | |
99 | 104 | | |
| |||
112 | 117 | | |
113 | 118 | | |
114 | 119 | | |
| 120 | + | |
115 | 121 | | |
116 | 122 | | |
117 | 123 | | |
| |||
123 | 129 | | |
124 | 130 | | |
125 | 131 | | |
| 132 | + | |
| 133 | + | |
126 | 134 | | |
127 | 135 | | |
128 | 136 | | |
| |||
143 | 151 | | |
144 | 152 | | |
145 | 153 | | |
| 154 | + | |
| 155 | + | |
146 | 156 | | |
147 | 157 | | |
148 | 158 | | |
| |||
158 | 168 | | |
159 | 169 | | |
160 | 170 | | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
161 | 178 | | |
162 | 179 | | |
163 | 180 | | |
| |||
243 | 260 | | |
244 | 261 | | |
245 | 262 | | |
| 263 | + | |
246 | 264 | | |
247 | 265 | | |
248 | 266 | | |
| |||
296 | 314 | | |
297 | 315 | | |
298 | 316 | | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
299 | 320 | | |
300 | 321 | | |
301 | 322 | | |
| |||
312 | 333 | | |
313 | 334 | | |
314 | 335 | | |
| 336 | + | |
315 | 337 | | |
316 | 338 | | |
317 | 339 | | |
318 | 340 | | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
319 | 345 | | |
320 | 346 | | |
321 | 347 | | |
| |||
341 | 367 | | |
342 | 368 | | |
343 | 369 | | |
| 370 | + | |
344 | 371 | | |
345 | 372 | | |
346 | 373 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
109 | 110 | | |
110 | 111 | | |
111 | 112 | | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
112 | 116 | | |
113 | 117 | | |
114 | 118 | | |
| |||
128 | 132 | | |
129 | 133 | | |
130 | 134 | | |
| 135 | + | |
131 | 136 | | |
132 | 137 | | |
133 | 138 | | |
| |||
144 | 149 | | |
145 | 150 | | |
146 | 151 | | |
| 152 | + | |
| 153 | + | |
147 | 154 | | |
148 | 155 | | |
149 | 156 | | |
| |||
164 | 171 | | |
165 | 172 | | |
166 | 173 | | |
| 174 | + | |
| 175 | + | |
167 | 176 | | |
168 | 177 | | |
169 | 178 | | |
| |||
180 | 189 | | |
181 | 190 | | |
182 | 191 | | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
183 | 199 | | |
184 | 200 | | |
185 | 201 | | |
| |||
282 | 298 | | |
283 | 299 | | |
284 | 300 | | |
| 301 | + | |
285 | 302 | | |
286 | 303 | | |
287 | 304 | | |
| |||
336 | 353 | | |
337 | 354 | | |
338 | 355 | | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
339 | 359 | | |
340 | 360 | | |
341 | 361 | | |
| |||
353 | 373 | | |
354 | 374 | | |
355 | 375 | | |
| 376 | + | |
356 | 377 | | |
357 | 378 | | |
358 | 379 | | |
359 | 380 | | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
360 | 385 | | |
361 | 386 | | |
362 | 387 | | |
| |||
382 | 407 | | |
383 | 408 | | |
384 | 409 | | |
| 410 | + | |
385 | 411 | | |
386 | 412 | | |
387 | 413 | | |
0 commit comments